Bug 127648 - -[JSContext evaluteScript:] calls JSEvaluteScript with startingLineNumber 0, later interpreted as a oneBasedInt
Summary: -[JSContext evaluteScript:] calls JSEvaluteScript with startingLineNumber 0, ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-26 02:11 PST by Joseph Pecoraro
Modified: 2014-01-27 23:25 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (17.20 KB, patch)
2014-01-27 17:11 PST, Joseph Pecoraro
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-01-26 02:11:06 PST
The inspector, via JSC::Debugger::scriptDidParse is seeing -[JSContext evaluteScript:] calls as scripts evaluated with an empty sourceURL starting at line -1. This can be tracked back to -[JSContext evaluteScript:] calling JSEvaluteScript with a 0 line number.

Seems like the JSEvaluteScript API should document that the lineNumber parameter is 1 based and we should update -[JSContext evaluteScript] to pass line 1 instead of 0. This would lead to better line numbers in the inspector when debugging a JSContext.

NOTE: There is also a FIXME in Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp that can be fixed:

    void InspectorDebuggerAgent::didParseSource(JSC::SourceID sourceID, const Script& inScript)
    {
        Script script = inScript;
        // FIXME: Why does -[JSContext evaluateScript] have a -1 startLine?
        if (script.startLine <= 0 && !script.startColumn)
            script.sourceURL = ContentSearchUtilities::findScriptSourceURL(script.source);
        ...
    }

Seeing as JSEvaluteScript is API and can be with any lineNumber, we might need to make the inspector (or the API) better support negative numbers.
Comment 1 Joseph Pecoraro 2014-01-27 15:26:41 PST
Although the documentation says these startingLineNumbers are "only used when reporting exceptions", now that we have the inspector the values can show in other places.

Lets clamp at all the API entry points. It never make sense to have a negative lineNumber.
Comment 2 Joseph Pecoraro 2014-01-27 16:55:13 PST
Ahh. I think I've worked this out.

In all the public JSC APIs with a startingLineNumber parameter it can only show up in JavaScript exceptions. And in those cases, the exception line number is correct, because ultimately the JSC::Source the APIs create does clamping of the line number appropriately. (See JSC::Source constructors and makeSource(...)).

However, for the same inputs, the JSC::SourceProvider created in the APIs has unclamped values. And it is these unclamped values that are passed to the inspector through JSC::Debugger::sourceParsed, and lead to the unexpected values seen by the inspector. As far as I can tell, JSC::SourceProvider's startPosition is _only_ used by the inspector.

I still think we should clamp at the API boundary.
Comment 3 Joseph Pecoraro 2014-01-27 17:11:12 PST
Created attachment 222384 [details]
[PATCH] Proposed Fix
Comment 4 Geoffrey Garen 2014-01-27 21:46:46 PST
Comment on attachment 222384 [details]
[PATCH] Proposed Fix

r=me

I'd remove "is interpreted", to be more direct, and hyphenate "one-based", since it's an adjective.
Comment 5 Joseph Pecoraro 2014-01-27 21:57:12 PST
(In reply to comment #4)
> (From update of attachment 222384 [details])
> r=me
> 
> I'd remove "is interpreted", to be more direct, and hyphenate "one-based", since it's an adjective.

Oo good suggestions. Will do.
Comment 6 Joseph Pecoraro 2014-01-27 23:25:52 PST
Landed <http://trac.webkit.org/changeset/162918> with rewording.