Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716553004:
Add temporary reference ownership to SurfaceManager. (Closed)
DescriptionAdd temporary reference ownership to SurfaceManager.
We normally remove a temporary reference when the first surface
reference is added for a surface. If something happens to the parent
client, say it crashes, it may never add a surface reference. We would
be left with dangling temporary references and memory wouldn't get
freed.
Start adding a system to SurfaceManager where each temporary reference
is assigned an owner by a trusted process. This assignment will happen
from the mus-ws process (or browser process). The owner is the
FrameSinkId that is expected to embed the new Surface. If the owner
FrameSinkId is invalidated, all temporary references it owns are removed
and the Surfaces can cleaned up.
The mus-ws and IPC parts of this will be added in a followup CL.
BUG=683738
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2716553004
Cr-Commit-Position: refs/heads/master@{#453599}
Committed: https://chromium.googlesource.com/chromium/src/+/803cb96d3089cc214ee46062cf3759a13646b172
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 20
Patch Set 3 : Rebase on new CLs. #Patch Set 4 : Fix comments. #Patch Set 5 : Rebase #
Depends on Patchset: Messages
Total messages: 22 (11 generated)
Description was changed from ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. Change how temporary references are stored to simplify this. There is a map that contains all of the temporary references now and an optional owner. This simplifies and lets us check for temporary references in O(1) time. Add new tests for temporary reference assignment and do some cleanup on existing tests. The code to use new SurfaceManager functionality will happen in a followup CL. BUG=683738 ========== to ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. Change how temporary references are stored to simplify this. There is a map that contains all of the temporary references now and an optional owner. This simplifies and lets us check for temporary references in O(1) time. Add new tests for temporary reference assignment and do some cleanup on existing tests. The code to use new SurfaceManager functionality will happen in a followup CL. BUG=683738 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
kylechar@chromium.org changed reviewers: + fsamuel@chromium.org
+fsamuel for overall review, will loop in OWNERS if we're on the same page.
This CL contains a bunch of subtle code as well as a bunch of cleanup/refactoring. Is it possible to do the no-op cleanup/refactoring first, then introduce the new functionality in a second CL? That'll make it easier for me to focus on the new functionality. Thanks! https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:183: SurfaceId last_surface_id = reference_tracker_.current_surface_id(); Why did you move this? https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:75: // Temporary references will have an asterix in front of them. s/asterix/asterisk
https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:411: TEST_F(SurfaceManagerRefTest, TemporaryReferenceNotGarbageCollected) { Top level comment about what the unit test is trying to verify. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:419: // Destroy both surfaces and remove the reference to |id1|. Add comment about when garbage collection happens. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:424: // Without any references |id1| is deleted. There is still a temporary Move the "There is still a temporary reference to |id2| so it is not deleted" to the next line? https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:430: TEST_F(SurfaceManagerRefTest, AssignTempReference) { Top level comment about what the unit test is trying to verify. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:444: TEST_F(SurfaceManagerRefTest, InvalidateHasNoEffectOnSurfaceReferences) { Top level comment about what the unit test is trying to verify. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:463: TEST_F(SurfaceManagerRefTest, DropTempReference) { Top level comment about what the unit test is trying to verify. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:474: TEST_F(SurfaceManagerRefTest, DropOnlyOneTempReference) { Top level comment about what the unit test is trying to verify. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:493: // If the owner is invalidated then the Window Server might not Incomplete sentence?
I've addressed a couple of comments and split this CL. I'll address the comments left in this CL next. (1) https://codereview.chromium.org/2714973002/ (2) https://codereview.chromium.org/2715973003/ https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:183: SurfaceId last_surface_id = reference_tracker_.current_surface_id(); On 2017/02/24 18:10:14, Fady Samuel wrote: > Why did you move this? The order was backwards. Moved change to https://codereview.chromium.org/2714973002 https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:411: TEST_F(SurfaceManagerRefTest, TemporaryReferenceNotGarbageCollected) { On 2017/02/24 19:08:11, Fady Samuel wrote: > Top level comment about what the unit test is trying to verify. I changed the name to be more descriptive. PTAL in other CL. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:419: // Destroy both surfaces and remove the reference to |id1|. On 2017/02/24 19:08:11, Fady Samuel wrote: > Add comment about when garbage collection happens. Done in other CL. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:424: // Without any references |id1| is deleted. There is still a temporary On 2017/02/24 19:08:11, Fady Samuel wrote: > Move the "There is still a temporary reference to |id2| so it is not deleted" to > the next line? Done in other CL.
https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:75: // Temporary references will have an asterix in front of them. On 2017/02/24 18:10:15, Fady Samuel wrote: > s/asterix/asterisk Done. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:430: TEST_F(SurfaceManagerRefTest, AssignTempReference) { On 2017/02/24 19:08:11, Fady Samuel wrote: > Top level comment about what the unit test is trying to verify. Done. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:444: TEST_F(SurfaceManagerRefTest, InvalidateHasNoEffectOnSurfaceReferences) { On 2017/02/24 19:08:11, Fady Samuel wrote: > Top level comment about what the unit test is trying to verify. Done. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:463: TEST_F(SurfaceManagerRefTest, DropTempReference) { On 2017/02/24 19:08:11, Fady Samuel wrote: > Top level comment about what the unit test is trying to verify. Done-ish. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:474: TEST_F(SurfaceManagerRefTest, DropOnlyOneTempReference) { On 2017/02/24 19:08:11, Fady Samuel wrote: > Top level comment about what the unit test is trying to verify. Done. https://codereview.chromium.org/2716553004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:493: // If the owner is invalidated then the Window Server might not On 2017/02/24 19:08:11, Fady Samuel wrote: > Incomplete sentence? Done.
Description was changed from ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. Change how temporary references are stored to simplify this. There is a map that contains all of the temporary references now and an optional owner. This simplifies and lets us check for temporary references in O(1) time. Add new tests for temporary reference assignment and do some cleanup on existing tests. The code to use new SurfaceManager functionality will happen in a followup CL. BUG=683738 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. The mus-ws and IPC parts of this will be added in a followup CL. BUG=683738 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The refactor parts have been split and this has been rebased on that change. PTAL.
lgtm
kylechar@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman for OWNERS
lgtm
The CQ bit was checked by kylechar@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: This issue passed the CQ dry run.
The CQ bit was checked by kylechar@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": 70001, "attempt_start_ts": 1488293456847750,
"parent_rev": "77b422d33e131d50f4af616b93fde525300c0072", "commit_rev":
"803cb96d3089cc214ee46062cf3759a13646b172"}
Message was sent while issue was closed.
Description was changed from ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. The mus-ws and IPC parts of this will be added in a followup CL. BUG=683738 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add temporary reference ownership to SurfaceManager. We normally remove a temporary reference when the first surface reference is added for a surface. If something happens to the parent client, say it crashes, it may never add a surface reference. We would be left with dangling temporary references and memory wouldn't get freed. Start adding a system to SurfaceManager where each temporary reference is assigned an owner by a trusted process. This assignment will happen from the mus-ws process (or browser process). The owner is the FrameSinkId that is expected to embed the new Surface. If the owner FrameSinkId is invalidated, all temporary references it owns are removed and the Surfaces can cleaned up. The mus-ws and IPC parts of this will be added in a followup CL. BUG=683738 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2716553004 Cr-Commit-Position: refs/heads/master@{#453599} Committed: https://chromium.googlesource.com/chromium/src/+/803cb96d3089cc214ee46062cf37... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as https://chromium.googlesource.com/chromium/src/+/803cb96d3089cc214ee46062cf37... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
