Bug 191740

<rdar://problem/45470897>

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!

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

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

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

Created attachment 355124 [details] [PATCH] Proposed Fix

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?

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.

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?

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

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

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.

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.

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

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