Bug 198276 - Web Inspector: REGRESSION(r239175): Source Maps: original source not shown when in nested folder
Summary: Web Inspector: REGRESSION(r239175): Source Maps: original source not shown wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL: https://github.com/gibatronic/webpack...
Keywords: InRadar
Depends on: 192388
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-27 08:36 PDT by Gibran Malheiros
Modified: 2020-05-11 11:31 PDT (History)
6 users (show)

See Also:


Attachments
Screen capture demonstration (244.90 KB, image/gif)
2019-05-27 08:36 PDT, Gibran Malheiros
no flags Details
Patch (7.22 KB, patch)
2020-05-01 16:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.12 KB, patch)
2020-05-04 12:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2020-05-04 16:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gibran Malheiros 2019-05-27 08:36:47 PDT
Created attachment 370693 [details]
Screen capture demonstration

Overview:
Both debugger and resources tabs don't show the original source file if its path contains more than one folder.

Steps to Reproduce:
1. Go to https://github.com/
2. Open web inspector
3. Go to the debugger tab
4. Expand the source folder tree to locate "apps/assets/modules/github"

Actual Results:
The tree item for "apps/assets/modules/github" has no children, empty.

Expected Results:
The tree item for "apps/assets/modules/github" should contain its children.

Build Date & Hardware:
Safari 12.1.1
macOS 10.14.5

Additional Information:
Does not happen in other browsers, I've checked with Chrome 74.0.3729.169 and Firefox 67.0
A bare minimum project is available to reproduce the issue at https://github.com/gibatronic/webpack-Safari-nested-folder-source-map-issue
Comment 1 Gibran Malheiros 2019-05-29 04:58:38 PDT
I was also able to reproduce this issue in Safari Technology Preview:
> Release 82 (Safari 12.2, WebKit 14608.1.23.1)
Comment 2 Christopher Grande 2020-02-20 16:35:51 PST
Seeing this still in Release 101 (Safari 13.2, WebKit 15610.1.3)
Comment 3 Devin Rousso 2020-05-01 16:05:25 PDT
Created attachment 398256 [details]
Patch
Comment 4 Devin Rousso 2020-05-04 11:50:49 PDT
Comment on attachment 398256 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:19
> +        Drive-by: don't attempt to combine folder chains, matching the behavior of "Group by Path".

After thinking about this a bit more, I think a better approach would be to combine folder chains when "Group by Type" (preserves the existing behavior), and to not do that when "Group by Path" as that makes the whole tree match look/feel.
Comment 5 Devin Rousso 2020-05-04 12:00:13 PDT
Created attachment 398396 [details]
Patch
Comment 6 Joseph Pecoraro 2020-05-04 14:04:44 PDT
Comment on attachment 398396 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:374
> +        // Copy the array so that it's not modified (e.g. if a caller saved it to iterate later).
> +        this.children = this.children.slice();

This seems kind of expensive and it sounds like the caller is doing something that might be wrong by assuming the array is not modified. Should the caller be making the duplicate?
Comment 7 Devin Rousso 2020-05-04 15:29:25 PDT
Comment on attachment 398396 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:374
>> +        this.children = this.children.slice();
> 
> This seems kind of expensive and it sounds like the caller is doing something that might be wrong by assuming the array is not modified. Should the caller be making the duplicate?

It's true that this is expensive, but this mimics the old behavior.  I tried looking around, but given how common `removeChildren` is, I'm not sure if there are other things that are relying on the old functionality before r239175.  It might just be this case.

I did this to be safe, but given that we haven't seen any other cases of issues like this, I'm inclined to remove it.  I'll investigate a bit more to see if we can remove this or at the very least go back to something like the old functionality.
Comment 8 Devin Rousso 2020-05-04 16:33:01 PDT
Created attachment 398433 [details]
Patch
Comment 9 Joseph Pecoraro 2020-05-05 14:48:04 PDT
Comment on attachment 398433 [details]
Patch

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

rs=me

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:392
> +        this.children = [];

I like this much better. I suppose it is possible that in response to an element being removed someone could add a child.. but `removeChildren` is a remove of all children, so you should be left with nothing at the end.
Comment 10 Devin Rousso 2020-05-05 14:56:03 PDT
Comment on attachment 398433 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:392
>> +        this.children = [];
> 
> I like this much better. I suppose it is possible that in response to an element being removed someone could add a child.. but `removeChildren` is a remove of all children, so you should be left with nothing at the end.

Yes, that's true.  FWIW, that issue would also affect `WI.DataGrid`, so i kinda doubt it :P
Comment 11 EWS 2020-05-05 15:09:39 PDT
Committed r261200: <https://trac.webkit.org/changeset/261200>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398433 [details].
Comment 12 Radar WebKit Bug Importer 2020-05-05 15:10:14 PDT
<rdar://problem/62905365>
Comment 13 Gibran Malheiros 2020-05-08 00:43:29 PDT
Thanks to everyone involved. This will be much appreciated.
Is it possible to get notified when this patch lands in a nightly build?
Comment 14 BJ Burg 2020-05-11 11:31:50 PDT
(In reply to Gibran Malheiros from comment #13)
> Thanks to everyone involved. This will be much appreciated.
> Is it possible to get notified when this patch lands in a nightly build?

We don't have nightly builds. It should make it into Safari Technology Preview 107.