OOPIF: Check whether localFrameRootTemporary is valid when processing resizing messages

OOPIF: Check whether localFrameRootTemporary is valid when processing resizing messages.

With --site-per-process, some content_browsertests were crashing in the renderer while processing a ViewMsg_Resize, because they accessed localFrameRootTemporary()->frameView() without first checking whether localFrameRootTemporary() is valid.  localFrameRootTemporary() won't be valid if site A opens a popup to site B: when we initialize the opener frame/view in B's process, the opener will just have one remote top-level frame.  For an example, see RenderFrameHostManagerTest.DontSwapProcessWithOnlyTargetBlank.  In this case, the resizing message is sent as part of initializing the swapped-out view for the opener, and should be ok to ignore.

BUG=417518
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183695

[alexmos, 2014-10-02 21:44:55]

alexmos@chromium.org changed reviewers:
+ dcheng@chromium.org

[alexmos, 2014-10-02 21:53:50]

Daniel, could you please take a look at this?

[dcheng, 2014-10-02 22:58:28]

Are we actually using RemoteFrames as top-level frames with --site-per-process?
That's a surprise to me--I thought we were still using the RFPH wrapping a RFH
hack.

[dcheng, 2014-10-09 20:31:09]

dcheng@chromium.org changed reviewers:
+ kenrb@chromium.org

[dcheng, 2014-10-09 20:31:09]

+kenrb

I just talked with Alex to understand more about this problem. The root of this
problem is essentially this:
Process A, frame A opens a popup (frame B). It is initially considered same
site. it does a cross-site navigation, and so we initialize process B. Process B
initialized a swapped-out RenderView for the opener frame A. As part of this, we
send a resize message. But remote frame A in process B has no local frame root
at all.

Ken: since I'm not as familiar with the layout/rendering aspects and our plans
for it... do we need the sizing information at all in this case? If remote frame
A later gets a local frame B, does it actually depend on any sizing information
from remote frame A? If not, then it seems like we should just throw away/ignore
the sizing message. If not, then we should have a FIXME to stash this
information somewhere. Given what I know, I think we can just ignore it.

[dcheng, 2014-10-09 20:32:42]

On 2014/10/09 at 20:31:09, dcheng wrote:
> +kenrb
> 
> I just talked with Alex to understand more about this problem. The root of
this problem is essentially this:
> Process A, frame A opens a popup (frame B). It is initially considered same
site. it does a cross-site navigation, and so we initialize process B. Process B
initialized a swapped-out RenderView for the opener frame A. As part of this, we
send a resize message. But remote frame A in process B has no local frame root
at all.
> 
> Ken: since I'm not as familiar with the layout/rendering aspects and our plans
for it... do we need the sizing information at all in this case? If remote frame
A later gets a local frame B, does it actually depend on any sizing information
from remote frame A? If not, then it seems like we should just throw away/ignore
the sizing message. If not, then we should have a FIXME to stash this
information somewhere. Given what I know, I think we can just ignore it.

Also, I'm not quite sure what should happen if we happen to swap remote frame A
to a local frame at some point. It seems like we'd need the sizing information
then. Is this something that would be resent during the swap?

[kenrb, 2014-10-10 19:19:49]

On 2014/10/09 20:32:42, dcheng wrote:
> On 2014/10/09 at 20:31:09, dcheng wrote:
> > +kenrb
> > 
> > I just talked with Alex to understand more about this problem. The root of
> this problem is essentially this:
> > Process A, frame A opens a popup (frame B). It is initially considered same
> site. it does a cross-site navigation, and so we initialize process B. Process
B
> initialized a swapped-out RenderView for the opener frame A. As part of this,
we
> send a resize message. But remote frame A in process B has no local frame root
> at all.
> > 
> > Ken: since I'm not as familiar with the layout/rendering aspects and our
plans
> for it... do we need the sizing information at all in this case? If remote
frame
> A later gets a local frame B, does it actually depend on any sizing
information
> from remote frame A? If not, then it seems like we should just throw
away/ignore
> the sizing message. If not, then we should have a FIXME to stash this
> information somewhere. Given what I know, I think we can just ignore it.
> 

We need to allow it now, but it doesn't make sense to send resize messages to a
swapped out RenderView. It's not rendered, so it has no size.

It sounds to me like we are just seeing the fact that we haven't implemented
out-of-process popups yet. They should work similarly to swapped out
RenderViews.

> Also, I'm not quite sure what should happen if we happen to swap remote frame
A
> to a local frame at some point. It seems like we'd need the sizing information
> then. Is this something that would be resent during the swap?

A is the top-level frame in your example. Do we have any notion of swapping the
top-level frame? Navigating it destroys its content.

