Bug 191740 – Web Inspector: Keep Web Inspector window alive across process swaps (PSON) (Local Inspector)

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

[Joseph Pecoraro]

Keep Web Inspector window alive across process swaps (PSON) (Local Inspector)

[Joseph Pecoraro]

<rdar://problem/45470897>

[Joseph Pecoraro]

Created attachment 355025 [details]
[PATCH] For Bots

Not good to review just yet but things are looking good. I want to see what the bots see on other ports and if there are any test crashes.

  - Local testing behaves as expected across process kills / process swaps
  - LayoutTests are using the new code path in WebKit (WI.MultiplexingBackendTarget)
  - LayoutTests are using the old code path in WebKitLegacy (WI.DirectBackendTarget)
  - Hide the TargetAgent.exists message...

I still need to do a few obvious things:

  - InspectorBackend.runAfterPendingDispatches
    - I can probably hack this to use the pageTarget's connection instead of the backendTarget connection (a.k.a. mainTarget)
  - WebInspectorUI::openInNewTab
    - Just give this a new path.
  - inspector/worker tests
    - likely have to wait a bit before Worker targets show up

But this is looking pretty close!

[EWS Watchlist]

Comment on attachment 355025 [details]
[PATCH] For Bots

Attachment 355025 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10013953

New failing tests:
inspector/canvas/setShaderProgramHighlighted.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/pause-reason.html
inspector/worker/debugger-pause.html
inspector/indexeddb/clearObjectStore.html
inspector/debugger/breakpoint-columns.html
inspector/canvas/requestContent-bitmaprenderer.html
inspector/canvas/setShaderProgramDisabled.html
inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
inspector/debugger/breakpoint-scope.html
inspector/unit-tests/target-manager.html
inspector/indexeddb/requestData.html

[EWS Watchlist]

Created attachment 355029 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

>   - InspectorBackend.runAfterPendingDispatches
>     - I can probably hack this to use the pageTarget's connection instead of

Turns out this 1 change fixed pretty much everything else.

I'm seeing weird test failures if 1 test fails / times out it takes down a ton of other tests running in parallel ("Failed to reset state to consistent values"). I'm not exactly sure what that means yet. But I'm going to put a patch up and see what the bots think.

[Joseph Pecoraro]

Created attachment 355124 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 355124 [details]
[PATCH] Proposed Fix

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

> Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp:95
> -    return m_page->inspector();
> +    return nullptr;

Why does this always return nullptr now? Change to void?

[Joseph Pecoraro]

Comment on attachment 355124 [details]
[PATCH] Proposed Fix

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

>> Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp:95
>> +    return nullptr;
> 
> Why does this always return nullptr now? Change to void?

This case returns nullptr, but WebKitLegacy will return a local inspector in-process. WebKitLegacy is still using the legacy path.

[Devin Rousso]

Comment on attachment 355124 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:100
> +            if (messageObject["error"].code !== -32000 && messageObject["error"].message !== "'Target' domain was not found")

Should you also check that the code isn't `-32601` for the expected Target error?

> Source/WebKit/ChangeLog:9
> +        When a web page asks to open a local Web Inspector that inspector

Typo: "Inspector, that inspector"

> Source/WebKit/ChangeLog:19
> +        is reset when the page is reset and updated when the page's WebProcess

Does this mean that it's theoretically possible to get "stuck" in an indeterminate state, where we have yet to connect to something new (e.g. if it doesn't get created for some reason)?

> Source/WebKit/UIProcess/WebPageProxy.cpp:943
> +    m_inspector->invalidate();

Should this also `m_inspector = nullptr;` like it did before in the function below?

[Joseph Pecoraro]

Comment on attachment 355124 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:100
>> +            if (messageObject["error"].code !== -32000 && messageObject["error"].message !== "'Target' domain was not found")
> 
> Should you also check that the code isn't `-32601` for the expected Target error?

Naw the message is specific enough.

>> Source/WebKit/ChangeLog:19
>> +        is reset when the page is reset and updated when the page's WebProcess
> 
> Does this mean that it's theoretically possible to get "stuck" in an indeterminate state, where we have yet to connect to something new (e.g. if it doesn't get created for some reason)?

Yes, in the same way that it is possible for a WebPageProxy to be in an indeterminate state between being connected to a WebPage or not. Hopefully by following WebPageProxy's methods we avoid these issues.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:943
>> +    m_inspector->invalidate();
> 
> Should this also `m_inspector = nullptr;` like it did before in the function below?

Sure

[Joseph Pecoraro]

<https://trac.webkit.org/r238330>

[Chris Dumez]

Comment on attachment 355124 [details]
[PATCH] Proposed Fix

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

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:943
>>> +    m_inspector->invalidate();
>> 
>> Should this also `m_inspector = nullptr;` like it did before in the function below?
> 
> Sure

This causes crashes in API tests. You set m_inspector to null here... then resetState() is called a little below.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6253
> +    m_inspector->reset();

And you dropped the null check here.. so null-deref.

[Joseph Pecoraro]

x(In reply to Chris Dumez from comment #12)
> Comment on attachment 355124 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355124&action=review
> 
> >>> Source/WebKit/UIProcess/WebPageProxy.cpp:943
> >>> +    m_inspector->invalidate();
> >> 
> >> Should this also `m_inspector = nullptr;` like it did before in the function below?
> > 
> > Sure
> 
> This causes crashes in API tests. You set m_inspector to null here... then
> resetState() is called a little below.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:6253
> > +    m_inspector->reset();
> 
> And you dropped the null check here.. so null-deref.

Oops, I'll remove the `= nullptr` to go back to what I've tested with.

[Joseph Pecoraro]

https://trac.webkit.org/r238338 - Follow-up

[Truitt Savell]

It looks like https://trac.webkit.org/changeset/238330/webkit

has caused inspector/css/modify-rule-selector.html to become flakey.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fmodify-rule-selector.html

I was able to reproduce this with:
run-webkit-tests --root debug-238330 inspector/css/modify-rule-selector.html --iterations 500 -f --debug

I get multiple failures on r238330 and none on r238329.

Diff:
--- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/inspector/css/modify-rule-selector-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/inspector/css/modify-rule-selector-actual.txt
@@ -1,6 +1,10 @@
 Testing that selectors are able to be modified more than once.
 
- 
+ ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+
 Selector before mutation: .foo
 Selector after mutation: span.foo