Bug 150010 - Web Inspector: React.js JSXTransformer produces bogus error locations
Summary: Web Inspector: React.js JSXTransformer produces bogus error locations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Safari 9
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 150114
  Show dependency treegraph
 
Reported: 2015-10-10 21:53 PDT by Nikita Vasilyev
Modified: 2016-04-25 17:45 PDT (History)
7 users (show)

See Also:


Attachments
[Animated GIF] Bug (907.90 KB, image/gif)
2015-10-10 21:53 PDT, Nikita Vasilyev
no flags Details
[PATCH] Proposed Fix (17.43 KB, patch)
2016-04-25 16:26 PDT, Joseph Pecoraro
timothy: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[IMAGE] Sidebar with dynamic scripts (233.96 KB, image/png)
2016-04-25 16:26 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Sidebar with dynamic scripts folder (176.22 KB, image/png)
2016-04-25 16:26 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-10-10 21:53:29 PDT
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).
Comment 1 Radar WebKit Bug Importer 2015-10-10 21:53:52 PDT
<rdar://problem/23062233>
Comment 2 Joseph Pecoraro 2015-10-12 12:07:53 PDT
Why do we show react-contenteditable.js at all? Nothing in the error's stack trace points to that script. This is very weird.
Comment 3 Joseph Pecoraro 2015-10-12 12:19:14 PDT
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.
Comment 4 Joseph Pecoraro 2016-04-25 16:26:09 PDT
Created attachment 277288 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2016-04-25 16:26:38 PDT
Created attachment 277289 [details]
[IMAGE] Sidebar with dynamic scripts
Comment 6 Joseph Pecoraro 2016-04-25 16:26:53 PDT
Created attachment 277290 [details]
[IMAGE] Sidebar with dynamic scripts folder
Comment 7 Joseph Pecoraro 2016-04-25 16:27:36 PDT
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.
Comment 8 Joseph Pecoraro 2016-04-25 16:45:40 PDT
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.
Comment 9 BJ Burg 2016-04-25 16:53:20 PDT
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?
Comment 10 Joseph Pecoraro 2016-04-25 17:02:56 PDT
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.
Comment 11 Timothy Hatcher 2016-04-25 17:17:59 PDT
Comment on attachment 277288 [details]
[PATCH] Proposed Fix

Looks good. Rename to Extra Scripts and extraScripts and it is good to go.
Comment 12 Joseph Pecoraro 2016-04-25 17:32:52 PDT
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.
Comment 13 Joseph Pecoraro 2016-04-25 17:45:51 PDT
<http://trac.webkit.org/changeset/200065>