Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9eb60ebf171a8fc0ba919d65c72661a13960f5b6
Cr-Commit-Position: refs/heads/master@{#401685}

[xlai (Olivia), 2016-06-22 19:38:01]

Description was changed from

==========
fix

BUG=
==========

to

==========
fix

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Justin Novosad, 2016-06-22 20:47:09]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2016-06-22 20:47:09]

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory.cc
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory...
cc/surfaces/surface_factory.cc:65: Surface* old_surface =
manager_->GetSurfaceForId(old_id);
Do you need a null check on manager_ here?

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory...
cc/surfaces/surface_factory.cc:88: if (!manager_->SurfaceModified(surface_id)) {
and here

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory...
cc/surfaces/surface_factory.cc:104: manager_->SurfaceModified(surface_id);
and here?

https://codereview.chromium.org/2087383002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2087383002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:29: if
(!GetSurfaceManager()) {
Is is not possible to call GetSurfaceManager in SurfaceFactory?  Would be nice
to just get rid of SurfaceFactory::manager_

[xlai (Olivia), 2016-06-23 14:48:38]

Description was changed from

==========
fix

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[xlai (Olivia), 2016-06-23 14:48:39]

xlai@chromium.org changed reviewers:
+ jbauman@chromium.org

[xlai (Olivia), 2016-06-23 14:48:53]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 14:49:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087383002/20001

[xlai (Olivia), 2016-06-23 14:49:39]

I now know what I miss yesterday---The SurfaceIdAllocator's destructor needs to
be fixed too. This one should fix the
error on Mac.

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory.cc
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2087383002/diff/1/cc/surfaces/surface_factory...
cc/surfaces/surface_factory.cc:65: Surface* old_surface =
manager_->GetSurfaceForId(old_id);
On 2016/06/22 20:47:09, Justin Novosad wrote:
> Do you need a null check on manager_ here?

I don't think we should put manager_ nullity check everywhere. The only problem
here only happens during its destruction (and the fact that it's  only get
caught by ClusterFuzz means the disordering of manager_'s destruction is not
that obvious).

https://codereview.chromium.org/2087383002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2087383002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:29: if
(!GetSurfaceManager()) {
On 2016/06/22 20:47:09, Justin Novosad wrote:
> Is is not possible to call GetSurfaceManager in SurfaceFactory?  Would be nice
> to just get rid of SurfaceFactory::manager_

Cannot...cc/ folder cannot import things from content/...

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_id_...
File cc/surfaces/surface_id_allocator.h (right):

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_id_...
cc/surfaces/surface_id_allocator.h:40: void didDestroySurfaceManager() {
manager_ = nullptr; }
Rename this based on feedback from junov@ on another temp CL

[commit-bot: I haz the power, 2016-06-23 16:17:18]

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

[commit-bot: I haz the power, 2016-06-23 16:17:19]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-06-23 19:25:22]

non-owner lgtm

[xlai (Olivia), 2016-06-23 19:30:32]

Description was changed from

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[xlai (Olivia), 2016-06-23 19:31:26]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-06-23 19:32:05]

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

[commit-bot: I haz the power, 2016-06-23 19:37:37]

Description was changed from

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-23 19:37:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-23 19:38:48]

Description was changed from

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix destruction order between SurfaceManger and OffscreenCanvasSurfaceImpl

In a whole chain of objects tear down during a page closure, SurfaceManager
is unexpectedly marked to be deleted before OffscreenCanvasSurfaceImpl is
torn down. But OffscreenCanvasSurfaceImpl's two members still rely on
manager_ in their destructors. This CL makes sure that before
OffscreenCanvasSurfaceImpl is torn down, it checks whether SurfaceManager is
alive and informs its two members (SurfaceIdAllocator and SurfaceFactory)
about this.

TBR=jbauman@chromium.org
BUG=621849
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9eb60ebf171a8fc0ba919d65c72661a13960f5b6
Cr-Commit-Position: refs/heads/master@{#401685}
==========

[commit-bot: I haz the power, 2016-06-23 19:38:50]

Patchset 2 (id:??) landed as
https://crrev.com/9eb60ebf171a8fc0ba919d65c72661a13960f5b6
Cr-Commit-Position: refs/heads/master@{#401685}

[jbauman, 2016-06-23 19:46:59]

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:88: void didDestroySurfaceManager() { manager_ =
nullptr; }
This should be called DidDestroySurfaceManager in chromium style.

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_id_...
File cc/surfaces/surface_id_allocator.h (right):

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_id_...
cc/surfaces/surface_id_allocator.h:40: void didDestroySurfaceManager() {
manager_ = nullptr; }
Same here.

[xlai (Olivia), 2016-06-23 20:09:28]

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/2087383002/diff/20001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:88: void didDestroySurfaceManager() { manager_ =
nullptr; }
On 2016/06/23 19:46:57, jbauman wrote:
> This should be called DidDestroySurfaceManager in chromium style.

Sorry I just landed the patch after being pushed to fix beta-blocker asap. I'll
follow up with a patch (if this one is not reverted) to fix style here and some
other places. 
Actually, I'm thinking probably "did_destroy_surface_manager()" may be better
because according to Google C++ guide it says cheap function like this may use
this style.