fix: crash when creating interface for speculative frame #33919
Conversation
|
@samuelmaddock Electron 16 is not affected? |
Looks like it should be backportable to 15 and 16. |
|
@samuelmaddock can you rebase this on the latest from main? I think that should resolve the windows failure. |
3431a69 to
d8c2b99
Compare
| @@ -316,7 +316,7 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const { | |||
| } | |||
|
|
|||
| void WebFrameMain::Connect() { | |||
| if (pending_receiver_) { | |||
| if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { | |||
There was a problem hiding this comment.
I have questions..! Will this reconnect later once the render frame is actually created? Will we miss any events or anything by not connecting immediately? Basically: is this the correct fix?
There was a problem hiding this comment.
When I initially implemented this change, I think I may have overlooked the case where a speculative frame becomes active and we're sending messages using the frame.send() API. I do wonder if it's possible that messages aren't received when using ipcRenderer.send early in a preload script. I'd like to try creating a test to see whether we can break it this way.
WebContentsObserver::RenderFrameHostStateChanged(new_frame, old_state, new_state) is called when the speculative frame, or any other frame, changes to active.
// When committing a cross-document, cross-RenderFrameHost navigation,
// navigation-related callbacks are dispatched in the following order:
// - RenderFrameHostStateChanged(new_frame, old_state, new_state)
// - RenderFrameHostChanged(old_frame, new_frame)
// - RenderFrameHostStateChanged(old_frame, old_state, new_state)
// - DidFinishNavigation(navigation_handle)
If we do end up missing reconnecting the frame, listening for new_state == contents::RenderFrameHost::LifecycleState::kActive should help resolve it.
I'll followup once I've been able to experiment writing a test against early preload messages.
Also, I wonder if we should be reconnecting as soon as we swap RFHs here:
electron/shell/browser/api/electron_api_web_frame_main.cc
Lines 101 to 106 in 9901d2f
There was a problem hiding this comment.
I attempted several different methods of creating a WebContents (window.open, new BrowserWindow, window dispositions) and was unable to ever reproduce the crash. The stack trace included with the reported issue simply called webContents.loadURL(url) with a cross-origin URL. There may possibly be a race condition somewhere in Chromium leading to this behavior...
As for "is this the correct fix?", you might be right about the call to GetInterface(pending_receiver_) never happening. To ensure that it eventually gets called, I made it so MaybeSetupMojoConnection() will always check this when we need to get the renderer API. 4a91a92
d8c2b99 to
4a91a92
Compare
This should eliminate an entire class of potential errors from appearing in the future.
bed2845 to
70e0689
Compare
|
Mac build failed due to maximum build time (120m), likely due to fork. Windows build has timeouts on |
|
Starting next week, 17.x will be our earliest supported version, and we only do security backports to our earliest supported version, so i've updated the targets for this PR. |
|
Release Notes Persisted
|
|
I have automatically backported this PR to "18-x-y", please check out #34293 |
|
I have automatically backported this PR to "19-x-y", please check out #34294 |
Description of Change
fixes #33880
Speculative render frames are created with pending cross-origin navigation requests. Their RenderFrameHost is not fully active until the navigation is later committed.
There seems to be a crash with our code to handle new RFHs. I've added a check to make sure the RFH has been notified of the RenderFrame being created prior to connecting the WebFrameMain's Mojo interface.
I wasn't able to reproduce the crash in a test.
Checklist
npm testpassesRelease Notes
Notes: Fixed potential crash with WebFrameMain when navigating between cross-origin websites.