Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731743002:
Decouple FrameSink Hierarchy registration from SurfaceFactoryClient registration (Closed)
|
Created:
3 years, 9 months ago by Fady Samuel Modified:
3 years, 9 months ago Reviewers:
enne (OOO) CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDecouple FrameSink Hierarchy registration from SurfaceFactoryClient
Prior to this patch, FrameSink hierarchy registration and
SurfaceFactoryClient registration were tightly coupled in
SurfaceManager.
In SurfaceManager's destructor, it verified that the
frame_sink_source_map_ is empty. This proved problematic in Mus+Ash
because the Mus window server tries to unregister the framesink
hierarchy at shutdown by issuing IPCs to Mus-GPU. Thus, there is
no guarantee these IPCs will arrive in the gpu process prior to
SurfaceManager destruction.
By design, Mojo provides no guarantees on process shutdown order, and
even if a process tries to override the behavior, Mojo may someday
decide to force quit processes and so we cannot guarantee Mus-WS will
outlive Mus-GPU.
Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger
than the set of FrameSinkIds with an associated SurfaceFactoryClient
(an associated CompositorFrameSink), in order to facilitate reparenting
windows in an environment where not all windows submit
CompositorFrames (a single client may embed only a single
CompositorFrame that embeds a bunch of local window concepts for
performance reasons). It seems to make sense to decouple the BeginFrame
hierarchy from the SurfaceFactoryClient hierarchy, in that case.
Furthermore, the DCHECK in the SurfaceManager destructor exists to
verify that there are no lingering clients and we haven't been
wastefully ticking BeginFrames. By decoupling client registration and
BeginFrame registration, we can continue to guard against that
potential bug while allowing Mus+Ash to have a clean shutdown.
BUG=697116
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2731743002
Cr-Commit-Position: refs/heads/master@{#454641}
Committed: https://chromium.googlesource.com/chromium/src/+/462ef8c3894d3dc1eceb4a2327006b372737f749
Patch Set 1 #Patch Set 2 : Fix some SurfaceManager unit tests #Patch Set 3 : Fixed #
Messages
Total messages: 25 (20 generated)
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient registration BUG= ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient registration BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient registration BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
fsamuel@chromium.org changed reviewers: + enne@chromium.org
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
PTAL Enne!
lgtm, thanks for working through all of this. :)
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. Thus, there is no guarantee these IPCs will arrive in the gpu process prior to SurfaceManager destruction. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 10003, "attempt_start_ts": 1488568054911910,
"parent_rev": "6fca2b025fc31a0baee84e938bdf1383a3be6fb4", "commit_rev":
"462ef8c3894d3dc1eceb4a2327006b372737f749"}
Message was sent while issue was closed.
Description was changed from ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. Thus, there is no guarantee these IPCs will arrive in the gpu process prior to SurfaceManager destruction. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Decouple FrameSink Hierarchy registration from SurfaceFactoryClient Prior to this patch, FrameSink hierarchy registration and SurfaceFactoryClient registration were tightly coupled in SurfaceManager. In SurfaceManager's destructor, it verified that the frame_sink_source_map_ is empty. This proved problematic in Mus+Ash because the Mus window server tries to unregister the framesink hierarchy at shutdown by issuing IPCs to Mus-GPU. Thus, there is no guarantee these IPCs will arrive in the gpu process prior to SurfaceManager destruction. By design, Mojo provides no guarantees on process shutdown order, and even if a process tries to override the behavior, Mojo may someday decide to force quit processes and so we cannot guarantee Mus-WS will outlive Mus-GPU. Currently, in Mus+Ash, the BeginFrame hierarchy can be much larger than the set of FrameSinkIds with an associated SurfaceFactoryClient (an associated CompositorFrameSink), in order to facilitate reparenting windows in an environment where not all windows submit CompositorFrames (a single client may embed only a single CompositorFrame that embeds a bunch of local window concepts for performance reasons). It seems to make sense to decouple the BeginFrame hierarchy from the SurfaceFactoryClient hierarchy, in that case. Furthermore, the DCHECK in the SurfaceManager destructor exists to verify that there are no lingering clients and we haven't been wastefully ticking BeginFrames. By decoupling client registration and BeginFrame registration, we can continue to guard against that potential bug while allowing Mus+Ash to have a clean shutdown. BUG=697116 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2731743002 Cr-Commit-Position: refs/heads/master@{#454641} Committed: https://chromium.googlesource.com/chromium/src/+/462ef8c3894d3dc1eceb4a232700... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:10003) as https://chromium.googlesource.com/chromium/src/+/462ef8c3894d3dc1eceb4a232700... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
