Check BrowsingInstance before swapping prerenders.

Check BrowsingInstance before swapping prerenders.

Even if a WebContents does not have an opener, if there is another WebContents
in the BrowsingInstance, swapping processes can break pages which expect script
access.

BUG=118294
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270581

[davidben, 2014-04-28 18:34:39]

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:536: if
(web_contents->GetSiteInstance()->GetRelatedActiveContentsCount() != 1u) {
Would we ever have a WebContents where a frame is in a different
BrowsingInstance than the top-level?

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1058:
DecrementRelatedActiveContentsCount();
Not super-thrilled about this bit. Another possibility is some kind of
WebContentsImpl::SetUsingExternalFrameTreeForTesting(). Another possibility is
to move the accounting into top-level RFHMs.

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1133: 
Ditto.

[Charlie Reis, 2014-04-29 00:48:06]

(Just a first thought; I'll review the rest tomorrow.)

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:536: if
(web_contents->GetSiteInstance()->GetRelatedActiveContentsCount() != 1u) {
On 2014/04/28 18:34:39, David Benjamin wrote:
> Would we ever have a WebContents where a frame is in a different
> BrowsingInstance than the top-level?

Let me chat with Nasko about this.  I'd like to say no, because (barring
<webview> tags) subframes can always have a reference to the other window
objects in their frame tree.  That said, we usually put extensions in a
different browsing instance than web pages, and extensions can have web iframes.
 One of those will have to give.

[davidben, 2014-04-29 17:01:24]

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:536: if
(web_contents->GetSiteInstance()->GetRelatedActiveContentsCount() != 1u) {
On 2014/04/29 00:48:06, Charlie Reis wrote:
> On 2014/04/28 18:34:39, David Benjamin wrote:
> > Would we ever have a WebContents where a frame is in a different
> > BrowsingInstance than the top-level?
> 
> Let me chat with Nasko about this.  I'd like to say no, because (barring
> <webview> tags) subframes can always have a reference to the other window
> objects in their frame tree.  That said, we usually put extensions in a
> different browsing instance than web pages, and extensions can have web
iframes.
>  One of those will have to give.

Well, if this does end up happening, probably this should switch from a
WebContents count on the SiteInstance to an RFH count, probably managed by RFHM.
Then the condition for swap should be:

1. All RFH's for this WC have related SI's.
2. The SI's related RFH count should equal the number of RFH's in the WC.

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.h:61: size_t active_view_count() { return
active_view_count_; }
Oh, I mentioned this out-of-band, but the reason this value doesn't work is
because it also counts pending RVHs. The existing use of that count (when
swapped out RVHs can go away) should including pending RVHs because there may be
a race.

[Charlie Reis, 2014-04-29 17:31:08]

Still thinking about your comments in RFHM unittest.  I need to run to some
meetings, but here's some comments so far.

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/257083002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:536: if
(web_contents->GetSiteInstance()->GetRelatedActiveContentsCount() != 1u) {
On 2014/04/29 17:01:25, David Benjamin wrote:
> On 2014/04/29 00:48:06, Charlie Reis wrote:
> > On 2014/04/28 18:34:39, David Benjamin wrote:
> > > Would we ever have a WebContents where a frame is in a different
> > > BrowsingInstance than the top-level?
> > 
> > Let me chat with Nasko about this.  I'd like to say no, because (barring
> > <webview> tags) subframes can always have a reference to the other window
> > objects in their frame tree.  That said, we usually put extensions in a
> > different browsing instance than web pages, and extensions can have web
> iframes.
> >  One of those will have to give.
> 
> Well, if this does end up happening, probably this should switch from a
> WebContents count on the SiteInstance to an RFH count, probably managed by
RFHM.
> Then the condition for swap should be:
> 
> 1. All RFH's for this WC have related SI's.
> 2. The SI's related RFH count should equal the number of RFH's in the WC.

Let's assume that it won't happen, apart from <webview> tags where the subframe
thinks it's a top-level page and (at least in our current design) will have its
own WebContents.

The extension issue requires some investigation of whether postMessage is
allowed when an extension page loads a web page via window.open or an iframe, or
vice versa.  Perhaps we should just leave it in the same BrowsingInstance.

https://codereview.chromium.org/257083002/diff/1/content/browser/browsing_ins...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/browsing_ins...
content/browser/browsing_instance.h:96: // site.
Oops, wonder how that got deleted.  (Looks like it dates back to the initial
open-sourcing of Chrome.)  Nice catch.

https://codereview.chromium.org/257083002/diff/1/content/browser/browsing_ins...
content/browser/browsing_instance.h:112: // Number of WebContentses currently on
this SiteInstance.
on this SiteInstance -> in this BrowsingInstance

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.cc:211:
browsing_instance_->increment_active_contents_count();
These are one-liners that could be inlined, right?  Seems like we can define
them in the .h file and use unix_hacker_style names.

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.h:61: size_t active_view_count() { return
active_view_count_; }
On 2014/04/29 17:01:25, David Benjamin wrote:
> Oh, I mentioned this out-of-band, but the reason this value doesn't work is
> because it also counts pending RVHs. The existing use of that count (when
> swapped out RVHs can go away) should including pending RVHs because there may
be
> a race.

This count is also specific to the SiteInstance and not the BrowsingInstance,
right?  I agree that it wouldn't work.

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.h:63: void
IncrementRelatedActiveContentsCount();
These need comments explaining that they're for tracking the number of
WebContents in the BrowsingInstance, not counting pending hosts.

https://codereview.chromium.org/257083002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1080:
IncrementRelatedActiveContentsCount();
nit: Wrong indent.

[davidben, 2014-04-29 21:31:40]

https://codereview.chromium.org/257083002/diff/1/content/browser/browsing_ins...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/browsing_ins...
content/browser/browsing_instance.h:112: // Number of WebContentses currently on
this SiteInstance.
On 2014/04/29 17:31:08, Charlie Reis wrote:
> on this SiteInstance -> in this BrowsingInstance

Done.

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.cc:211:
browsing_instance_->increment_active_contents_count();
On 2014/04/29 17:31:08, Charlie Reis wrote:
> These are one-liners that could be inlined, right?  Seems like we can define
> them in the .h file and use unix_hacker_style names.

That would require BrowsingInstance to be included rather than just
forward-declared. *shrug* I can do that if it's still preferable.

...though it is odd that BrowsingInstance is currently forward-declared in
site_instance.h but not even exported from content/. Fixing that...

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.h:61: size_t active_view_count() { return
active_view_count_; }
On 2014/04/29 17:31:08, Charlie Reis wrote:
> On 2014/04/29 17:01:25, David Benjamin wrote:
> > Oh, I mentioned this out-of-band, but the reason this value doesn't work is
> > because it also counts pending RVHs. The existing use of that count (when
> > swapped out RVHs can go away) should including pending RVHs because there
may
> be
> > a race.
> 
> This count is also specific to the SiteInstance and not the BrowsingInstance,
> right?  I agree that it wouldn't work.

Yeah. Though if that were the only issue, BrowsingInstance could maintain a set
of all SiteInstances attached to it and sum over the active_view_counts.

https://codereview.chromium.org/257083002/diff/1/content/browser/site_instanc...
content/browser/site_instance_impl.h:63: void
IncrementRelatedActiveContentsCount();
On 2014/04/29 17:31:08, Charlie Reis wrote:
> These need comments explaining that they're for tracking the number of
> WebContents in the BrowsingInstance, not counting pending hosts.

Done.

https://codereview.chromium.org/257083002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:366: if (GetSiteInstance()) {
Dropped the conditional here. RVHI::GetSiteInstance() never returns NULL and it
doesn't look like RFHM::current_host() can actually be NULL? Comment says
"except in some tests". Either way, it would have crashed by then already.

https://codereview.chromium.org/257083002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1080:
IncrementRelatedActiveContentsCount();
On 2014/04/29 17:31:08, Charlie Reis wrote:
> nit: Wrong indent.

Done.

[Charlie Reis, 2014-04-29 22:13:51]

One note on the tests, plus a few comment nits.  Rest looks good.

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1058:
DecrementRelatedActiveContentsCount();
On 2014/04/28 18:34:39, David Benjamin wrote:
> Not super-thrilled about this bit. Another possibility is some kind of
> WebContentsImpl::SetUsingExternalFrameTreeForTesting(). Another possibility is
> to move the accounting into top-level RFHMs.

I'm not entirely sure why we need a separate FrameTree in this test in the first
place.  Looks like it was a legacy thing of creating a fresh RVHM for the test
using the existing WebContents as a mostly-unused delegate, but I don't see a
reason why we can't just use the RFHM of the WebContents itself.  See:
https://codereview.chromium.org/117693002/diff/20001/content/browser/frame_ho...

The test seems to pass if we replace tree, manager, and the Init call with:
RenderFrameHostManager* manager = web_contents->GetRenderManagerForTesting();

Same below.

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
File content/browser/browsing_instance.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
content/browser/browsing_instance.cc:19: active_contents_count_(0) {
Minor nit: You're using 0u everywhere else, so perhaps we should here as well.

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
content/browser/browsing_instance.h:82: size_t active_contents_count() const {
return active_contents_count_; }
// Tracks the number of WebContents currently in this BrowsingInstance.

https://codereview.chromium.org/257083002/diff/40001/content/browser/site_ins...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/site_ins...
content/browser/site_instance_impl.h:64: // Increase the number of active
WebContentses on this SiteInstance. Note
nit: on -> using (here, below, and in browsing_instance.h)

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1077: // Update SiteInstance
reference counts.
It feels strange to do this in WebContents rather than RFHM, so it would be nice
to elaborate in the comment on why this is the correct place.  Something like:

Update the count of WebContents objects using this SiteInstance.  All subframes
are in the same BrowsingInstance as the main frame, so we only do this for the
top-level frame.

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:2622: // Navigate to
a URL with WebUI. This will change SiteInstances.
will change BrowsingInstances.

[davidben, 2014-04-29 23:02:25]

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/257083002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1058:
DecrementRelatedActiveContentsCount();
On 2014/04/29 22:13:51, Charlie Reis wrote:
> On 2014/04/28 18:34:39, David Benjamin wrote:
> > Not super-thrilled about this bit. Another possibility is some kind of
> > WebContentsImpl::SetUsingExternalFrameTreeForTesting(). Another possibility
is
> > to move the accounting into top-level RFHMs.
> 
> I'm not entirely sure why we need a separate FrameTree in this test in the
first
> place.  Looks like it was a legacy thing of creating a fresh RVHM for the test
> using the existing WebContents as a mostly-unused delegate, but I don't see a
> reason why we can't just use the RFHM of the WebContents itself.  See:
>
https://codereview.chromium.org/117693002/diff/20001/content/browser/frame_ho...
> 
> The test seems to pass if we replace tree, manager, and the Init call with:
> RenderFrameHostManager* manager = web_contents->GetRenderManagerForTesting();
> 
> Same below.

Oh. Well then. Handy. I've gone and fixed those.

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
File content/browser/browsing_instance.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
content/browser/browsing_instance.cc:19: active_contents_count_(0) {
On 2014/04/29 22:13:51, Charlie Reis wrote:
> Minor nit: You're using 0u everywhere else, so perhaps we should here as well.

Done.

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/browsing...
content/browser/browsing_instance.h:82: size_t active_contents_count() const {
return active_contents_count_; }
On 2014/04/29 22:13:51, Charlie Reis wrote:
> // Tracks the number of WebContents currently in this BrowsingInstance.

Done.

https://codereview.chromium.org/257083002/diff/40001/content/browser/site_ins...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/site_ins...
content/browser/site_instance_impl.h:64: // Increase the number of active
WebContentses on this SiteInstance. Note
On 2014/04/29 22:13:51, Charlie Reis wrote:
> nit: on -> using (here, below, and in browsing_instance.h)

Done.

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1077: // Update SiteInstance
reference counts.
On 2014/04/29 22:13:51, Charlie Reis wrote:
> It feels strange to do this in WebContents rather than RFHM, so it would be
nice
> to elaborate in the comment on why this is the correct place.  Something like:

Moved it to RFHM. It's much much cleaner this way.

> Update the count of WebContents objects using this SiteInstance.  All
subframes
> are in the same BrowsingInstance as the main frame, so we only do this for the
> top-level frame.

Done.

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/257083002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:2622: // Navigate to
a URL with WebUI. This will change SiteInstances.
On 2014/04/29 22:13:51, Charlie Reis wrote:
> will change BrowsingInstances.

Done.

[Charlie Reis, 2014-04-29 23:16:40]

Agreed, that feels much nicer to move it into RFHM.

LGTM.  Please have John look at content/public/browser/site_instance.h for the
new API we're exposing.

https://codereview.chromium.org/257083002/diff/60001/content/browser/browsing...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/60001/content/browser/browsing...
content/browser/browsing_instance.h:113: // Number of WebContentses currently on
this BrowsingInstance.
nit: on -> using

[davidben, 2014-04-30 00:02:17]

Also +asvitkine for histograms.xml to get that done in parallel. (I'll wait for
the other reviewers as well.)

https://codereview.chromium.org/257083002/diff/60001/content/browser/browsing...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/60001/content/browser/browsing...
content/browser/browsing_instance.h:113: // Number of WebContentses currently on
this BrowsingInstance.
On 2014/04/29 23:16:40, Charlie Reis wrote:
> nit: on -> using

Done.

[Alexei Svitkine (slow), 2014-04-30 13:59:08]

histograms lgtm

[jam, 2014-04-30 16:13:23]

content/public lgtm with nit

https://codereview.chromium.org/257083002/diff/100001/content/public/browser/...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/257083002/diff/100001/content/public/browser/...
content/public/browser/site_instance.h:111: virtual size_t
GetRelatedActiveContentsCount() const = 0;
nit: remove the const, see
http://www.chromium.org/developers/content-module/content-api

[davidben, 2014-04-30 16:21:52]

https://codereview.chromium.org/257083002/diff/100001/content/public/browser/...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/257083002/diff/100001/content/public/browser/...
content/public/browser/site_instance.h:111: virtual size_t
GetRelatedActiveContentsCount() const = 0;
On 2014/04/30 16:13:23, jam wrote:
> nit: remove the const, see
> http://www.chromium.org/developers/content-module/content-api

Done.

[tburkard, 2014-05-07 08:52:22]

lgtm

But I think it'd be great to add some DCHECK/DCHECK's to verify that we keep the
count accurately and that it won't drop below 0.

Thanks.

https://codereview.chromium.org/257083002/diff/140001/content/browser/browsin...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/140001/content/browser/browsin...
content/browser/browsing_instance.h:85: void decrement_active_contents_count() {
active_contents_count_--; }
How about adding a DCHECK that active_contents_count_ >= 0?

[davidben, 2014-05-07 18:51:43]

https://codereview.chromium.org/257083002/diff/140001/content/browser/browsin...
File content/browser/browsing_instance.h (right):

https://codereview.chromium.org/257083002/diff/140001/content/browser/browsin...
content/browser/browsing_instance.h:85: void decrement_active_contents_count() {
active_contents_count_--; }
On 2014/05/07 08:52:23, tburkard wrote:
> How about adding a DCHECK that active_contents_count_ >= 0?

There's actually one in the destructor already, but asserting that it never goes
below 0 is also reasonable. Done.

[davidben, 2014-05-07 21:34:01]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-07 21:39:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/257083002/180001

[davidben, 2014-05-07 23:25:57]

The CQ bit was unchecked by davidben@chromium.org

[davidben, 2014-05-07 23:26:20]

On 2014/05/07 21:39:15, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/davidben@chromium.org/257083002/180001

Looks like that needs a rebase. Also, on second thought, let's hold on this
until after branch point anyway.

[davidben, 2014-05-15 03:03:42]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-15 03:04:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/257083002/200001

[commit-bot: I haz the power, 2014-05-15 04:15:16]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-15 05:50:22]

Change committed as 270581