Skip to content

Commit fabc11a

Browse files
JosephPecoraroJosephPecoraro
committed
-[JSContext evaluteScript:] calls JSEvaluteScript with startingLineNumber 0, later interpreted as a oneBasedInt
https://bugs.webkit.org/show_bug.cgi?id=127648 Reviewed by Geoffrey Garen. The actual bug being fixed here is that the line number for scripts evaluated via the JSC APIs is now sane. However, there is no good infrastructure in place right now to test that. * API/tests/testapi.c: (main): * API/tests/testapi.mm: (testObjectiveCAPI): Add tests for exception line numbers and handling of bad startingLineNumbers in public APIs. These tests were already passing, I just add them to make sure they are not regressed in the future. * API/JSBase.cpp: (JSEvaluateScript): (JSCheckScriptSyntax): * API/JSBase.h: * API/JSObjectRef.cpp: (JSObjectMakeFunction): * API/JSObjectRef.h: * API/JSScriptRef.cpp: * API/JSScriptRefPrivate.h: * API/JSStringRef.h: - Clarify documentation that startingLineNumber is 1 based and clamped. - Add clamping in the implementation to put sane values into JSC::SourceProvider. * inspector/agents/InspectorDebuggerAgent.cpp: (Inspector::InspectorDebuggerAgent::didParseSource): Remove the FIXME now that the SourceProvider is giving us expected values. Canonical link: https://commits.webkit.org/145762@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@162918 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 0c0f39e commit fabc11a

11 files changed

Lines changed: 95 additions & 14 deletions

File tree

‎Source/JavaScriptCore/API/JSBase.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ JSValueRef JSEvaluateScript(JSContextRef ctx, JSStringRef script, JSObjectRef th
5353

5454
JSObject* jsThisObject = toJS(thisObject);
5555

56+
startingLineNumber = std::max(1, startingLineNumber);
57+
5658
// evaluate sets "this" to the global object if it is NULL
5759
JSGlobalObject* globalObject = exec->vmEntryGlobalObject();
5860
SourceCode source = makeSource(script->string(), sourceURL->string(), TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber::first()));
@@ -82,6 +84,8 @@ bool JSCheckScriptSyntax(JSContextRef ctx, JSStringRef script, JSStringRef sourc
8284
ExecState* exec = toJS(ctx);
8385
APIEntryShim entryShim(exec);
8486

87+
startingLineNumber = std::max(1, startingLineNumber);
88+
8589
SourceCode source = makeSource(script->string(), sourceURL->string(), TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber::first()));
8690

8791
JSValue syntaxException;

‎Source/JavaScriptCore/API/JSBase.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
2121
* OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
2222
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
23-
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
23+
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2424
*/
2525

