Bug 198276 – Web Inspector: REGRESSION(r239175): Source Maps: original source not shown when in nested folder

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

[Gibran Malheiros]

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

[Gibran Malheiros]

I was also able to reproduce this issue in Safari Technology Preview:
> Release 82 (Safari 12.2, WebKit 14608.1.23.1)

[Christopher Grande]

Seeing this still in Release 101 (Safari 13.2, WebKit 15610.1.3)

[Devin Rousso]

Created attachment 398256 [details]
Patch

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 398396 [details]
Patch

[Joseph Pecoraro]

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?

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 398433 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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

[EWS]

Committed r261200: <https://trac.webkit.org/changeset/261200>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398433 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62905365>

[Gibran Malheiros]

Thanks to everyone involved. This will be much appreciated.
Is it possible to get notified when this patch lands in a nightly build?

[Blaze Burg]

(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.