Bug 156021 – Web Inspector: sourceMappingURL not used when sourceURL is set

• Component: Web Inspector
• Status: RESOLVED FIXED

Created attachment 275193 [details]
Example to reproduce the bug.

If both `sourceURL` and `sourceMappingURL` are set in a script, the mapping is not used.

To reproduce:
- Unzip the attached project.
- Host it by executing 'python -m SimpleHTTPServer 1234'
- Go to http://0.0.0.0:1234
- Open the Web Inspector Console (option-cmd-C)
- Look at the first error

Observed:
- The error is reported as originating from app.js:3

Expected:
- The error is reported as originating from app.coffee:1

Notes:
While adding `sourceURL` in this case is not necessary (the script was not generated), it should also not break the source mapping.  Removing the `sourceURL` from `app.js` will lead to the expected behavior.

[Radar WebKit Bug Importer]

<rdar://problem/25438417>

[Joseph Pecoraro]

Wow, thanks for the great reduction!

This was actually originally fixed in WebKit but got lost when we changed frontends right before this change landed, see bug 107939. In this case, we have a non-absolute sourceURL ("app.js" instead of something like "http://example.com/js/app.js") so we don't actually resolve the correct URL to the source map resource.

[Joseph Pecoraro]

Hmm, so fixing this first issue gets us to download the source map, but exposes other problems.

The way WebKit's Web Inspector deals with resources is it prefers a Resource over a Script whenever possible. It relates Scripts to Resources based on their URL. However, the "sourceURL" in this case overrides the actual resource URL to this resource, and so this Script never gets associated with the Resource.

So the Resources Sidebar ends up with:

    Scripts
        app.js (this is the Resource)

    Extra Scripts
        app.js (this is the Script)
            app.coffee

Had they been correlated correctly we would expect to see just the Resource:

    Scripts
        app.js (this is the Resource, Script associated)
            app.coffee

Then when the error comes in, we try to find SourceCodeLocations for that, and prefer a Resource over a Script. Currently the Resource doesn't have the SourceMap associated with it, so we still show the Resource source code location "app.js:3".

---

On a related note, we have always had this FIXME in Script.js that relating the Script to the Resource by just "URL" is not good enough. To really relate the Script to the correct Resource you have to know at least know the Frame it came from as well to select the correct one.

    // FIXME: We should be able to associate a Script with a Resource through identifiers,
    // we shouldn't need to lookup by URL, which is not safe with frames, where there might
    // be multiple resources with the same URL.
    // <rdar://problem/13373951> Scripts should be able to associate directly with a Resource

So while we can fix the very basic problem that the SourceMap doesn't get downloaded (part of this and bug 156022), addressing the other more systemic issue will take more thought and move us in the right direction. Note this will probably also be needed for bug 150010 as well.

I think now is the time to either:

  1. Figure out a proper way to relate a Script to its Resource
  2. Move the frontend off of requiring this hard relationship to work properly

I'm more for (1) because I think having that relationship is important (the Type Profiler for example requires it).

[Joseph Pecoraro]

Created attachment 276371 [details]
[PATCH] Work In Progress - Better BaseURL for resolving Source Mapping URL

[Joseph Pecoraro]

By providing both the resource URL and sourceURL separately, I get this case working as expected. However, since I'm already changing Debugger.sourceParsed in a non-backwards compatible way, I want to see if I can go a step further and provide the frameIdentifier as well for the script.

Maybe I should do that in a follow-up patch though, since that will likely mean a bunch of tests orthogonal to this specific issue.

[Joseph Pecoraro]

Created attachment 276463 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 276463 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:429
> +    hasEventParameter(eventName, eventParameterName)

Nice!

[WebKit Commit Bot]

Comment on attachment 276463 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 276463

Committed r199602: <http://trac.webkit.org/changeset/199602>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.