Oilpan: extend tracing over WebFrame trees.

Oilpan: extend tracing over WebFrame trees.

Have the tracing also encompass the parent and opener.

Also refreshed some unrelated LocalFrame handling comments.

TBR=tkent
R=haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183241

[sof, 2014-09-22 10:03:38]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-09-22 10:03:39]

Please take a look.

[haraken, 2014-09-22 10:12:31]

LGTM, thanks for a quick fix!

[sof, 2014-09-22 12:23:25]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-22 12:24:05]

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

[commit-bot: I haz the power, 2014-09-22 12:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-22 12:33:54]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[sof, 2014-09-22 12:40:16]

On 2014/09/22 12:33:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

Changes in web/ needs approval.

tkent@: could you add this to your queue and have a look, please? tia.

[dcheng, 2014-09-22 16:04:20]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[dcheng, 2014-09-22 16:04:20]

Hmm. Ever since I moved the frame tree from Source/core to Source/web, a
WebFrame node hasn't explicitly kept its parent/opener/children alive--they used
to be RefPtrs in FrameTree, but now they are just raw pointers. So Blink has
already been relying on the embedder to do the right thing for some time now.

I would argue that we shouldn't trace over the opener at all--this field is
cleared if the opener WebFrame closes by the OpenedFrameTracker helper.

As for tracing over parent/children, I think its fine to match the non-Oilpan
behavior and remove it completely. But I don't feel as strongly about this.

[sof, 2014-09-22 18:38:38]

On 2014/09/22 16:04:20, dcheng (OOO) wrote:
> Hmm. Ever since I moved the frame tree from Source/core to Source/web, a
> WebFrame node hasn't explicitly kept its parent/opener/children alive--they
used
> to be RefPtrs in FrameTree, but now they are just raw pointers. So Blink has
> already been relying on the embedder to do the right thing for some time now.
> 
> I would argue that we shouldn't trace over the opener at all--this field is
> cleared if the opener WebFrame closes by the OpenedFrameTracker helper.
> 
> As for tracing over parent/children, I think its fine to match the non-Oilpan
> behavior and remove it completely. But I don't feel as strongly about this.

So this is (solid) evidence that the tracing isn't needed, strictly speaking.
But it is somewhat unsatisfying to have bare on-heap pointers that we don't
trace.

I'm willing to accept not doing it though.. what's the Oilpan consensus?

[haraken, 2014-09-22 22:44:39]

On 2014/09/22 18:38:38, sof wrote:
> On 2014/09/22 16:04:20, dcheng (OOO) wrote:
> > Hmm. Ever since I moved the frame tree from Source/core to Source/web, a
> > WebFrame node hasn't explicitly kept its parent/opener/children alive--they
> used
> > to be RefPtrs in FrameTree, but now they are just raw pointers. So Blink has
> > already been relying on the embedder to do the right thing for some time
now.
> > 
> > I would argue that we shouldn't trace over the opener at all--this field is
> > cleared if the opener WebFrame closes by the OpenedFrameTracker helper.
> > 
> > As for tracing over parent/children, I think its fine to match the
non-Oilpan
> > behavior and remove it completely. But I don't feel as strongly about this.
> 
> So this is (solid) evidence that the tracing isn't needed, strictly speaking.
> But it is somewhat unsatisfying to have bare on-heap pointers that we don't
> trace.
> 
> I'm willing to accept not doing it though.. what's the Oilpan consensus?

I'd prefer not doing the tracing with a good comment :)

[Mads Ager (chromium), 2014-09-23 07:04:28]

On 2014/09/22 22:44:39, haraken wrote:
> On 2014/09/22 18:38:38, sof wrote:
> > On 2014/09/22 16:04:20, dcheng (OOO) wrote:
> > > Hmm. Ever since I moved the frame tree from Source/core to Source/web, a
> > > WebFrame node hasn't explicitly kept its parent/opener/children
alive--they
> > used
> > > to be RefPtrs in FrameTree, but now they are just raw pointers. So Blink
has
> > > already been relying on the embedder to do the right thing for some time
> now.
> > > 
> > > I would argue that we shouldn't trace over the opener at all--this field
is
> > > cleared if the opener WebFrame closes by the OpenedFrameTracker helper.
> > > 
> > > As for tracing over parent/children, I think its fine to match the
> non-Oilpan
> > > behavior and remove it completely. But I don't feel as strongly about
this.
> > 
> > So this is (solid) evidence that the tracing isn't needed, strictly
speaking.
> > But it is somewhat unsatisfying to have bare on-heap pointers that we don't
> > trace.
> > 
> > I'm willing to accept not doing it though.. what's the Oilpan consensus?
> 
> I'd prefer not doing the tracing with a good comment :)