[kenrb, 2014-10-10 19:20:21]

I would remove the comment but otherwise lgtm. You will need to find an owner
for approval though.

https://codereview.chromium.org/613043005/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/613043005/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1737: // sizing.
I don't think this is a valid concern. Size messages should be destined to the
process where a frame is actually being rendered. A remote frame's embedder
knows how big the frame is supposed to be, and it is sending that size to the
process where the frame is local.

Ultimately WebViewImpl::resize() should only be called on the top-level frame,
because semantically this is declaring the size of the content viewport in the
browser window. Part of getting rid of localFrameRootTemporary() is making
WebViewImpl not have to route messages to remote frames.

[dcheng, 2014-10-10 19:34:59]

On 2014/10/10 at 19:19:49, kenrb wrote:
> On 2014/10/09 20:32:42, dcheng wrote:
> > On 2014/10/09 at 20:31:09, dcheng wrote:
> > > +kenrb
> > > 
> > > I just talked with Alex to understand more about this problem. The root of
> > this problem is essentially this:
> > > Process A, frame A opens a popup (frame B). It is initially considered
same
> > site. it does a cross-site navigation, and so we initialize process B.
Process B
> > initialized a swapped-out RenderView for the opener frame A. As part of
this, we
> > send a resize message. But remote frame A in process B has no local frame
root
> > at all.
> > > 
> > > Ken: since I'm not as familiar with the layout/rendering aspects and our
plans
> > for it... do we need the sizing information at all in this case? If remote
frame
> > A later gets a local frame B, does it actually depend on any sizing
information
> > from remote frame A? If not, then it seems like we should just throw
away/ignore
> > the sizing message. If not, then we should have a FIXME to stash this
> > information somewhere. Given what I know, I think we can just ignore it.
> > 
> 
> We need to allow it now, but it doesn't make sense to send resize messages to
a swapped out RenderView. It's not rendered, so it has no size.
> 
> It sounds to me like we are just seeing the fact that we haven't implemented
out-of-process popups yet. They should work similarly to swapped out
RenderViews.

Are you saying that if we do a cross-site swap to B later, we'll resend the
resize information even though we're not recreating the RenderView?

> 
> > Also, I'm not quite sure what should happen if we happen to swap remote
frame A
> > to a local frame at some point. It seems like we'd need the sizing
information
> > then. Is this something that would be resent during the swap?
> 
> A is the top-level frame in your example. Do we have any notion of swapping
the top-level frame? Navigating it destroys its content.

Yes, we are going to swap top-level frames eventually. The potentially
problematic scenario is a cross-site swap from A to B--it's not clear to me how
the top-level B being swapped in will know its size.

[kenrb, 2014-10-10 20:44:13]

On 2014/10/10 19:34:59, dcheng wrote:
> > We need to allow it now, but it doesn't make sense to send resize messages
to
> a swapped out RenderView. It's not rendered, so it has no size.
> > 
> > It sounds to me like we are just seeing the fact that we haven't implemented
> out-of-process popups yet. They should work similarly to swapped out
> RenderViews.
> 
> Are you saying that if we do a cross-site swap to B later, we'll resend the
> resize information even though we're not recreating the RenderView?
> 

Why not recreate the RenderView? This is a top-level navigation that is causing
a process change.

> > 
> > > Also, I'm not quite sure what should happen if we happen to swap remote
> frame A
> > > to a local frame at some point. It seems like we'd need the sizing
> information
> > > then. Is this something that would be resent during the swap?
> > 
> > A is the top-level frame in your example. Do we have any notion of swapping
> the top-level frame? Navigating it destroys its content.
> 
> Yes, we are going to swap top-level frames eventually. The potentially
> problematic scenario is a cross-site swap from A to B--it's not clear to me
how
> the top-level B being swapped in will know its size.

I am not clear on what you have in mind for this kind of swap, so it's hard for
me to make suggestions about how it would be implemented.

[dcheng, 2014-10-10 20:50:55]

On 2014/10/10 20:44:13, kenrb wrote:
> On 2014/10/10 19:34:59, dcheng wrote:
> > > We need to allow it now, but it doesn't make sense to send resize messages
> to
> > a swapped out RenderView. It's not rendered, so it has no size.
> > > 
> > > It sounds to me like we are just seeing the fact that we haven't
implemented
> > out-of-process popups yet. They should work similarly to swapped out
> > RenderViews.
> > 
> > Are you saying that if we do a cross-site swap to B later, we'll resend the
> > resize information even though we're not recreating the RenderView?
> > 
> 
> Why not recreate the RenderView? This is a top-level navigation that is
causing
> a process change.

