New issue
Advanced search Search tips

Issue 763548 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 304341



Sign in to add a comment

Deprecate RenderViewCreated and RenderViewHost::GetMainFrame() usage

Project Member Reported by alex...@chromium.org, Sep 8 2017

Issue description

WebContentsObserver::RenderViewCreated is fired when a new RenderViewHost has been created for a particular WebContents.  With cross-process navigations and OOPIFs, this is a confusing API to use and can easily lead to bugs.  For example, a RenderViewHost created for an out-of-process iframe is "inactive" and does not fire RenderViewCreated.  Pending cross-process navigations in main frames used to fire it inconsistently: navigations into a new process would fire it, but a main frame navigation in an existing process, such as when A window.open's B and then A navigates to B, would not fire it. This should be fixed in https://chromium-review.googlesource.com/c/chromium/src/+/636786, which consistently fires it for RVHs used for pending main frame navigations, but the better solution is to migrate the code that relies on RenderViewCreated to RenderFrameCreated, which will force the code to reason about OOPIF behavior correctly.  Similarly, NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED is subject to the same pitfalls.  

A similarly problematic API is RenderViewHost::GetMainFrame(), which might return null for RVHs used by OOPIFs.  https://chromium-review.googlesource.com/c/chromium/src/+/636786 temporarily extended GetMainFrame to also return pending main frame RFHs if available, but this should be cleaned up by moving affected code to deal with RenderFrameHosts (and WebContents::GetMainFrame() if needed), rather than RenderViewHosts.

See issues  760341  and  762824 , as well discussion on https://chromium-review.googlesource.com/c/chromium/src/+/636786, for examples of problems these APIs might cause.  I'm filing this as a followup to that CL and solving issue 756790.

Some data and proposals:

- There are 20 current uses of RenderViewHost::GetMainFrame() outside of content/, and only one is a non-test use (in MemoryDetails::CollectChildInfoOnUIThread, where it can easily be replaced with looking up the main frame from WebContents).  We could easily change RenderViewHost::GetMainFrame to RenderViewHost::GetMainFrameForTesting() and then deal with refactoring tests as a second step.

- Inside content/, the non-test uses of RVHI::GetMainFrame() are:
 - RenderViewHostImpl::SetWebUIProperty mentioned above
 - three in interstitials
 - SpeechRecognitionDispatcherHost::OnStartRequest
 - WebContentsImpl::OnMessageReceived to let webUI handle messages if needed.
These need to be audited to see if they might have issues with OOPIFs.

- WebUI code in particular is bad at assuming it can do GetMainFrame() on any RenderViewHost.  Although WebUI doesn't currently use OOPIFs, it should be better prepared to deal with pending RFHs from cross-process navigations.  Paths like WebUITestHandler::PreloadJavaScript and RenderViewHostImpl::SetWebUIProperty should be refactored to use RenderFrameCreated and plumb pending RFHs rather than RVHs where needed.  (There's already a longer-term goal of migrating WebUI completely off of RVH to RFH.)

- There are 27 mostly non-test uses of WCO::RenderViewCreated and three uses of NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED.  These need to be audited for problems with OOPIFs or pending cross-process navigations.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Sep 10

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold

Sign in to add a comment