SurfaceManager::CreateSurface must set the surface's factory

SurfaceManager::CreateSurface must set the surface's factory

This change is necessary because DelegatedFrameHost can be destroyed
and recreated when RendererCompositorFrameSink is alive the whole time
and can potentially send the same LocalSurfaceId to both instances of 
DelegatedFrameHost.

BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2760433002
Cr-Commit-Position: refs/heads/master@{#458138}
Committed: https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b8946604abe71

[Saman Sami, 2017-03-16 18:26:08]

Description was changed from

==========
SurfaceManager::CreateSurface must set the surface's factory
==========

to

==========
SurfaceManager::CreateSurface must set the surface's factory
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-16 18:26:15]

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

[commit-bot: I haz the power, 2017-03-16 18:27:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-16 18:28:59]

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

[Saman Sami, 2017-03-16 18:29:05]

Description was changed from

==========
SurfaceManager::CreateSurface must set the surface's factory
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
SurfaceManager::CreateSurface must set the surface's factory
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-16 18:29:06]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[commit-bot: I haz the power, 2017-03-16 18:29:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-16 18:29:49]

PTAL

[Saman Sami, 2017-03-16 18:50:10]

Description was changed from

==========
SurfaceManager::CreateSurface must set the surface's factory
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
SurfaceManager::CreateSurface must set the surface's factory

This change is necessary because DelegatedFrameHost can be destroyed
and recreated when RendererCompositorFrameSink is alive the whole time
and can potentially send the same LocalSurfaceId to both instances of 
DelegatedFrameHost.

BUG=697864

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-03-16 19:25:57]

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

[commit-bot: I haz the power, 2017-03-16 19:25:58]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-16 23:31:49]

LGTM

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1067: CompositorFrame());
MakeCompositorFrame(empty_surface_ids())

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1069:
EXPECT_TRUE(surface);
ASSERT_TRUE(surface)

[Saman Sami, 2017-03-16 23:42:22]

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

[Saman Sami, 2017-03-16 23:42:36]

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

[commit-bot: I haz the power, 2017-03-16 23:42:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-16 23:43:53]

jbauman@ PTAL

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1067: CompositorFrame());
On 2017/03/16 23:31:49, Fady Samuel wrote:
> MakeCompositorFrame(empty_surface_ids())

Done.

https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1069:
EXPECT_TRUE(surface);
On 2017/03/16 23:31:49, Fady Samuel wrote:
> ASSERT_TRUE(surface)

Done.

[Fady Samuel, 2017-03-17 01:01:11]

https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1085: CompositorFrame());
Oops, here too: MakeCompositorFrame(empty_surface_ids())

[commit-bot: I haz the power, 2017-03-17 01:50:49]

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

[commit-bot: I haz the power, 2017-03-17 01:50:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-03-17 20:25:50]

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

[commit-bot: I haz the power, 2017-03-17 20:26:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2017-03-17 20:56:55]

So the previous resources will be leaked if the Surface is destroyed before the
Surface is reused? Do we know how this happens?

[Fady Samuel, 2017-03-17 21:03:02]

On 2017/03/17 20:56:55, jbauman wrote:
> So the previous resources will be leaked if the Surface is destroyed before
the
> Surface is reused? Do we know how this happens?

Yes, it's a bug. Site isolation folks are looking into it.

https://bugs.chromium.org/p/chromium/issues/detail?id=701988

[jbauman, 2017-03-17 21:19:58]

lgtm, though I think there are some additional pitfalls you have to worry about.

I think this could hit the DCHECK in SurfaceResourceHolder::RefResources if the
Surface is aggregated after it's reattached before a new frame is given, though
that probably isn't likely given the way the DelegatedFrameHost uses it now.

Also, if the same resources ids are in use both before and after the surface is
reused then they'll be returned early, as the refcount will be off. Though this
also seems a bit unlikely, depending on the reason the DelegatedFrameHost is
being created.

[Saman Sami, 2017-03-17 21:52:06]

On 2017/03/17 21:19:58, jbauman wrote:
> lgtm, though I think there are some additional pitfalls you have to worry
about.
> 
> I think this could hit the DCHECK in SurfaceResourceHolder::RefResources if
the
> Surface is aggregated after it's reattached before a new frame is given,
though
> that probably isn't likely given the way the DelegatedFrameHost uses it now.
> 
> Also, if the same resources ids are in use both before and after the surface
is
> reused then they'll be returned early, as the refcount will be off. Though
this
> also seems a bit unlikely, depending on the reason the DelegatedFrameHost is
> being created.

Thanks for the lgtm. I'll look into to SurfaceResourceHolder. I just cc'd you to
the bug report of this issue and also just added a comment about why this is
happening. I hope that helps.

[commit-bot: I haz the power, 2017-03-18 02:01:27]

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

[commit-bot: I haz the power, 2017-03-18 02:01:28]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-20 16:20:03]

Patchset #4 (id:60001) has been deleted

[Saman Sami, 2017-03-20 16:21:40]

Based on the discussion I had with fsamuel@, probably evicting the frames if
factory changes is the best solution. I updated the CL accordingly.

[Saman Sami, 2017-03-20 16:21:54]

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

[commit-bot: I haz the power, 2017-03-20 16:22:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-20 16:24:40]

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

[commit-bot: I haz the power, 2017-03-20 16:25:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-20 16:26:09]

https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1085: CompositorFrame());
On 2017/03/17 01:01:10, Fady Samuel wrote:
> Oops, here too: MakeCompositorFrame(empty_surface_ids())

Done.

[commit-bot: I haz the power, 2017-03-20 17:01:53]

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

[commit-bot: I haz the power, 2017-03-20 17:01:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-03-20 17:35:14]

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

[commit-bot: I haz the power, 2017-03-20 17:35:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-20 17:37:07]

PTAL. Recent changes warrant another look IMO.

[commit-bot: I haz the power, 2017-03-20 18:35:45]

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

[commit-bot: I haz the power, 2017-03-20 18:35:46]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-20 18:37:52]

Still LGTM. It's unfortunate we need to do this, but I support it in the
interest of making forward progress while other bugs are being resolved.

[Saman Sami, 2017-03-20 18:39:52]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-20 18:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2760433002/#ps120001
(title: "Reset")

[commit-bot: I haz the power, 2017-03-20 18:40:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 18:56:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490035192332190,
"parent_rev": "492fc92cdb9f2f5d555c4bf92cbc897dbf3952d4", "commit_rev":
"9e0fd9a87d69362103af512b8e7b8946604abe71"}

[commit-bot: I haz the power, 2017-03-20 18:56:48]

Description was changed from

==========
SurfaceManager::CreateSurface must set the surface's factory

This change is necessary because DelegatedFrameHost can be destroyed
and recreated when RendererCompositorFrameSink is alive the whole time
and can potentially send the same LocalSurfaceId to both instances of 
DelegatedFrameHost.

BUG=697864

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
SurfaceManager::CreateSurface must set the surface's factory

This change is necessary because DelegatedFrameHost can be destroyed
and recreated when RendererCompositorFrameSink is alive the whole time
and can potentially send the same LocalSurfaceId to both instances of 
DelegatedFrameHost.

BUG=697864

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2760433002
Cr-Commit-Position: refs/heads/master@{#458138}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b...
==========

[commit-bot: I haz the power, 2017-03-20 18:56:49]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b...

[Saman Sami, 2017-04-03 17:37:26]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/2794993002/ by samans@chromium.org.

The reason for reverting is: Should not be necessary anymore now that
crbug.com/701988 is fixed..