fix: reset render_frame_disposed_ after render frame host change

[VerteDinde]

Description of Change

This PR fixes a JavaScript crash that occurs if a webContents is trying to send or reload when a render frame has been marked as disposed. Previously, if this occurred, the send would fail silently and eventually recover and reload. Now, we throw an error in CheckRenderFrame, which appears to an app user as a hard crash.

This PR resets the value of render_frame_disposed_ after the render frame host has been updated, and prevents throwing a hard error on webFrameMain.send or webFrameMain.reload, while still checking for the existence of the render_frame_.

Checklist

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

Release Notes

Notes: Fixed a JavaScript exception from webContents if render frame was disposed in WebFrameMain, resets the value of render_frame_disposed_ after updating render frame host

[samuelmaddock]

Was this change added to silence the errors? I'm unsure if this is the best direction we should take.

I'm wondering if we should have something similar to WebContents.isDestroyed(). Otherwise we could add try/catch where this fails. Did we have internal code (in lib/*.ts) which was failing?

[VerteDinde]

It was, yeah. From what I can tell, this is the call that's failing on the *.ts side: https://github.com/electron/electron/blob/main/lib/browser/api/web-contents.ts#L125 Since it connects directly to mainFrame, maybe we could add a try/catch on the send directly?

[VerteDinde]

Added a try/catch to send within web-frame-main.ts, not webContents, to bring it a little closer to the source - I'm a little unsure about my original approach and the new try/catch, since it feels more like a band-aid than actually addressing the crash, but this should at least stop a hard JavaScript error from being thrown on send failure.

[samuelmaddock]

Although it's difficult to know for sure without being able to reproduce the crash, my theory is that WebFrameMain::MarkRenderFrameDisposed() was called prior to WebFrameMain::UpdateRenderFrameHost(). In that case, the new RFH would be set, but render_frame_disposed_ == true would still hold.

This could happen when the RFH is deleted

electron/shell/browser/api/electron_api_web_contents.cc

Lines 1467 to 1469 in b2a2b07

	auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
	if (web_frame && web_frame->render_frame_host() == render_frame_host)
	web_frame->MarkRenderFrameDisposed();

followed by it being swapped for a new RFH

electron/shell/browser/api/electron_api_web_contents.cc

Lines 1480 to 1484 in b2a2b07

	auto* web_frame =
	WebFrameMain::FromFrameTreeNodeId(new_host->GetFrameTreeNodeId());
	if (web_frame) {
	web_frame->UpdateRenderFrameHost(new_host);
	}

It may be possible that the changes in electron_api_web_frame_main.cc alone fix the problem, in which case the try/catch might be unnecessary.

[VerteDinde]

@samuelmaddock I dug more into this yesterday; the MarkRenderFrameDisposed prior to the new RFH being updated and set is definitely what's keeping the renderer process in a continuous loop 🙂 I think fixing that should fix the main issue! I did test the crash with only that section fixed, just to verify, and it's still throwing one hard JS exception when you try to reload, which then keeps the renderer process for recovering:

I'll keep the try/catch to avoid throwing a hard exception; this should allow apps with stalled renderer processes to reload and recover.

[release-clerk]

Release Notes Persisted

Fixed a JavaScript exception from webContents if render frame was disposed in WebFrameMain, resets the value of render_frame_disposed_ after updating render frame host

[trop]

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

[trop]

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

[trop]

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

[miniak]

/trop run backport-to 13-x-y

[trop]

The backport process for this PR has been manually initiated - sending your PR to 13-x-y!

[trop]

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

[poiru]

I don't think this is an ideal fix. I don't think we want to swallow all exceptions like for example:

electron/shell/browser/api/electron_api_web_frame_main.cc

Lines 171 to 175 in cf0959f

	if (!gin::ConvertFromV8(isolate, args, &message)) {
	isolate->ThrowException(v8::Exception::Error(
	gin::StringToV8(isolate, "Failed to serialize arguments")));
	return;
	}

That should continue to throw. We only want to avoid the exception from:

electron/shell/browser/api/electron_api_web_frame_main.cc

Lines 108 to 118 in bff20bd

	bool WebFrameMain::CheckRenderFrame() const {
	if (render_frame_disposed_) {
	v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
	v8::Locker locker(isolate);
	v8::HandleScope scope(isolate);
	gin_helper::ErrorThrower(isolate).ThrowError(
	"Render frame was disposed before WebFrameMain could be accessed");
	return false;
	}
	return true;
	}

IMO we should instead avoid throwing in the first place in WebFrameMain::Send, WebFrameMain::PostMessage, and perhaps WebContents::MessageTo. We could do that by replacing the CheckRenderFrame() call with something that just checks the status instead of also throwing.

cc @VerteDinde

[VerteDinde]

Thanks @poiru! Just saw your comments here, will open a new PR with those improvements. We're in the middle of a December quiet period, so we may not have a new release with the fix until January, but will work on getting that fix ready.

[poiru]

Perfect, thank you @VerteDinde!

[poiru]

@VerteDinde Friendly reminder about this!

[poiru]

The this._postMessage call below can also throw in the same way as the calls above. We should fix it too.