New issue
Advanced search Search tips

Issue 817075 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ExtensionAPIFrameIDMap::CacheFrameData is not called for all frames.

Project Member Reported by karandeepb@chromium.org, Feb 27 2018

Issue description

This happens for frames which are live during ExtensionWebContentsObserver construction. 
 
Components: Platform>Extensions
Fixing this might also slightly improve Extensions.ExtensionFrameMapCacheHit, since the frame data for these would be cached early on.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc11e912fccca09e4c8a18e979bffac8993e7b41

commit dc11e912fccca09e4c8a18e979bffac8993e7b41
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Fri Mar 02 02:38:31 2018

Extensions: Ensure ExtensionAPIFrameIdMap::OnRenderFrameCreated is called.

Currently ExtensionAPIFrameIDMap::OnRenderFrameCreated is not called for frames
which are already alive during ExtensionWebContentsObserver construction. Ensure
that we call OnRenderFrameCreated for them as well. Fixing this might also
slightly improve Extensions.ExtensionFrameMapCacheHit, since the frame data for
these would be cached early on.

BUG= 817075 

Change-Id: I9f378f515741c6954bb6a5fc55adae1ff6b6f000
Reviewed-on: https://chromium-review.googlesource.com/939717
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540423}
[modify] https://crrev.com/dc11e912fccca09e4c8a18e979bffac8993e7b41/extensions/browser/extension_api_frame_id_map.cc
[modify] https://crrev.com/dc11e912fccca09e4c8a18e979bffac8993e7b41/extensions/browser/extension_web_contents_observer.cc

Cc: clamy@chromium.org
Posted on the wrong bug earlier. 

So another issue is that the ExtensionWebContentsObserver does not get the RenderFrameCreated notification for the Devtools main frame. This happens even though we are attaching it to main_web_contents_ of DevToolsWindow in ExtensionWebContentsObserver. Why it happens turns out to be quite obscure:

1) In DevToolsWindow::Create, we create the |main_web_contents| and load a url in it. During this navigation, DevtoolsUIBindings is created for the web contents. 
2) Now during DevtoolsUIBindings construction, ChromeExtensionWebContentsObserver is created. This uses WebContents::ForEachFrame to intialize the currently live frames and calls ExtensionAPIFrameIDMap::OnRenderFrameCreated. WebContents enumerates all its frames using FrameTreeNode::current_frame_host(). 
3) After some time, its speculative RFH becomes its active RFH. But EWCO doesn't listen to RenderFrameHostChanged and the RenderFrameCreated notification for this RFH had already been sent (before EWCO construction)

So it seems for a WCO interested in knowing about all RenderFrameHosts related to a WebContents, all three are necessary:
1) Track all live hosts during WCO construction.
2) Track RenderFrameCreated/Destroyed.
3) Track the |new_host| in RenderFrameHostChanged.

This seems quite subtle. Also, cc'ing clamy@ who can take a look and decide if something like this needs to be clarified better as part of the WebContentsObserver interface and if this is WAI.

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/399eed41d62b91b9d28b3447d870cca523b2b24d

commit 399eed41d62b91b9d28b3447d870cca523b2b24d
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Mon Mar 05 23:30:44 2018

Extensions: Ensure Devtools frames are correctly tracked by ExtensionWebContentsObserver.

Currently ExtensionWebContentsObserver(EWCO) does not get RenderFrameCreated
notification for the Devtools main frame. This happens because the frame is
created before the EWCO construction. Also, at the time of
ExtensionWebContentsObserver construction, this frame happens to be the
speculative RenderFrameHost of the root FrameTreeNode and hence is not
enumerated via WebContents::ForEachFrame. To fix this, also track the new hosts
seen through the WebContentsObserver::RenderFrameHostChanged method. Add a
browser test for the same.

Also, rename ExtensionAPIFrameIdMap::OnRenderFrameCreated to
InitializeRenderFrameHost to clarify the new usage.

BUG= 817075 

Change-Id: I81213f0441162a483fda9191991763a3fee65905
Reviewed-on: https://chromium-review.googlesource.com/942684
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540978}
[modify] https://crrev.com/399eed41d62b91b9d28b3447d870cca523b2b24d/chrome/browser/extensions/web_contents_browsertest.cc
[modify] https://crrev.com/399eed41d62b91b9d28b3447d870cca523b2b24d/extensions/browser/extension_api_frame_id_map.cc
[modify] https://crrev.com/399eed41d62b91b9d28b3447d870cca523b2b24d/extensions/browser/extension_api_frame_id_map.h
[modify] https://crrev.com/399eed41d62b91b9d28b3447d870cca523b2b24d/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/399eed41d62b91b9d28b3447d870cca523b2b24d/extensions/browser/extension_web_contents_observer.h

Status: Fixed (was: Assigned)

Sign in to add a comment