2626
#ifndef JSBase_h
@@ -102,7 +102,7 @@ extern "C" {
102102
@param script A JSString containing the script to evaluate.
103103
@param thisObject The object to use as "this," or NULL to use the global object as "this."
104104
@param sourceURL A JSString containing a URL for the script's source file. This is only used when reporting exceptions. Pass NULL if you do not care to include source file information in exceptions.
105-
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions.
105+
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions. The value is one-based, so the first line is line 1 and invalid values are clamped to 1.
106106
@param exception A pointer to a JSValueRef in which to store an exception, if any. Pass NULL if you do not care to store an exception.
107107
@result The JSValue that results from evaluating script, or NULL if an exception is thrown.
108108
*/
@@ -114,21 +114,21 @@ JS_EXPORT JSValueRef JSEvaluateScript(JSContextRef ctx, JSStringRef script, JSOb
114114
@param ctx The execution context to use.
115115
@param script A JSString containing the script to check for syntax errors.
116116
@param sourceURL A JSString containing a URL for the script's source file. This is only used when reporting exceptions. Pass NULL if you do not care to include source file information in exceptions.
117-
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions.
117+
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions. The value is one-based, so the first line is line 1 and invalid values are clamped to 1.
118118
@param exception A pointer to a JSValueRef in which to store a syntax error exception, if any. Pass NULL if you do not care to store a syntax error exception.
119119
@result true if the script is syntactically correct, otherwise false.
120120
*/
121121
JS_EXPORT bool JSCheckScriptSyntax(JSContextRef ctx, JSStringRef script, JSStringRef sourceURL, int startingLineNumber, JSValueRef* exception);
122122

123123
/*!
124124
@function JSGarbageCollect
125-
@abstract Performs a JavaScript garbage collection.
125+
@abstract Performs a JavaScript garbage collection.
126126
@param ctx The execution context to use.
127-
@discussion JavaScript values that are on the machine stack, in a register,
128-
protected by JSValueProtect, set as the global object of an execution context,
127+
@discussion JavaScript values that are on the machine stack, in a register,
128+
protected by JSValueProtect, set as the global object of an execution context,
129129
or reachable from any such value will not be collected.
130130
131-
During JavaScript execution, you are not required to call this function; the
131+
During JavaScript execution, you are not required to call this function; the
132132
JavaScript engine will garbage collect as needed. JavaScript values created
133133
within a context group are automatically destroyed when the last reference
134134
to the context group is released.

‎Source/JavaScriptCore/API/JSObjectRef.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
2222
* OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
2323
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
24-
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
24+
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2525
*/
2626

2727
#include "config.h"
@@ -135,6 +135,7 @@ JSObjectRef JSObjectMakeFunction(JSContextRef ctx, JSStringRef name, unsigned pa
135135
ExecState* exec = toJS(ctx);
136136
APIEntryShim entryShim(exec);
137137

138+
startingLineNumber = std::max(1, startingLineNumber);
138139
Identifier nameID = name ? name->identifier(&exec->vm()) : Identifier(exec, "anonymous");
139140

140141
MarkedArgumentBuffer args;

‎Source/JavaScriptCore/API/JSObjectRef.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
2222
* OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
2323
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
24-
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
24+
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2525
*/
2626

2727
#ifndef JSObjectRef_h
@@ -485,7 +485,7 @@ JS_EXPORT JSObjectRef JSObjectMakeRegExp(JSContextRef ctx, size_t argumentCount,
485485
@param parameterNames A JSString array containing the names of the function's parameters. Pass NULL if parameterCount is 0.
486486
@param body A JSString containing the script to use as the function's body.
487487
@param sourceURL A JSString containing a URL for the script's source file. This is only used when reporting exceptions. Pass NULL if you do not care to include source file information in exceptions.
488-
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions.
488+
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions. The value is one-based, so the first line is line 1 and invalid values are clamped to 1.
489489
@param exception A pointer to a JSValueRef in which to store a syntax error exception, if any. Pass NULL if you do not care to store a syntax error exception.
490490
@result A JSObject that is a function, or NULL if either body or parameterNames contains a syntax error. The object's prototype will be the default function prototype.
491491
@discussion Use this method when you want to execute a script repeatedly, to avoid the cost of re-parsing the script before each execution.

‎Source/JavaScriptCore/API/JSScriptRef.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ JSScriptRef JSScriptCreateReferencingImmortalASCIIText(JSContextGroupRef context
8383
return 0;
8484
}
8585

86+
startingLineNumber = std::max(1, startingLineNumber);
87+
8688
RefPtr<OpaqueJSScript> result = OpaqueJSScript::create(vm, url->string(), startingLineNumber, String(StringImpl::createFromLiteral(source, length)));
8789

8890
ParserError error;
@@ -102,6 +104,8 @@ JSScriptRef JSScriptCreateFromString(JSContextGroupRef contextGroup, JSStringRef
102104
VM* vm = toJS(contextGroup);
103105
APIEntryShim entryShim(vm);
104106

107+
startingLineNumber = std::max(1, startingLineNumber);
108+
105109
RefPtr<OpaqueJSScript> result = OpaqueJSScript::create(vm, url->string(), startingLineNumber, source->string());
106110

107111
ParserError error;

‎Source/JavaScriptCore/API/JSScriptRefPrivate.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ extern "C" {
4242
@abstract Creates a script reference from an ascii string, without copying or taking ownership of the string
4343
@param contextGroup The context group the script is to be used in.
4444
@param url The source url to be reported in errors and exceptions.
45-
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions.
45+
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions. The value is one-based, so the first line is line 1 and invalid values are clamped to 1.
4646
@param source The source string. This is required to be pure ASCII and to never be deallocated.
4747
@param length The length of the source string.
4848
@param errorMessage A pointer to a JSStringRef in which to store the parse error message if the source is not valid. Pass NULL if you do not care to store an error message.
@@ -58,7 +58,7 @@ JS_EXPORT JSScriptRef JSScriptCreateReferencingImmortalASCIIText(JSContextGroupR
5858
@abstract Creates a script reference from a string
5959
@param contextGroup The context group the script is to be used in.
6060
@param url The source url to be reported in errors and exceptions.
61-
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions.
61+
@param startingLineNumber An integer value specifying the script's starting line number in the file located at sourceURL. This is only used when reporting exceptions. The value is one-based, so the first line is line 1 and invalid values are clamped to 1.
6262
@param source The source string.
6363
@param errorMessage A pointer to a JSStringRef in which to store the parse error message if the source is not valid. Pass NULL if you do not care to store an error message.
6464
@param errorLine A pointer to an int in which to store the line number of a parser error. Pass NULL if you do not care to store an error line.

‎Source/JavaScriptCore/API/JSStringRef.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
2121
* OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
2222
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
23-
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
23+
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2424
*/
2525

2626
#ifndef JSStringRef_h

‎Source/JavaScriptCore/API/tests/testapi.c‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1628,6 +1628,26 @@ int main(int argc, char* argv[])
16281628
JSStringRelease(functionBody);
16291629
JSStringRelease(line);
16301630

1631+
exception = NULL;
1632+
functionBody = JSStringCreateWithUTF8CString("rreturn Array;");
1633+
line = JSStringCreateWithUTF8CString("line");
1634+
ASSERT(!JSObjectMakeFunction(context, NULL, 0, NULL, functionBody, NULL, -42, &exception));
1635+
ASSERT(JSValueIsObject(context, exception));
1636+
v = JSObjectGetProperty(context, JSValueToObject(context, exception, NULL), line, NULL);
1637+
assertEqualsAsNumber(v, 1);
1638+
JSStringRelease(functionBody);
1639+
JSStringRelease(line);
1640+
1641+
exception = NULL;
1642+
functionBody = JSStringCreateWithUTF8CString("// Line one.\nrreturn Array;");
1643+
line = JSStringCreateWithUTF8CString("line");
1644+
ASSERT(!JSObjectMakeFunction(context, NULL, 0, NULL, functionBody, NULL, 1, &exception));
1645+
ASSERT(JSValueIsObject(context, exception));
1646+
v = JSObjectGetProperty(context, JSValueToObject(context, exception, NULL), line, NULL);
1647+
assertEqualsAsNumber(v, 2);
1648+
JSStringRelease(functionBody);
1649+
JSStringRelease(line);
1650+
16311651
exception = NULL;
16321652
functionBody = JSStringCreateWithUTF8CString("return Array;");
16331653
function = JSObjectMakeFunction(context, NULL, 0, NULL, functionBody, NULL, 1, &exception);

‎Source/JavaScriptCore/API/tests/testapi.mm‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,6 +592,22 @@ void testObjectiveCAPI()
592592
checkResult(@"JSContext.exceptionHandler", caught);
593593
}
594594

595+
@autoreleasepool {
596+
JSContext *context = [[JSContext alloc] init];
597+
__block int expectedExceptionLineNumber = 1;
598+
__block bool sawExpectedExceptionLineNumber = false;
599+
context.exceptionHandler = ^(JSContext *, JSValue *exception) {
600+
sawExpectedExceptionLineNumber = [exception[@"line"] toInt32] == expectedExceptionLineNumber;
601+
};
602+
[context evaluateScript:@"!@#$%^&*() THIS IS NOT VALID JAVASCRIPT SYNTAX !@#$%^&*()"];
603+
checkResult(@"evaluteScript exception on line 1", sawExpectedExceptionLineNumber);
604+
605+
expectedExceptionLineNumber = 2;
606+
sawExpectedExceptionLineNumber = false;
607+
[context evaluateScript:@"// Line 1\n!@#$%^&*() THIS IS NOT VALID JAVASCRIPT SYNTAX !@#$%^&*()"];
608+
checkResult(@"evaluteScript exception on line 2", sawExpectedExceptionLineNumber);
609+
}
610+
595611
@autoreleasepool {
596612
JSContext *context = [[JSContext alloc] init];
597613
context[@"callback"] = ^{

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,40 @@
1+
2014-01-27 Joseph Pecoraro <[email protected]>
2+
3+
-[JSContext evaluteScript:] calls JSEvaluteScript with startingLineNumber 0, later interpreted as a oneBasedInt
4+
https://bugs.webkit.org/show_bug.cgi?id=127648
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
The actual bug being fixed here is that the line number for
9+
scripts evaluated via the JSC APIs is now sane. However,
10+
there is no good infrastructure in place right now to test that.
11+
12+
* API/tests/testapi.c:
13+
(main):
14+
* API/tests/testapi.mm:
15+
(testObjectiveCAPI):
16+
Add tests for exception line numbers and handling of bad
17+
startingLineNumbers in public APIs. These tests were already
18+
passing, I just add them to make sure they are not regressed
19+
in the future.
20+
21+
* API/JSBase.cpp:
22+
(JSEvaluateScript):
23+
(JSCheckScriptSyntax):
24+
* API/JSBase.h:
25+
* API/JSObjectRef.cpp:
26+
(JSObjectMakeFunction):
27+
* API/JSObjectRef.h:
28+
* API/JSScriptRef.cpp:
29+
* API/JSScriptRefPrivate.h:
30+
* API/JSStringRef.h:
31+
- Clarify documentation that startingLineNumber is 1 based and clamped.
32+
- Add clamping in the implementation to put sane values into JSC::SourceProvider.
33+
34+
* inspector/agents/InspectorDebuggerAgent.cpp:
35+
(Inspector::InspectorDebuggerAgent::didParseSource):
36+
Remove the FIXME now that the SourceProvider is giving us expected values.
37+
138
2014-01-27 Joseph Pecoraro <[email protected]>
239

340
Web Inspector: CRASH when debugger closes remote inspecting JSContext

0 commit comments

Comments
 (0)