Err, I don't get the impression that we even do that today with swapping out
render views. Why would this be any different with OOPI?

> 
> > > 
> > > > Also, I'm not quite sure what should happen if we happen to swap remote
> > frame A
> > > > to a local frame at some point. It seems like we'd need the sizing
> > information
> > > > then. Is this something that would be resent during the swap?
> > > 
> > > A is the top-level frame in your example. Do we have any notion of
swapping
> > the top-level frame? Navigating it destroys its content.
> > 
> > Yes, we are going to swap top-level frames eventually. The potentially
> > problematic scenario is a cross-site swap from A to B--it's not clear to me
> how
> > the top-level B being swapped in will know its size.
> 
> I am not clear on what you have in mind for this kind of swap, so it's hard
for
> me to make suggestions about how it would be implemented.

[alexmos, 2014-10-10 21:42:36]

On 2014/10/10 20:50:55, dcheng wrote:
> On 2014/10/10 20:44:13, kenrb wrote:
> > On 2014/10/10 19:34:59, dcheng wrote:
> > > > We need to allow it now, but it doesn't make sense to send resize
messages
> > to
> > > a swapped out RenderView. It's not rendered, so it has no size.
> > > > 
> > > > It sounds to me like we are just seeing the fact that we haven't
> implemented
> > > out-of-process popups yet. They should work similarly to swapped out
> > > RenderViews.
> > > 
> > > Are you saying that if we do a cross-site swap to B later, we'll resend
the
> > > resize information even though we're not recreating the RenderView?
> > > 
> > 
> > Why not recreate the RenderView? This is a top-level navigation that is
> causing
> > a process change.
> 
> Err, I don't get the impression that we even do that today with swapping out
> render views. Why would this be any different with OOPI?
> 
> > 
> > > > 
> > > > > Also, I'm not quite sure what should happen if we happen to swap
remote
> > > frame A
> > > > > to a local frame at some point. It seems like we'd need the sizing
> > > information
> > > > > then. Is this something that would be resent during the swap?
> > > > 
> > > > A is the top-level frame in your example. Do we have any notion of
> swapping
> > > the top-level frame? Navigating it destroys its content.
> > > 
> > > Yes, we are going to swap top-level frames eventually. The potentially
> > > problematic scenario is a cross-site swap from A to B--it's not clear to
me
> > how
> > > the top-level B being swapped in will know its size.
> > 
> > I am not clear on what you have in mind for this kind of swap, so it's hard
> for
> > me to make suggestions about how it would be implemented.

I think the scenario Daniel and I are wondering about is actually the same one I
filed a bug for a few days ago: crbug.com/420777.  Suppose I resized the
original a.com window in step 3.5.  Today, would that send resize messages to
a.com's process only, or to b.com's process as well (since it contains the
swapped-out view for the same window)?  And related, how should the b.com
process know about that resize once step 4 makes the opener's RenderView local? 
Due to that bug I can't get far enough in the test to actually see what resizing
messages would be sent today.  Without OOPIF, that scenario keeps both
RenderViews in the same process (due to crbug.com/24447), so a view swap was not
ever needed.

[kenrb, 2014-10-14 17:29:26]

On 2014/10/10 21:42:36, alexmos wrote:
> On 2014/10/10 20:50:55, dcheng wrote:
> > On 2014/10/10 20:44:13, kenrb wrote:
> > > On 2014/10/10 19:34:59, dcheng wrote:
> > > > > We need to allow it now, but it doesn't make sense to send resize
> messages
> > > to
> > > > a swapped out RenderView. It's not rendered, so it has no size.
> > > > > 
> > > > > It sounds to me like we are just seeing the fact that we haven't
> > implemented
> > > > out-of-process popups yet. They should work similarly to swapped out
> > > > RenderViews.
> > > > 
> > > > Are you saying that if we do a cross-site swap to B later, we'll resend
> the
> > > > resize information even though we're not recreating the RenderView?
> > > > 
> > > 
> > > Why not recreate the RenderView? This is a top-level navigation that is
> > causing
> > > a process change.
> > 
> > Err, I don't get the impression that we even do that today with swapping out
> > render views. Why would this be any different with OOPI?
> > 
> > > 
> > > > > 
> > > > > > Also, I'm not quite sure what should happen if we happen to swap
> remote
> > > > frame A
> > > > > > to a local frame at some point. It seems like we'd need the sizing
> > > > information
> > > > > > then. Is this something that would be resent during the swap?
> > > > > 
> > > > > A is the top-level frame in your example. Do we have any notion of
> > swapping
> > > > the top-level frame? Navigating it destroys its content.
> > > > 
> > > > Yes, we are going to swap top-level frames eventually. The potentially
> > > > problematic scenario is a cross-site swap from A to B--it's not clear to
> me
> > > how
> > > > the top-level B being swapped in will know its size.
> > > 
> > > I am not clear on what you have in mind for this kind of swap, so it's
hard
> > for
> > > me to make suggestions about how it would be implemented.
> 
> I think the scenario Daniel and I are wondering about is actually the same one
I
> filed a bug for a few days ago: crbug.com/420777.  Suppose I resized the
> original a.com window in step 3.5.  Today, would that send resize messages to
> a.com's process only, or to b.com's process as well (since it contains the
> swapped-out view for the same window)?

