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.
Comment 1 by sheriffbot@chromium.org
, Sep 10Status: Untriaged (was: Available)