REGRESSION(313207@main): Web Inspector: ProxyingNetworkAgent leaks IPC::MessageReceiver after cross-origin iframe process swap

[burg]

04f8f8e

REGRESSION(313207@main): Web Inspector: ProxyingNetworkAgent leaks IPC::MessageReceiver after cross-origin iframe process swap
https://bugs.webkit.org/show_bug.cgi?id=314808
rdar://177056433

Reviewed by Abrar Rahman Protyasha.

Two distinct teardown bugs surface from the same cross-origin iframe
process swap path under Site Isolation. Both manifest as flaky crashes
attributed to whatever non-SI test runs after a Web Inspector layout
test in WKTR's run order, which is why
http/tests/ssl/applepay/ApplePayButton.html was the visible victim.

The first issue is that WebPageInspectorController::didCommitProvisionalFrame
re-instruments the new WebContent process for network events but never
removes the old process's instrumentation. The (oldProcessID, oldPageID)
entry stays in ProxyingNetworkAgent::m_instrumentedProcessPageCounts and
its IPC::MessageReceiver registration on the old WebProcessProxy outlives
the agent. ~ProxyingNetworkAgent then asserts in ~IPC::MessageReceiver
during the next WKWebView teardown. Thread the iframe's old pageID from
WebFrameProxy::commitProvisionalFrame through to didCommitProvisionalFrame,
and disable instrumentation for the old process before re-enabling on the
new one.

The second issue is that FrameInspectorTarget::connect() chooses
provisionalFrame() ?: coreLocalFrame() and registers the frontend channel
with that frame's inspector controller. disconnect() makes the same
choice independently. Under Site Isolation those two choices can resolve
to different LocalFrames -- a provisional load can start after connect,
or the previously-provisional frame commits and replaces coreLocalFrame().
Disconnecting from a controller that never registered the channel asserts
in FrontendRouter::disconnectFrontend. Pin a WeakPtr<LocalFrame> at
connect time and disconnect from that same frame's controller.

Test changes:
- evaluate-in-cross-origin-iframe.html (was [ Failure Crash ]) now passes.
- execution-context-from-frame-target.html (was [ Failure ]) now passes.

* Source/WebKit/UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::commitProvisionalFrame):
* Source/WebKit/UIProcess/Inspector/WebPageInspectorController.h:
* Source/WebKit/UIProcess/Inspector/WebPageInspectorController.cpp:
(WebKit::WebPageInspectorController::didCommitProvisionalFrame):
* Source/WebKit/WebProcess/Inspector/FrameInspectorTarget.h:
* Source/WebKit/WebProcess/Inspector/FrameInspectorTarget.cpp:
(WebKit::FrameInspectorTarget::connect):
(WebKit::FrameInspectorTarget::disconnect):
* LayoutTests/platform/mac-wk2/TestExpectations:

Canonical link: https://commits.webkit.org/313318@main

ea5988f

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	🧪 win-tests	✅ 🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe		✅ 🛠 vision-apple
	✅ 🧪 ios-wk2-wpt	✅ 🧪 api-mac-debug	✅ 🛠 gtk3-libwebrtc
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🛠 ios-safer-cpp	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🛠 playstation
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash ea5988f)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	🧪 win-tests	✅ 🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe		✅ 🛠 vision-apple
	✅ 🧪 ios-wk2-wpt	✅ 🧪 api-mac-debug	✅ 🛠 gtk3-libwebrtc
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🛠 ios-safer-cpp	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🛠 playstation
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-commit-queue]

Committed 313318@main (04f8f8e): https://commits.webkit.org/313318@main

Reviewed commits have been landed. Closing PR #64973 and removing active labels.

[the-chenergy]

I question the validity of the second issue you brought up in the commit message, specifically whether these two things are actually what happened to cause the bug:

The second issue is that FrameInspectorTarget::connect() chooses
provisionalFrame() ?: coreLocalFrame() and registers the frontend channel
with that frame's inspector controller. disconnect() makes the same
choice independently. Under Site Isolation those two choices can resolve
to different LocalFrames — a provisional load can start after connect,
or the previously-provisional frame commits and replaces coreLocalFrame().
Disconnecting from a controller that never registered the channel asserts
in FrontendRouter::disconnectFrontend. Pin a WeakPtr<LocalFrame> at
connect time and disconnect from that same frame's controller.

Regarding "the previously-provisional frame commits and replaces coreLocalFrame()": This clearly does not change what ?: returns; the same LocalFrame simply migrates from m_provisionalFrame into m_coreFrame in WebFrame::commitProvisionalFrame

Regarding "a provisional load can start after connect":

• First, I doubt this can even happen in practice because ProvisionalFrameProxy constructor sends CreateProvisionalFrame to that web process with the WebFrame in question before WebPageInspectorController::didCreateProvisionalFrame runs and the target agent auto-connects with the ConnectInspector message. These two messages will arrive in this order, and WebFrame::createProvisionalFrame ensures that a m_provisionalFrame exists before FrameInspectorTarget::connect runs
• Second, if we're worried about a new provisional load happening after connect has been run, and the internal states of the WebFrame changes unexpectedly, I don't think using a pin (recording the WeakPtr<LocalFrame>) would be the right solution either. An unexpected state change in WebFrame would be a bug we should properly address in our provisional target system (e.g. by surfacing a new provisional target and destroying the old one in time) rather than bypass it with the pin

Can you share more about how the stacktrace or logs confirms the issue in FrameInspectorTarget? If neither of the two scenarios is what actually happened, I'd rather us landing the network-leak fix first and splitting any changes in provisional frame targets' handling to a later patch

[the-chenergy]

Since two lines above we're already unconditionally doing *webPageIDInCurrentProcess(), we shouldn't need the std::optional here (and therefore the parameter of didCommitProvisionalFrame) because we're asserting the id always exists at this point already