fix: crash when creating interface for speculative frame

[samuelmaddock]

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

• PR description included and stakeholders cc'd
• npm test passes
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed potential crash with WebFrameMain when navigating between cross-origin websites.

[miniak]

@samuelmaddock Electron 16 is not affected?

[samuelmaddock]

@samuelmaddock Electron 16 is not affected?

Looks like it should be backportable to 15 and 16.

[jkleinsc]

@samuelmaddock can you rebase this on the latest from main? I think that should resolve the windows failure.

[nornagon]

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?

[samuelmaddock]

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

	void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) {
	// Should only be called when swapping frames.
	render_frame_disposed_ = false;
	render_frame_ = rfh;
	renderer_api_.reset();
	}

[samuelmaddock]

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

[samuelmaddock]

Mac build failed due to maximum build time (120m), likely due to fork. Windows build has timeouts on document.visibilityState tests; seems unrelated.

[nornagon]

I'm not 100% sure this is the right fix but the tests pass and it theoretically fixes a crash (which we can't repro?)

LGTM to unblock but I'm a bit iffy.

[nornagon]

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-clerk]

Release Notes Persisted

Fixed potential crash with WebFrameMain when navigating between cross-origin websites.

[trop]

I have automatically backported this PR to "18-x-y", please check out #34293

[trop]

I have automatically backported this PR to "19-x-y", please check out #34294