New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 789864 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
creis@chromium.org
alex...@chromium.org
█ nasko@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

RVH::GetMainFrame != FrameTree::GetMainFrame

Project Member Reported by jochen@chromium.org, Nov 30 2017 
In https://chromium-review.googlesource.com/c/chromium/src/+/795716 I changed

RenderFrameHost::FromID(process_id, frame_id)->GetRenderViewHost()->GetMainFrame()

to

RenderFrameHostImpl::FromID(process_id, frame_id)->frame_tree_node()->frame_tree()->GetMainFrame()

which surprised me, because I had expected that both should give me the same frame, however, the former gives me a nullptr when running with --site-isolation

Comment 1 by nasko@chromium.org, Nov 30 2017

Cc: creis@chromium.org alex...@chromium.org nasko@chromium.org
This could happen with OOPIFs if the frame identified by frame_id is a subframe in a process different than the main frame. In the presence of OOPIFs, there is one RVH for each process that is rendering the full page and all RVHs except the one that is associated with the main frame will return nullptr when asked about their main frame, as they don't have one.

In general, RenderView(Host) should not be used anymore and our goal is to remove it from the codebase in the long run (or most likely morph it into a page-level object).

I don't know if there is anything actionable here other than possibly auditing the codebase for the old pattern and updating any instances we find.

Comment 2 by creis@chromium.org, Nov 30 2017 

We could look at removing RVH::GetMainFrame() (or at least making it internal-only) for that reason.  Not sure if that's easy, but worth trying when we get a chance.

Comment 3 by jochen@chromium.org, Nov 30 2017 

I mainly filed this because it was surprising to me (and dcheng who reviewed the code didn't catch it either..)

Comment 4 by nasko@chromium.org, Nov 30 2017 

There aren't that many calls to RVH::GetMainFrame, it might actually be feasible to remove it. Quick search reveals 17 references, some of them inside content/.
►

Sign in to add a comment