Bug 150009 - Web Inspector: Line error widget showed in the wrong resource
Summary: Web Inspector: Line error widget showed in the wrong resource
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
: 142553 (view as bug list)
Depends on:
Blocks: 150114
  Show dependency treegraph
 
Reported: 2015-10-10 21:24 PDT by Nikita Vasilyev
Modified: 2016-04-25 17:45 PDT (History)
9 users (show)

See Also:


Attachments
[Image] Bug (195.83 KB, image/png)
2015-10-10 21:24 PDT, Nikita Vasilyev
no flags Details
[PATCH] Proposed Fix (27.16 KB, patch)
2016-04-25 14:26 PDT, Joseph Pecoraro
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
[IMAGE] Before - Format String (94.94 KB, image/png)
2016-04-25 14:27 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After - Format String (99.10 KB, image/png)
2016-04-25 14:27 PDT, Joseph Pecoraro
no flags Details
Archive of layout-test-results from ews100 for mac-yosemite (947.56 KB, application/zip)
2016-04-25 14:55 PDT, Build Bot
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:24:27 PDT
Created attachment 262846 [details]
[Image] Bug

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/React-JSXTransformer/
2. Open Resources tab
3. Select the main HTML resource

Actual:
JS error "Can't find variable: nope" showed in the wrong (HTML) file.

Expected:
The error should only be showed in the JS file.
Comment 1 Radar WebKit Bug Importer 2015-10-10 21:24:43 PDT
<rdar://problem/23062199>
Comment 2 Joseph Pecoraro 2015-10-20 11:11:55 PDT
Is this the same as bug 150010?
Comment 3 Nikita Vasilyev 2015-10-20 15:19:32 PDT
(In reply to comment #2)
> Is this the same as bug 150010?

They are different from the user's perspective. I don't know if the underling issue is the same.
Comment 4 Nikita Vasilyev 2015-10-20 15:21:24 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Is this the same as bug 150010?
> 
> They are different from the user's perspective. I don't know if the
> underling issue is the same.

*underlying issue
Comment 5 Timothy Hatcher 2016-03-03 13:50:13 PST
This looks like a backend issue where we report an error on the main resource URL with a line number from the script.
Comment 6 Joseph Pecoraro 2016-04-22 21:10:41 PDT
*** Bug 142553 has been marked as a duplicate of this bug. ***
Comment 7 Joseph Pecoraro 2016-04-25 14:26:51 PDT
Created attachment 277277 [details]
[PATCH] Proposed Fix
Comment 8 Joseph Pecoraro 2016-04-25 14:27:08 PDT
Created attachment 277278 [details]
[IMAGE] Before - Format String
Comment 9 Joseph Pecoraro 2016-04-25 14:27:24 PDT
Created attachment 277279 [details]
[IMAGE] After - Format String
Comment 10 Joseph Pecoraro 2016-04-25 14:28:37 PDT
Including images showing the drive-by format fixes also addressed by this.
Comment 11 Build Bot 2016-04-25 14:55:40 PDT
Comment on attachment 277277 [details]
[PATCH] Proposed Fix

Attachment 277277 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1219701

New failing tests:
fast/layers/no-clipping-overflow-hidden-added-after-transform.html
Comment 12 Build Bot 2016-04-25 14:55:44 PDT
Created attachment 277281 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Joseph Pecoraro 2016-04-25 16:33:11 PDT
Comment on attachment 277277 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Controllers/IssueManager.js:45
> +        if (sourceCode instanceof WebInspector.Script)
> +            return issue.url === sourceCode.url || (issue.sourceCodeLocation && issue.sourceCodeLocation.sourceCode === sourceCode);

I think I should drop the url comparison here, or fallback to it if the sourceCodeLocation is unavailable. Otherwise, two dynamically added <script>s will both show this issue, even if the sourceCode specifically matches one of them.
Comment 14 Timothy Hatcher 2016-04-25 17:16:56 PDT
Comment on attachment 277277 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/IssueManager.js:45
>> +            return issue.url === sourceCode.url || (issue.sourceCodeLocation && issue.sourceCodeLocation.sourceCode === sourceCode);
> 
> I think I should drop the url comparison here, or fallback to it if the sourceCodeLocation is unavailable. Otherwise, two dynamically added <script>s will both show this issue, even if the sourceCode specifically matches one of them.

Maybe we should nullify SourceCode's url for dynamic scripts after we associate things? Though it likely makes sense to just drop this compare too.
Comment 15 Joseph Pecoraro 2016-04-25 17:45:36 PDT
<http://trac.webkit.org/changeset/200064>