Bug 200364 - Web Inspector: "Inspector.initialized" happens before breakpoints are set
Summary: Web Inspector: "Inspector.initialized" happens before breakpoints are set
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:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-01 15:14 PDT by Joseph Pecoraro
Modified: 2019-08-02 13:26 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.82 KB, patch)
2019-08-01 17:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (7.11 KB, patch)
2019-08-02 13:01 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 Joseph Pecoraro 2019-08-01 15:14:50 PDT
"Inspector.initialized" happens before breakpoints are set

This probably means `auto-attach` will not have breakpoints set up appropriately and would miss breakpoints.

Notes:
• Inspector page loading protocol log:

    ...
    [Log] Trace: request (page-338) – {id: 62, method: "Inspector.initialized"}
    ...
    [Log] Trace: request (page-338) – {id: 72, method: "Debugger.setBreakpointByUrl", params: Object}
    [Log] Trace: request (page-338) – {id: 74, method: "Debugger.setBreakpointByUrl", params: Object}
    ...
Comment 1 Devin Rousso 2019-08-01 17:32:42 PDT
Created attachment 375370 [details]
Patch
Comment 2 Joseph Pecoraro 2019-08-02 12:36:19 PDT
Comment on attachment 375370 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:89
> +        Promise.all(Target._initializationPromises).then(() => {

Would we want to assert here that `Target._initializationPromises` is not empty? We always expect at least one (Breakpoints)

This assert would let us know if we change something and went too early.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:145
> +        promise.then(() => {
> +            Target._initializationPromises.remove(promise);
> +        });

Oh, you're removing promises from the list... probably to reduce memory.

Maybe we should leave them for the assert, which seems pretty valuable. Or have an assertion:

    console.assert((Target._initializationPromises.length > 0) || (Target._completedInitializationPromises > 0));

And have this increment `Target._completedInitializationPromises` as well as remove the promise.

That keeps removing the promises at the cost of a counter for a useful assertion.
Comment 3 Devin Rousso 2019-08-02 13:01:53 PDT
Created attachment 375443 [details]
Patch
Comment 4 WebKit Commit Bot 2019-08-02 13:25:07 PDT
Comment on attachment 375443 [details]
Patch

Clearing flags on attachment: 375443

Committed r248176: <https://trac.webkit.org/changeset/248176>
Comment 5 WebKit Commit Bot 2019-08-02 13:25:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-08-02 13:26:13 PDT
<rdar://problem/53876805>