I think it only goes to a.com's process. At least, this is true for OOPIFs and I
am guessing it is the same for out-of-process popups.

> And related, how should the b.com
> process know about that resize once step 4 makes the opener's RenderView
local? 

If we aren't recreating the RenderView, it probably makes sense to send any
initialization parameters to it at the point where it is swapped in. Size
information comes from the platform widget in the browser.

> Due to that bug I can't get far enough in the test to actually see what
resizing
> messages would be sent today.  Without OOPIF, that scenario keeps both
> RenderViews in the same process (due to crbug.com/24447), so a view swap was
not
> ever needed.

[alexmos, 2014-10-14 18:36:42]

On 2014/10/14 17:29:26, kenrb wrote:
> On 2014/10/10 21:42:36, alexmos wrote:
> > On 2014/10/10 20:50:55, dcheng wrote:
> > > On 2014/10/10 20:44:13, kenrb wrote:
> > > > On 2014/10/10 19:34:59, dcheng wrote:
> > > > > > We need to allow it now, but it doesn't make sense to send resize
> > messages
> > > > to
> > > > > a swapped out RenderView. It's not rendered, so it has no size.
> > > > > > 
> > > > > > It sounds to me like we are just seeing the fact that we haven't
> > > implemented
> > > > > out-of-process popups yet. They should work similarly to swapped out
> > > > > RenderViews.
> > > > > 
> > > > > Are you saying that if we do a cross-site swap to B later, we'll
resend
> > the
> > > > > resize information even though we're not recreating the RenderView?
> > > > > 
> > > > 
> > > > Why not recreate the RenderView? This is a top-level navigation that is
> > > causing
> > > > a process change.
> > > 
> > > Err, I don't get the impression that we even do that today with swapping
out
> > > render views. Why would this be any different with OOPI?
> > > 
> > > > 
> > > > > > 
> > > > > > > Also, I'm not quite sure what should happen if we happen to swap
> > remote
> > > > > frame A
> > > > > > > to a local frame at some point. It seems like we'd need the sizing
> > > > > information
> > > > > > > then. Is this something that would be resent during the swap?
> > > > > > 
> > > > > > A is the top-level frame in your example. Do we have any notion of
> > > swapping
> > > > > the top-level frame? Navigating it destroys its content.
> > > > > 
> > > > > Yes, we are going to swap top-level frames eventually. The potentially
> > > > > problematic scenario is a cross-site swap from A to B--it's not clear
to
> > me
> > > > how
> > > > > the top-level B being swapped in will know its size.
> > > > 
> > > > I am not clear on what you have in mind for this kind of swap, so it's
> hard
> > > for
> > > > me to make suggestions about how it would be implemented.
> > 
> > I think the scenario Daniel and I are wondering about is actually the same
one
> I
> > filed a bug for a few days ago: crbug.com/420777.  Suppose I resized the
> > original a.com window in step 3.5.  Today, would that send resize messages
to
> > a.com's process only, or to b.com's process as well (since it contains the
> > swapped-out view for the same window)?
> 
> I think it only goes to a.com's process. At least, this is true for OOPIFs and
I
> am guessing it is the same for out-of-process popups.
> 
> > And related, how should the b.com
> > process know about that resize once step 4 makes the opener's RenderView
> local? 
> 
> If we aren't recreating the RenderView, it probably makes sense to send any
> initialization parameters to it at the point where it is swapped in. Size
> information comes from the platform widget in the browser.
> 

That makes sense, thanks.  So then it seems ok to go ahead with this change
(confirmed with Daniel as well).  I've removed the comment and updated the
description.

[alexmos, 2014-10-14 18:37:01]

alexmos@chromium.org changed reviewers:
+ japhet@chromium.org

[alexmos, 2014-10-14 18:37:44]

Nate, could you please review this for owner's approval?

[Nate Chapin, 2014-10-14 18:45:07]

lgtm

[alexmos, 2014-10-14 19:36:42]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2014-10-14 19:37:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613043005/120001

[commit-bot: I haz the power, 2014-10-14 20:18:46]

Committed patchset #2 (id:120001) as 183695