8000 fix: cross-origin navigation disposing WebFrameMain instances by samuelmaddock · Pull Request #30076 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: cross-origin navigation disposing WebFrameMain instances #30076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Aug 18, 2021
Merged

fix: cross-origin navigation disposing WebFrameMain instances #30076

merged 5 commits into from
Aug 18, 2021

Conversation

samuelmaddock
Copy link
Member
@samuelmaddock samuelmaddock commented Jul 11, 2021 •
edited
Loading

Description of Change

Recommended reading to better understand RenderFrameHosts and swapping of them during cross-origin navigation:
https://www.chromium.org/developers/design-documents/oop-iframes

This PR is to address one of the issues brought up in #29290 (comment).

WebFrameMain (WFM) currently abstracts content::RenderFrameHost and aligns closely with its lifespan. Under the OOPIF architecture and Site Isolation, cross-origin navigation leads to a RFH getting swapped for a new RFH in a separate process. WFM doesn't handle this case which will lead to cached references being marked as disposed if held through cross-origin navigation.

To address this, I've modified lookup of a WFM instance to use a FrameTreeNode (FTN) ID. This is a globally unique ID associated with a FTN which indirectly owns a RFH.

Chromium keeps track of the full frame tree for each tab in the browser process. WebContents hosts a tree of FrameTreeNode objects, mirroring the frame tree of the current page. Each FrameTreeNode contains frame-specific information (e.g., the frame's name, origin, etc). Its RenderFrameHostManager is responsible for cross-process navigations in the frame, and it supports replicating state and routing messages from proxies in other processes to the active frame.

content::WebContentsObserver::RenderFrameHostChanged(old_host, new_host) is invoked when a RFH is swapped during cross-origin navigation. By observing this event, we can update the internal RFH of a WFM instance if one exists.

With WFM instances persisting through navigation, we can safely emit events for frame navigation in future work.

cc @nornagon

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added

Release Notes

Notes: none

< 8000 svg aria-hidden="true" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true" class="octicon octicon-tag color-fg-inherit">
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 11, 2021
@samuelmaddock samuelmaddock added the semver/minor backwards-compatible functionality label Jul 11, 2021
@samuelmaddock samuelmaddock force-pushed the fix/webframemain-cross-origin branch 4 times, most recently from 69c11b5 to 59d6132 Compare July 11, 2021 19:55
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jul 22, 2021
zcbenz
zcbenz approved these changes Aug 5, 2021
View reviewed changes
nornagon
nornagon reviewed Aug 10, 2021
View reviewed changes
nornagon
nornagon reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on samuelmaddock#1 as an approach though?

@samuelmaddock
Copy link
Member Author
samuelmaddock commented Aug 11, 2021 •
edited
Loading

Thoughts on samuelmaddock#1 as an approach though?

I added some thoughts just now to that PR.

nornagon
nornagon approved these changes Aug 11, 2021
View reviewed changes
@nornagon
Copy link
Contributor

@samuelmaddock looks like lint is stuck, unfortunately the only way to fix this is to push to the branch. eg. git ci --allow-empty -m 'bump lint'

@samuelmaddock samuelmaddock force-pushed the fix/webframemain-cross-origin branch from 71cf384 to 6346a6a Compare August 12, 2021 19:58
@samuelmaddock samuelmaddock requested review from a team as code owners August 12, 2021 19:58
@samuelmaddock samuelmaddock force-pushed the fix/webframemain-cross-origin branch from 6346a6a to 245d022 Compare August 12, 2021 20:03
@samuelmaddock
Copy link
Member Author

Rebased to latest main

@trop trop bot removed the target/12-x-y label Aug 18, 2021
@trop
Copy link
Contributor
trop bot commented Aug 18, 2021

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

@trop
Copy link
Contributor
trop bot commented Aug 18, 2021

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

miniak pushed a commit that referenced this pull request Aug 18, 2021
@samuelmaddock @miniak
fix: cross-origin navigation disposing WebFrameMain instances (#30076)
230d0da
@zcbenz
Copy link
Contributor
zcbenz commented Aug 19, 2021

@samuelmaddock This change breaks the protocol module protocol.registerSchemesAsPrivileged allowServiceWorkers should fail when registering invalid service worker test on macOS.

@samuelmaddock
Copy link
Member Author

@samuelmaddock This change breaks the protocol module protocol.registerSchemesAsPrivileged allowServiceWorkers should fail when registering invalid service worker test on macOS.

We can close the backport PRs and revert this one. Then I'll follow up with a fixed PR.

@samuelmaddock
Copy link
Member Author

The issue seems to occur when executeJavaScript() is called leading to WebFrameMain::OnRendererConnectionError() being invoked. It only comes up after a sufficient amount of tests from api-protocol-spec.ts are run.

electron/spec-main/api-protocol-spec.ts

Lines 764 to 767 in 11de995

it('should fail when registering invalid service worker', async () => {
await contents.loadURL(`${serviceWorkerScheme}://${v4()}.com`);
await expect(contents.executeJavaScript(`navigator.serviceWorker.register('${v4()}.notjs', {scope: './'})`)).to.be.rejected();
});

electron/shell/browser/api/electron_api_web_frame_main.cc

Lines 183 to 198 in 11de995

const mojo::Remote<mojom::ElectronRenderer>& WebFrameMain::GetRendererApi() {
if (!renderer_api_) {
pending_receiver_ = renderer_api_.BindNewPipeAndPassReceiver();
if (render_frame_->IsRenderFrameCreated()) {
render_frame_->GetRemoteInterfaces()->GetInterface(
std::move(pending_receiver_));
}
renderer_api_.set_disconnect_handler(base::BindOnce(
&WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr()));
}
return renderer_api_;
}
void WebFrameMain::OnRendererConnectionError() {
renderer_api_.reset();
}

@samuelmaddock
Copy link
Member Author

Fix is to reset the mojo pipe when the RFH changes:

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

https://github.com/electron/electron/blob/11de995d381c3dc58cc6736f4fe0f7cd4ff42ae9/shell/browser/api/electron_api_web_frame_main.cc

@samuelmaddock samuelmaddock mentioned this pull request Aug 19, 2021
Merged
2 tasks
@samuelmaddock samuelmaddock mentioned this pull request Aug 23, 2021
Merged
5 tasks
BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@samuelmaddock @BlackHole1
fix: cross-origin navigation disposing WebFrameMain instances (electr…
de0c56d 
…on#30076)
@samuelmaddock samuelmaddock mentioned this pull request Aug 30, 2021
Closed
4 tasks
@trop trop bot removed the in-flight/12-x-y label Sep 23, 2021
@codebytere codebytere mentioned this pull request Sep 28, 2021
Merged
3 tasks
codebytere added a commit that referenced this pull request Mar 7, 2024
@codebytere
fix: potential crash getting WebFrameMain from RenderFrameHost
490a95e 
Refs https://issues.chromium.org/issues/40169570
Refs #30076
@codebytere codebytere mentioned this pull request Mar 7, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
api-review/requested 🗳 semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samuelmaddock @nornagon @zcbenz @jkleinsc @codebytere
0