Cleanup various WebView/WebFrame APIs to properly handle remote frames.

Cleanup various WebView/WebFrame APIs to properly handle remote frames.

WebCore can now also properly set a RemoteFrame as the main frame.

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

[dcheng, 2014-06-17 04:22:28]

I had to throw in a call to relaxAdoptionRequirement() to prevent debug crashes,
since setting the main frame takes a reference to the frame, but the original
reference hasn't yet been adopted at that point.

I'm trying to see if there's a better solution, since otherwise, we'd be able to
remove relaxAdoptionRequirement() from RefCounted completely...

[eseidel, 2014-06-17 18:00:29]

https://codereview.chromium.org/338993003/diff/40001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/338993003/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:79: relaxAdoptionRequirement();
I find it difficult to imagine why this function exists, let alone that we would
ever want to call it. :)

[jamesr, 2014-06-17 18:04:13]

The function exists so that callers can add references before adopting, but
that's always the sign of a real bug.  The adoption requirement is in place to
avoid bugs where the refcount goes to 1 and back to 0 before construction has
fully completed (signalled by the adoptRef call).  In this situation, it appears
that you have code attempting to manipulate the refcount before initialization
is done.  Can't you refactor things to only hand out references once the object
is fully ready to go?

[dcheng, 2014-06-17 18:14:22]

On 2014/06/17 at 18:04:13, jamesr wrote:
> The function exists so that callers can add references before adopting, but
that's always the sign of a real bug.  The adoption requirement is in place to
avoid bugs where the refcount goes to 1 and back to 0 before construction has
fully completed (signalled by the adoptRef call).  In this situation, it appears
that you have code attempting to manipulate the refcount before initialization
is done.  Can't you refactor things to only hand out references once the object
is fully ready to go?

I can, but not without duplicating code into RemoteFrame::create and
LocalFrame::create. It's not a lot of code, but the duplication makes me a
little sad. I've uploaded a patch that doesn't require this for comparison.

I tried other solutions yesterday which involved lifting the
Page::setMainFrame() call into Source/web, but it turns out without duplicating
code there, it's impossible to place it in a common location too =(

[jamesr, 2014-06-17 18:17:22]

I'm not super familiar with the ownership here - why does setting the main frame
take an ownership stake in the frame?  If the main frame can delete the child
(which taking a reference implies that it can) then it definitely should only do
so after the adoptRef() call.

[dcheng, 2014-06-17 18:27:23]

On 2014/06/17 at 18:17:22, jamesr wrote:
> I'm not super familiar with the ownership here - why does setting the main
frame take an ownership stake in the frame?  If the main frame can delete the
child (which taking a reference implies that it can) then it definitely should
only do so after the adoptRef() call.

I'm not entirely sure I follow here--which child are you referring to?

I suspect Page takes a reference to the main frame just to prevent it from
having a pointer to a deleted object--I don't think we clear the main frame
pointer, even during teardown, so it makes sure other code referencing
Page::mainFrame() doesn't cause a UaF.

[jamesr, 2014-06-17 18:30:37]

On 2014/06/17 18:27:23, dcheng wrote:
> On 2014/06/17 at 18:17:22, jamesr wrote:
> > I'm not super familiar with the ownership here - why does setting the main
> frame take an ownership stake in the frame?  If the main frame can delete the
> child (which taking a reference implies that it can) then it definitely should
> only do so after the adoptRef() call.
> 
> I'm not entirely sure I follow here--which child are you referring to?

Sorry, was mixing up the page with a child frame.

> 
> I suspect Page takes a reference to the main frame just to prevent it from
> having a pointer to a deleted object--I don't think we clear the main frame
> pointer, even during teardown, so it makes sure other code referencing
> Page::mainFrame() doesn't cause a UaF.

OK - if the Page has a reference to the main frame, that means that it has an
ownership stake (i.e. can delete the main frame).  If so, then it doesn't make
sense to let the page destroy the main frame before adoption has completed
because the XXFrame::create() functions are not prepared to take a dead object.

If the Page is not supposed to delete the main frame then it should not take a
reference to it.

