Bug 192016 - Web Inspector: REGRESSION(?): all "Show *" develop menu items cause the page to crash
Summary: Web Inspector: REGRESSION(?): all "Show *" develop menu items cause the page ...
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-27 11:03 PST by Devin Rousso
Modified: 2018-11-28 20:41 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (9.36 KB, patch)
2018-11-28 15:39 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-11-27 11:03:50 PST
This affects:
 - Show JavaScript Console
 - Show Page Source
 - Show Page Resources
 - Start Timeline Recording
 - Start Element Selection

The "Show Web Inspector" item (and it's associated keyboard shortcut) work correctly.
Comment 1 Radar WebKit Bug Importer 2018-11-27 12:20:27 PST
<rdar://problem/46284421>
Comment 2 Radar WebKit Bug Importer 2018-11-27 12:20:34 PST
<rdar://problem/46284417>
Comment 3 Joseph Pecoraro 2018-11-27 12:20:42 PST
Interesting. Should be easy to track these down and pipe them appropriately.
Comment 4 Joseph Pecoraro 2018-11-28 15:39:54 PST
Created attachment 355936 [details]
[PATCH] Proposed Fix
Comment 5 Devin Rousso 2018-11-28 17:11:44 PST
Comment on attachment 355936 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:160
> +    this._targetsAvailablePromise = new WI.WrappedPromise;

This does seem like the "perfect" use case for a Promise, but it scares me a bit cause it means delaying the code it runs just _that_ much more.  Considering this is at startup, I'd like to avoid any delays (no matter how small) whenever/wherever we can.  Perhaps we can do something like this:

--- WI.loaded ---
        this._loadOperations = new Set;
--- WI.loaded ---

--- WI.initializeBackendTarget ---
        this._completeLoadOperation("initializeBackendTarget");
--- WI.initializeBackendTarget ---

--- WI.contentLoaded ---
        this._completeLoadOperation("contentLoaded");
--- WI.contentLoaded ---

    WI._completeLoadOperation = function(name) {
        this._loadOperations.add(name);
        if (this._loadOperations.has("initializeBackendTarget") && this._loadOperations.has("contentLoaded"))
            InspectorFrontendAPI.loadCompleted();
    };

> Source/WebInspectorUI/UserInterface/Base/Main.js:559
> +    })

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Base/Main.js:588
> +WI.whenTargetsAvailable = function()

Since `WI._targetsAvailablePromise` is only used within `Main.js`, can we remove this function and just inline:

    WI._targetsAvailablePromise.promise.then(() => {
        InspectorFrontendAPI.loadCompleted();
    });

As an aside, maybe we should add a `then()` to `WI.WrappedPromise` so we can avoid this 🤔 (e.g. forwarding)

> Source/WebInspectorUI/UserInterface/Base/Main.js:591
> +}

Style: missing `;`

> Source/WebKit/ChangeLog:12
> +        once the UIProcess creates it. So queue actions that to take place

Typo: "actions that take place"

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:195
> +    whenFrontendConnectionEstablished([=] {

Capture by value?  Not by reference?

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:207
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:219
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:238
> +        m_frontendConnection->send(Messages::WebInspectorUI::ShowMainResourceForFrame(inspectorFrameIdentifier), 0);

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:247
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:257
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:267
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:277
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)
Comment 6 Joseph Pecoraro 2018-11-28 20:31:54 PST
Comment on attachment 355936 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:160
>> +    this._targetsAvailablePromise = new WI.WrappedPromise;
> 
> This does seem like the "perfect" use case for a Promise, but it scares me a bit cause it means delaying the code it runs just _that_ much more.  Considering this is at startup, I'd like to avoid any delays (no matter how small) whenever/wherever we can.  Perhaps we can do something like this:
> 
> --- WI.loaded ---
>         this._loadOperations = new Set;
> --- WI.loaded ---
> 
> --- WI.initializeBackendTarget ---
>         this._completeLoadOperation("initializeBackendTarget");
> --- WI.initializeBackendTarget ---
> 
> --- WI.contentLoaded ---
>         this._completeLoadOperation("contentLoaded");
> --- WI.contentLoaded ---
> 
>     WI._completeLoadOperation = function(name) {
>         this._loadOperations.add(name);
>         if (this._loadOperations.has("initializeBackendTarget") && this._loadOperations.has("contentLoaded"))
>             InspectorFrontendAPI.loadCompleted();
>     };

I agree with the sentiment but I think the delay is the smartest thing to do here so I'm going to stick with it for now. In fact I went with a Promise so that it would happen in a microtask as soon as possible and not a timeout delay.

In legacy cases, this should happen approximately a few statements later than it used to. In the multi-target case it'll happen as soon as the backend target (and page target) are initialized.

Using immediate callbacks like you've done would call InspectorFrontendAPI.loadCompleted before the page target is initialized, and so in the particular case (Cmd+Shift+C not working before page target loaded) it wouldn't have been enough.

I do think there is room for improvement in startup here. I ran into a bunch of complexities separating target initialization from the UI initialization, so I know we can be doing better. But I think once we've worked out all the correctness issues (like realizing the mistaken order of IFH.loadCompleted for example) we can address performance of a working system.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:588
>> +WI.whenTargetsAvailable = function()
> 
> Since `WI._targetsAvailablePromise` is only used within `Main.js`, can we remove this function and just inline:
> 
>     WI._targetsAvailablePromise.promise.then(() => {
>         InspectorFrontendAPI.loadCompleted();
>     });
> 
> As an aside, maybe we should add a `then()` to `WI.WrappedPromise` so we can avoid this 🤔 (e.g. forwarding)

I expect whenTargetsAvailable to have potential use elsewhere given I've already hit this pattern once and could think of another place to use it. I'll leave it for now.

>> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:195
>> +    whenFrontendConnectionEstablished([=] {
> 
> Capture by value?  Not by reference?

Nothing is being captured in almost all of these (other than `this`). The only one that has anything of value is a string, which we treat as a value anyways. If I'm overlooking something let me know.
Comment 7 Joseph Pecoraro 2018-11-28 20:41:38 PST
https://trac.webkit.org/changeset/238660/webkit