Bug 192016 – Web Inspector: REGRESSION(?): all "Show *" develop menu items cause the page to crash

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Radar WebKit Bug Importer]

<rdar://problem/46284421>

[Radar WebKit Bug Importer]

<rdar://problem/46284417>

[Joseph Pecoraro]

Interesting. Should be easy to track these down and pipe them appropriately.

[Joseph Pecoraro]

Created attachment 355936 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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)

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/238660/webkit