[dcheng, 2014-06-17 18:41:05]

On 2014/06/17 at 18:30:37, jamesr wrote:
> On 2014/06/17 18:27:23, dcheng wrote:
> > On 2014/06/17 at 18:17:22, jamesr wrote:
> > > I'm not super familiar with the ownership here - why does setting the main
> > frame take an ownership stake in the frame?  If the main frame can delete
the
> > child (which taking a reference implies that it can) then it definitely
should
> > only do so after the adoptRef() call.
> > 
> > I'm not entirely sure I follow here--which child are you referring to?
> 
> Sorry, was mixing up the page with a child frame.
> 
> > 
> > I suspect Page takes a reference to the main frame just to prevent it from
> > having a pointer to a deleted object--I don't think we clear the main frame
> > pointer, even during teardown, so it makes sure other code referencing
> > Page::mainFrame() doesn't cause a UaF.
> 
> OK - if the Page has a reference to the main frame, that means that it has an
ownership stake (i.e. can delete the main frame).  If so, then it doesn't make
sense to let the page destroy the main frame before adoption has completed
because the XXFrame::create() functions are not prepared to take a dead object.
> 
> If the Page is not supposed to delete the main frame then it should not take a
reference to it.

It's possible we could make it a raw pointer... but I think that's best saved
for another patch. This is how it's been written for a long time, and I'm
worried about what else might tip over when I try changing it.

[dcheng, 2014-06-17 19:26:10]

On 2014/06/17 at 18:41:05, dcheng wrote:
> On 2014/06/17 at 18:30:37, jamesr wrote:
> > On 2014/06/17 18:27:23, dcheng wrote:
> > > On 2014/06/17 at 18:17:22, jamesr wrote:
> > > > I'm not super familiar with the ownership here - why does setting the
main
> > > frame take an ownership stake in the frame?  If the main frame can delete
the
> > > child (which taking a reference implies that it can) then it definitely
should
> > > only do so after the adoptRef() call.
> > > 
> > > I'm not entirely sure I follow here--which child are you referring to?
> > 
> > Sorry, was mixing up the page with a child frame.
> > 
> > > 
> > > I suspect Page takes a reference to the main frame just to prevent it from
> > > having a pointer to a deleted object--I don't think we clear the main
frame
> > > pointer, even during teardown, so it makes sure other code referencing
> > > Page::mainFrame() doesn't cause a UaF.
> > 
> > OK - if the Page has a reference to the main frame, that means that it has
an ownership stake (i.e. can delete the main frame).  If so, then it doesn't
make sense to let the page destroy the main frame before adoption has completed
because the XXFrame::create() functions are not prepared to take a dead object.
> > 
> > If the Page is not supposed to delete the main frame then it should not take
a reference to it.
> 
> It's possible we could make it a raw pointer... but I think that's best saved
for another patch. This is how it's been written for a long time, and I'm
worried about what else might tip over when I try changing it.

(Oops, reuploaded another patch properly updated. I left a few diffs out by
accident while testing out the suggestion from James)

[dcheng, 2014-06-17 22:18:38]

PTAL. I've updated it so setting the main frame doesn't require ref'ing it. It
seems to be OK (with ASAN enabled) locally, and the bots (so far) seem to be
happy with it.

[eseidel, 2014-06-19 16:55:31]

lgtm

I'm OK with this so long as you document in a comment why that weakptr is safe.

https://codereview.chromium.org/338993003/diff/120001/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/338993003/diff/120001/Source/core/page/Page.h...
Source/core/page/Page.h:262: Frame* m_mainFrame;
So the Page no longer owns its main frame.  Someone else is responsible for
that... and it would be good to document in a comment why this is a weak
pointer.

[jamesr, 2014-06-19 17:46:15]

I agree some comments describing the ownership model would be nice but the code
lgtm

[dcheng, 2014-06-19 20:19:57]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:20:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/338993003/140001

[eseidel, 2014-06-19 20:21:11]

lgtm

[dcheng, 2014-06-19 20:21:43]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:21:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/338993003/160001

[commit-bot: I haz the power, 2014-06-19 21:28:20]

Change committed as 176551