I agree with Sigbjorn that it is unsatisfying to have bare on-heap pointers that
are not traced. I agree that the tracing is not *needed*, however, we are
tracing for safety and we only get safety if all pointers are traced. If,
somehow, a bug is introduced that means that a Frame gets deallocated (close is
called by the embedder) but all pointers to the Frame are not removed from other
Frame objects you can get a use-after-free situation if you are not tracing. If
we are tracing that will no longer be possible and you will get a leak instead
of a security issue. I prefer accidentally adding a leak to accidentally adding
a use-after-free situation. Therefore, I would definitely trace all of these
pointers.

[haraken, 2014-09-23 07:29:55]

On 2014/09/23 07:04:28, Mads Ager (chromium) wrote:
> On 2014/09/22 22:44:39, haraken wrote:
> > On 2014/09/22 18:38:38, sof wrote:
> > > On 2014/09/22 16:04:20, dcheng (OOO) wrote:
> > > > Hmm. Ever since I moved the frame tree from Source/core to Source/web, a
> > > > WebFrame node hasn't explicitly kept its parent/opener/children
> alive--they
> > > used
> > > > to be RefPtrs in FrameTree, but now they are just raw pointers. So Blink
> has
> > > > already been relying on the embedder to do the right thing for some time
> > now.
> > > > 
> > > > I would argue that we shouldn't trace over the opener at all--this field
> is
> > > > cleared if the opener WebFrame closes by the OpenedFrameTracker helper.
> > > > 
> > > > As for tracing over parent/children, I think its fine to match the
> > non-Oilpan
> > > > behavior and remove it completely. But I don't feel as strongly about
> this.
> > > 
> > > So this is (solid) evidence that the tracing isn't needed, strictly
> speaking.
> > > But it is somewhat unsatisfying to have bare on-heap pointers that we
don't
> > > trace.
> > > 
> > > I'm willing to accept not doing it though.. what's the Oilpan consensus?
> > 
> > I'd prefer not doing the tracing with a good comment :)
> 
> I agree with Sigbjorn that it is unsatisfying to have bare on-heap pointers
that
> are not traced. I agree that the tracing is not *needed*, however, we are
> tracing for safety and we only get safety if all pointers are traced. If,
> somehow, a bug is introduced that means that a Frame gets deallocated (close
is
> called by the embedder) but all pointers to the Frame are not removed from
other
> Frame objects you can get a use-after-free situation if you are not tracing.
If
> we are tracing that will no longer be possible and you will get a leak instead
> of a security issue. I prefer accidentally adding a leak to accidentally
adding
> a use-after-free situation. Therefore, I would definitely trace all of these
> pointers.

Yeah, I'm now convinced. Agreed with Mads.

[dcheng, 2014-09-23 17:36:05]

Two thoughts:
- Right now, we already have OpenedFrameTracker, which clears m_opener fields
that point to a WebFrame being destroyed.
- But since WebFrame objects live in the Oilpan heap, running this in the
destructor is unsafe, right? So we need to move when this code runs anyway...

[sof, 2014-09-23 17:47:20]

On 2014/09/23 17:36:05, dcheng wrote:
> Two thoughts:
> - Right now, we already have OpenedFrameTracker, which clears m_opener fields
> that point to a WebFrame being destroyed.
> - But since WebFrame objects live in the Oilpan heap, running this in the
> destructor is unsafe, right? So we need to move when this code runs anyway...

If the WebFrames kept by the OpenedFrameTracker have been finalized already,
that's a definite bug. I will have a closer look if that's a possibility.

[sof, 2014-09-24 10:36:32]

On 2014/09/23 17:47:20, sof wrote:
> On 2014/09/23 17:36:05, dcheng wrote:
> > Two thoughts:
> > - Right now, we already have OpenedFrameTracker, which clears m_opener
fields
> > that point to a WebFrame being destroyed.
> > - But since WebFrame objects live in the Oilpan heap, running this in the
> > destructor is unsafe, right? So we need to move when this code runs
anyway...
> 
> If the WebFrames kept by the OpenedFrameTracker have been finalized already,
> that's a definite bug. I will have a closer look if that's a possibility.

Should an opener and an opened WebFrame end up being swept at the same time,
it's clearly possible, but I haven't been able to trigger this.

Consequently, I've made m_opener weak for Oilpan. And added tracing over the
OpenedFrameTracker hash set also. Please take another look?

[haraken, 2014-09-24 11:26:21]

