Bug 150010 – Web Inspector: React.js JSXTransformer produces bogus error locations

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 262850 [details]
[Animated GIF] Bug

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/React-JSXTransformer/
2. Open Console
3. Click on the error location

Expected:
App.js (line 4) opens in Debugger.

Actual:
react-contenteditable.js (line 4), completely different file, opens in Debugger.

Notes:
This works as expected in Chrome DevTools but doesn't work in Firefox 43 (points to index.html and not App.js).

[Radar WebKit Bug Importer]

<rdar://problem/23062233>

[Joseph Pecoraro]

Why do we show react-contenteditable.js at all? Nothing in the error's stack trace points to that script. This is very weird.

[Joseph Pecoraro]

Hmm. So apparently <script>s with //# sourceMappingURLs are added to the head of the page. The script that really executes is this <script>. And we probably do not correctly select the <script> / scriptId associated with this error. It does look like the source map data, per injected <script> is correct.

[Joseph Pecoraro]

Created attachment 277288 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 277289 [details]
[IMAGE] Sidebar with dynamic scripts

[Joseph Pecoraro]

Created attachment 277290 [details]
[IMAGE] Sidebar with dynamic scripts folder

[Joseph Pecoraro]

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

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

> LayoutTests/ChangeLog:10
> +        * inspector/model/frame-dynamic-scripts-expected.txt: Added.
> +        * inspector/model/frame-dynamic-scripts.html: Added.

Alternatives to "Dynamic Scripts" would be "Script Elements", "Inline Scripts", "Dynamic Inline Scripts", etc.

[Joseph Pecoraro]

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

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

>> LayoutTests/ChangeLog:10
>> +        * inspector/model/frame-dynamic-scripts.html: Added.
> 
> Alternatives to "Dynamic Scripts" would be "Script Elements", "Inline Scripts", "Dynamic Inline Scripts", etc.

Brian and Tim both suggested "Extra Scripts". I'll move to that. And Frame.extraScripts, which ultimately could include the `evals` and Anonymous Scripts executed within the frame.

[Blaze Burg]

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

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

Per IRC discussion let's rename at least the user-visible strings to "Extra Scripts". This will eventually be per-Frame and include InjectedScripts and any other 'eval'uations with a sourceURL.

> LayoutTests/inspector/model/frame-dynamic-scripts.html:36
> +                let mainFrame = WebInspector.frameResourceManager.mainFrame;

With just one frame in play here, this test isn't too good for detecting per-frame behavior. Is this handled by script-resource-relationship.html test? I had expected to see a new test case there.

> LayoutTests/inspector/model/frame-dynamic-scripts.html:38
> +                resolve();

If possible it would be nice to test that the dynamic scripts are cleared when the frame navigates. This might have to happen in an iframe to work around flaky test infrastructure caused by reloading the test page itself.

> Source/WebInspectorUI/UserInterface/Models/Script.js:48
> +        // with the frame as a dynamic script.

Is this so that hyperlinks to the fragment don't try to look it up in the document at 0/0?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Models/Script.js:48
>> +        // with the frame as a dynamic script.
> 
> Is this so that hyperlinks to the fragment don't try to look it up in the document at 0/0?

This is so that we treat the script as separate from the main resource. A real inline <script>...</script> is treated as associated with the main resource because that is where its contents are. A dynamically added script doesn't have anything of value in the main resource's content, so it should be treated separately.

[Timothy Hatcher]

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

Looks good. Rename to Extra Scripts and extraScripts and it is good to go.

[Joseph Pecoraro]

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

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

>> LayoutTests/inspector/model/frame-dynamic-scripts.html:36
>> +                let mainFrame = WebInspector.frameResourceManager.mainFrame;
> 
> With just one frame in play here, this test isn't too good for detecting per-frame behavior. Is this handled by script-resource-relationship.html test? I had expected to see a new test case there.

I added a new script-resource-relationship test for a dynamicallyAddedScriptElement Script.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/200065>