Cleanup ChildFrameCompositingHelper on destruction.

Cleanup ChildFrameCompositingHelper on destruction.

BUG=448507
Committed: https://crrev.com/a7d368e724924635100c64b5c7e2483acae117bb
Cr-Commit-Position: refs/heads/master@{#314426}

[lfg, 2015-01-16 23:26:55]

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

[lfg, 2015-02-03 19:33:08]

lfg@chromium.org changed reviewers:
+ danakj@chromium.org
- kenrb@chromium.org

[lfg, 2015-02-03 19:33:08]

Hi, kenrb@ suggested you as a reviewer, PTAL.

[danakj, 2015-02-03 20:07:42]

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
content/renderer/child_frame_compositing_helper.cc:66:
resource_collection_->SetClient(NULL);
Destruction implies the child compositor is shutting down? Or do we need to
lose+return all the resources in the collection first?

[lfg, 2015-02-03 20:26:22]

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
content/renderer/child_frame_compositing_helper.cc:66:
resource_collection_->SetClient(NULL);
On 2015/02/03 20:07:42, danakj wrote:
> Destruction implies the child compositor is shutting down? Or do we need to
> lose+return all the resources in the collection first?

The child compositor is shutting down. I'm not sure if we need to return the
resources in the collection.

The issue I'm trying to fix is that since the resource collection is refcounted
and may be referenced in the cc tree, it may be left with a dangling pointer to
the ChildFrameCompositingHelper. Then, if we update the cc tree at the wrong
time we may get a crash.

[danakj, 2015-02-03 20:40:10]

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/857643002/diff/1/content/renderer/child_frame...
content/renderer/child_frame_compositing_helper.cc:66:
resource_collection_->SetClient(NULL);
On 2015/02/03 20:26:22, lfg wrote:
> On 2015/02/03 20:07:42, danakj wrote:
> > Destruction implies the child compositor is shutting down? Or do we need to
> > lose+return all the resources in the collection first?
> 
> The child compositor is shutting down. I'm not sure if we need to return the
> resources in the collection.
> 
> The issue I'm trying to fix is that since the resource collection is
refcounted
> and may be referenced in the cc tree, it may be left with a dangling pointer
to
> the ChildFrameCompositingHelper. Then, if we update the cc tree at the wrong
> time we may get a crash.

Right. But if you just drop the client, you'll leak any resources being held by
this compositor instead of returning them to the child compositor ever. If it's
shutting down anyway, it'll just delete/drop its refs anyways so that's okay. If
it's not you leak them until it does shutdown.

So, it sounds like this is okay then. LGTM

[lfg, 2015-02-03 20:58:01]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2015-02-03 20:58:01]

Charlie, PTAL.

[Charlie Reis, 2015-02-03 21:13:21]

The bug report makes it sound like this solves an issue in the
NavigateRemoteFrame test.  Can we enable that test now as a way to test this
change?

Apart from that, I'm happy to defer to danakj and rubber stamp this.

[lfg, 2015-02-03 21:16:02]

On 2015/02/03 21:13:21, Charlie Reis wrote:
> The bug report makes it sound like this solves an issue in the
> NavigateRemoteFrame test.  Can we enable that test now as a way to test this
> change?
> 
> Apart from that, I'm happy to defer to danakj and rubber stamp this.

No, to enable that test we also need this blink change
https://codereview.chromium.org/842033003/ , and there's some flakyness in that
test, where sometimes the frame owner element is null.


I'll follow-up with another CL to enable that test.

[Charlie Reis, 2015-02-03 21:26:58]

On 2015/02/03 21:16:02, lfg wrote:
> On 2015/02/03 21:13:21, Charlie Reis wrote:
> > The bug report makes it sound like this solves an issue in the
> > NavigateRemoteFrame test.  Can we enable that test now as a way to test this
> > change?
> > 
> > Apart from that, I'm happy to defer to danakj and rubber stamp this.
> 
> No, to enable that test we also need this blink change
> https://codereview.chromium.org/842033003/ , and there's some flakyness in
that
> test, where sometimes the frame owner element is null.
> 
> 
> I'll follow-up with another CL to enable that test.

Ok, LGTM.

[lfg, 2015-02-03 22:56:40]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-02-03 22:57:47]

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

[commit-bot: I haz the power, 2015-02-03 23:02:05]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-03 23:03:07]

Patchset 1 (id:??) landed as
https://crrev.com/a7d368e724924635100c64b5c7e2483acae117bb
Cr-Commit-Position: refs/heads/master@{#314426}