On 2014/09/24 10:36:32, sof wrote:
> On 2014/09/23 17:47:20, sof wrote:
> > On 2014/09/23 17:36:05, dcheng wrote:
> > > Two thoughts:
> > > - Right now, we already have OpenedFrameTracker, which clears m_opener
> fields
> > > that point to a WebFrame being destroyed.
> > > - But since WebFrame objects live in the Oilpan heap, running this in the
> > > destructor is unsafe, right? So we need to move when this code runs
> anyway...
> > 
> > If the WebFrames kept by the OpenedFrameTracker have been finalized already,
> > that's a definite bug. I will have a closer look if that's a possibility.
> 
> Should an opener and an opened WebFrame end up being swept at the same time,
> it's clearly possible, but I haven't been able to trigger this.
> 
> Consequently, I've made m_opener weak for Oilpan. And added tracing over the
> OpenedFrameTracker hash set also. Please take another look?

Hmm, the tracing looks a bit complicated.

Just a random idea but would it be possible to introduce WebFrameProxy such
that:

- WebFrame is off-heap (Since WebFrame is exposed to the embedder, we cannot
move WebFrame to the heap).
- WebFrameProxy is on-heap.
- Change the WebFrame* pointers to Member<WebFrameProxy>s and trace them. (If we
want to trace WebFrame*, we use Member<WebFrameProxy> instead.)

[sof, 2014-09-24 11:51:58]

On 2014/09/24 11:26:21, haraken wrote:
> On 2014/09/24 10:36:32, sof wrote:
> > On 2014/09/23 17:47:20, sof wrote:
> > > On 2014/09/23 17:36:05, dcheng wrote:
> > > > Two thoughts:
> > > > - Right now, we already have OpenedFrameTracker, which clears m_opener
> > fields
> > > > that point to a WebFrame being destroyed.
> > > > - But since WebFrame objects live in the Oilpan heap, running this in
the
> > > > destructor is unsafe, right? So we need to move when this code runs
> > anyway...
> > > 
> > > If the WebFrames kept by the OpenedFrameTracker have been finalized
already,
> > > that's a definite bug. I will have a closer look if that's a possibility.
> > 
> > Should an opener and an opened WebFrame end up being swept at the same time,
> > it's clearly possible, but I haven't been able to trigger this.
> > 
> > Consequently, I've made m_opener weak for Oilpan. And added tracing over the
> > OpenedFrameTracker hash set also. Please take another look?
> 
> Hmm, the tracing looks a bit complicated.
> 
> Just a random idea but would it be possible to introduce WebFrameProxy such
> that:
> 
> - WebFrame is off-heap (Since WebFrame is exposed to the embedder, we cannot
> move WebFrame to the heap).

WebFrames are on heap, but an implementation detail not known/of concern to the
embedder.

> - WebFrameProxy is on-heap.
> - Change the WebFrame* pointers to Member<WebFrameProxy>s and trace them. (If
we
> want to trace WebFrame*, we use Member<WebFrameProxy> instead.)

We're in a tight spot with using Member<> inside public/web/WebFrame.h.

[haraken, 2014-09-24 14:13:04]

Sorry I was misunderstanding the code.

LGTM!

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:251: return true;

Shouldn't this be false?

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:257: #else

Is it possible that isAlive is called in non-oilpan?

[sof, 2014-09-24 14:24:34]

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:251: return true;
On 2014/09/24 14:13:04, haraken wrote:
> 
> Shouldn't this be false?

It could be, I'll move the null check to the call site instead and avoid the
repeated frame->m_opener clearing that way.

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:257: #else
On 2014/09/24 14:13:04, haraken wrote:
> 
> Is it possible that isAlive is called in non-oilpan?

Not possible, but I've resisted the temptation to use "#if ENABLE(OILPAN)"
inside "#if BLINK_IMPLEMENTATION" blocks in public/ header files. Is it allowed?

[haraken, 2014-09-24 14:28:51]

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
Source/web/WebFrame.cpp:257: #else
On 2014/09/24 14:24:34, sof wrote:
> On 2014/09/24 14:13:04, haraken wrote:
> > 
> > Is it possible that isAlive is called in non-oilpan?
> 
> Not possible, but I've resisted the temptation to use "#if ENABLE(OILPAN)"
> inside "#if BLINK_IMPLEMENTATION" blocks in public/ header files. Is it
allowed?

I'd guess it's allowed.

Even if it's not allowed, you can write ASSERT_NOT_REACHED here since non-oilpan
doesn't call the isAlive method.

[sof, 2014-09-24 14:41:25]

