Bug 97032 – Web Inspector: load sourcemaps asynchronously, part 1/2

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[johnjbarton]

In CompilerScriptMapping.js loadSourceMapForScript we read:
   // FIXME: make sendRequest async.
However if the sendRequest is async, then the dependent code must also be async.

Thus the FIXME has two parts:
  1. Make the callers of CompilerScriptMapping.loadSourceMapForScript async,
  2. load the source map async

[johnjbarton]

Created attachment 164601 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164601 [details]
Patch

Attachment 164601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13898266

New failing tests:
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/compiler-script-mapping.html

[johnjbarton]

Created attachment 164624 [details]
Oh that's where this is tested

[Vsevolod Vlasov]

Comment on attachment 164624 [details]
Oh that's where this is tested

View in context: https://bugs.webkit.org/attachment.cgi?id=164624&action=review

> Source/WebCore/inspector/front-end/CompilerScriptMapping.js:96
> +                script.setSourceMapping(this);

This means the script won't have source mapping until source map is loaded, which means stopping on breakpoint at this point would break things.

[johnjbarton]

(In reply to comment #4)
> (From update of attachment 164624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164624&action=review
> 
> > Source/WebCore/inspector/front-end/CompilerScriptMapping.js:96
> > +                script.setSourceMapping(this);
> 
> This means the script won't have source mapping until source map is loaded, which means stopping on breakpoint at this point would break things.

Specifically, Script.rawLocationToUILocation will fail when this._sourceMapping is undefined, correct?

(I believe this already occurs, at least I recall seeing exceptions related to rawLocationToUILocation).

Assuming we want async, then it's impossible to avoid a time when the mapping will be incorrect. So I guess we need to minimize the damage.

Based on DebuggerModel.js rawLocationToUILocation can sometimes return null; UISourceCode.js returns null from uiLocationToRawLocation() if !this._sourceMap. So one solution is to cause Script.rawLocationToUILocation to return null if this._sourceMapping is undefined.

BTW it looks like http/tests/inspector/compiler-source-mapping-debug.html will fail if we complete 2/2 for exactly the reason you cite. So in addition to the issue you raise, we need a way to test breakpoints with source mapping under async for 2/2.

[johnjbarton]

Created attachment 164962 [details]
rebase and respond to review

[johnjbarton]

(In reply to comment #5)
> 
> (I believe this already occurs, at least I recall seeing exceptions related to rawLocationToUILocation).

Ok here is one such exception:

WebInspector.JavaScriptSource.workingCopyCommitted() :
   var rawLocation = this.uiLocationToRawLocation(0, 0);
will be null if
  1) the source file had a syntax error,
  2) the user edits the file to fix the syntax error,
  3) the user saves.

This is a consequent of UISourceCode.uiLocationToRawLocation:
        if (!this._sourceMapping)
            return null;

(reported as https://code.google.com/p/chromium/issues/detail?id=151399)

So there are other circumstances where we don't have a sourceMapping: we need to handle these cases gracefully.

[Vsevolod Vlasov]

Few thoughts:
1) rawLocationToUILocation should never return null. This method is manly used to stop on the breakpoint: we don't want to end up in the situation when we are stopped but the user can't see the execution line.

2) I'm fine (to some extent) if uiLocationToRawLocation returns null at some point. This is actually happening for snippets and that will happen even more often when we support editing of files other than those loaded from the network (a script with syntax error is a similar situation).

3) rawLocationToUILocation should always fallback to "temporary" uiSourceCodes.
This could be implemented based on the https://bugs.webkit.org/show_bug.cgi?id=97680. The patch in this bug is not yet landed so I suggest that we wait another week before moving on with your patch.

[Vsevolod Vlasov]

Comment on attachment 164962 [details]
rebase and respond to review

Clearing r? for now.

[Vsevolod Vlasov]

See also https://bugs.webkit.org/show_bug.cgi?id=97065

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.