On 2014/09/24 14:28:51, haraken wrote:
> https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp
> File Source/web/WebFrame.cpp (right):
> 
>
https://codereview.chromium.org/594483002/diff/20001/Source/web/WebFrame.cpp#...
> Source/web/WebFrame.cpp:257: #else
> On 2014/09/24 14:24:34, sof wrote:
> > On 2014/09/24 14:13:04, haraken wrote:
> > > 
> > > Is it possible that isAlive is called in non-oilpan?
> > 
> > Not possible, but I've resisted the temptation to use "#if ENABLE(OILPAN)"
> > inside "#if BLINK_IMPLEMENTATION" blocks in public/ header files. Is it
> allowed?
> 
> I'd guess it's allowed.
> 
> Even if it's not allowed, you can write ASSERT_NOT_REACHED here since
non-oilpan
> doesn't call the isAlive method.

Good idea, assert added.

We have no such atm, but if the public/ reviewer prefers it, then let's
#if-protect these new static methods.

[dcheng, 2014-09-25 08:04:54]

https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
File Source/web/OpenedFrameTracker.cpp (right):

https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
Source/web/OpenedFrameTracker.cpp:52: WebFrame::traceFrame(visitor, *it);
Doesn't this mean the opened frame tracker is keeping these frames alive? Frames
should have a weak pointer to their opener and to any frames they opened.

[sof, 2014-09-25 08:11:47]

https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
File Source/web/OpenedFrameTracker.cpp (right):

https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
Source/web/OpenedFrameTracker.cpp:52: WebFrame::traceFrame(visitor, *it);
On 2014/09/25 08:04:54, dcheng wrote:
> Doesn't this mean the opened frame tracker is keeping these frames alive?
Frames
> should have a weak pointer to their opener and to any frames they opened.

Isn't that handled by the explicit removal that's in place already?

[sof, 2014-09-25 08:25:09]

On 2014/09/25 08:11:47, sof wrote:
>
https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
> File Source/web/OpenedFrameTracker.cpp (right):
> 
>
https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
> Source/web/OpenedFrameTracker.cpp:52: WebFrame::traceFrame(visitor, *it);
> On 2014/09/25 08:04:54, dcheng wrote:
> > Doesn't this mean the opened frame tracker is keeping these frames alive?
> Frames
> > should have a weak pointer to their opener and to any frames they opened.
> 
> Isn't that handled by the explicit removal that's in place already?

i.e., tracing ensures that the WebFrames remain valid while they have to be. It
doesn't prolong membership in this (bare pointer) WebFrame* hash set.

tkent@: would you prefer if we used ENABLE(OILPAN) in WebFrame.h and/or are
there other changes needed in web/,public/ before this one is ready?

[dcheng, 2014-09-25 08:31:33]

On 2014/09/25 at 08:25:09, sigbjornf wrote:
> On 2014/09/25 08:11:47, sof wrote:
> >
https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
> > File Source/web/OpenedFrameTracker.cpp (right):
> > 
> >
https://codereview.chromium.org/594483002/diff/40001/Source/web/OpenedFrameTr...
> > Source/web/OpenedFrameTracker.cpp:52: WebFrame::traceFrame(visitor, *it);
> > On 2014/09/25 08:04:54, dcheng wrote:
> > > Doesn't this mean the opened frame tracker is keeping these frames alive?
> > Frames
> > > should have a weak pointer to their opener and to any frames they opened.
> > 
> > Isn't that handled by the explicit removal that's in place already?
> 
> i.e., tracing ensures that the WebFrames remain valid while they have to be.
It doesn't prolong membership in this (bare pointer) WebFrame* hash set.
> 
> tkent@: would you prefer if we used ENABLE(OILPAN) in WebFrame.h and/or are
there other changes needed in web/,public/ before this one is ready?

I thought I sent my earlier draft, but I guess not. I recalled that the opener
is cleared as part of frame detach but couldn't find it--so if we weren't
clearing the owner in frame detach, then this set would be keeping those frames
alive.

Since we are clearing the opener as part of frame detach (in
FrameLoader::detachClient), this is fine.

[sof, 2014-09-28 21:17:00]

Needs {Source, public}/web/ review.

[dcheng, 2014-09-29 04:11:45]

https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:139:
visitor->registerWeakMembers<WebRemoteFrameImpl,
&WebRemoteFrameImpl::clearWeakMembers>(this);
Why do we need to forward this through WebRemoteFrameImpl/WebLocalFrameImpl?
Can't WebFrame::traceFrames() register this itself?

[sof, 2014-09-29 06:41:25]

https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:139:
visitor->registerWeakMembers<WebRemoteFrameImpl,
&WebRemoteFrameImpl::clearWeakMembers>(this);
On 2014/09/29 04:11:45, dcheng wrote:
> Why do we need to forward this through WebRemoteFrameImpl/WebLocalFrameImpl?
> Can't WebFrame::traceFrames() register this itself?

You could do that, but you have to be careful not to re-register the weak
callback each time you call traceFrame(). Having it here provides a clearer
separation & avoidance of that detail.

I didn't like that quirkiness, which is why it is like in the above, but I've
put up a version that moves weak pointer processing to the WebFrame side
instead. Do we want to do this?

[sof, 2014-09-30 05:01:51]

tkent: ping.

[sof, 2014-10-02 08:40:29]

On 2014/09/29 06:41:25, sof wrote:
>
https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
> File Source/web/WebRemoteFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/594483002/diff/40001/Source/web/WebRemoteFram...
> Source/web/WebRemoteFrameImpl.cpp:139:
> visitor->registerWeakMembers<WebRemoteFrameImpl,
> &WebRemoteFrameImpl::clearWeakMembers>(this);
> On 2014/09/29 04:11:45, dcheng wrote:
> > Why do we need to forward this through WebRemoteFrameImpl/WebLocalFrameImpl?
> > Can't WebFrame::traceFrames() register this itself?
> 
> You could do that, but you have to be careful not to re-register the weak
> callback each time you call traceFrame(). Having it here provides a clearer
> separation & avoidance of that detail.
> 
> I didn't like that quirkiness, which is why it is like in the above, but I've
> put up a version that moves weak pointer processing to the WebFrame side
> instead. Do we want to do this?

What's the collective verdict?

[sof, 2014-10-03 05:08:53]

ping?

[dcheng, 2014-10-03 08:30:40]

On 2014/10/03 at 05:08:53, sigbjornf wrote:
> ping?

Sorry, I'll take a more detailed look at this again tomorrow. It's pretty late
over here, and I don't think my head is up to the task of figuring out all the
complexities of this patch. However, I think your original version was
better--having the bool registerWeak is a bit confusing to follow.

Is there a reason that WebLocalFrameImpl/WebRemoteFrameImpl can't do this in
their traceFrame() though?
  visitor->registerWeakMembers<WebFrame, &WebFrame::clearWeakMembers>(this);
instead of
  visitor->registerWeakMembers<WebLocalFrameImpl,
&WebLocalFrameImpl::clearWeakMembers>(this);

[sof, 2014-10-03 09:04:19]

On 2014/10/03 08:30:40, dcheng wrote:
> On 2014/10/03 at 05:08:53, sigbjornf wrote:
> > ping?
> 
> Sorry, I'll take a more detailed look at this again tomorrow. It's pretty late
> over here, and I don't think my head is up to the task of figuring out all the
> complexities of this patch. However, I think your original version was
> better--having the bool registerWeak is a bit confusing to follow.
> 

thanks, ok - i will undo that version.

> Is there a reason that WebLocalFrameImpl/WebRemoteFrameImpl can't do this in
> their traceFrame() though?
>   visitor->registerWeakMembers<WebFrame, &WebFrame::clearWeakMembers>(this);
> instead of
>   visitor->registerWeakMembers<WebLocalFrameImpl,
> &WebLocalFrameImpl::clearWeakMembers>(this);

You could split it like that, modulo getting the method visibility set up right.
Will do (but does it really matter much?)

[sof, 2014-10-03 12:17:06]

On 2014/10/03 09:04:19, sof wrote:
> On 2014/10/03 08:30:40, dcheng wrote:
> > On 2014/10/03 at 05:08:53, sigbjornf wrote:
> > > ping?
> > 
> > Sorry, I'll take a more detailed look at this again tomorrow. It's pretty
late
> > over here, and I don't think my head is up to the task of figuring out all
the
> > complexities of this patch. However, I think your original version was
> > better--having the bool registerWeak is a bit confusing to follow.
> > 
> 
> thanks, ok - i will undo that version.
> 
> > Is there a reason that WebLocalFrameImpl/WebRemoteFrameImpl can't do this in
> > their traceFrame() though?
> >   visitor->registerWeakMembers<WebFrame, &WebFrame::clearWeakMembers>(this);
> > instead of
> >   visitor->registerWeakMembers<WebLocalFrameImpl,
> > &WebLocalFrameImpl::clearWeakMembers>(this);
> 
> You could split it like that, modulo getting the method visibility set up
right.
> Will do (but does it really matter much?)

Now done.

[sof, 2014-10-05 19:32:59]

TBRing tkent for the web/ changes.

[sof, 2014-10-05 19:33:04]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-05 19:33:56]

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

[commit-bot: I haz the power, 2014-10-05 20:37:19]

Committed patchset #5 (id:80001) as 183241