Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2685393003:
Surfaces: Allow parent to refer to child surfaces that don't exist yet. (Closed)
|
Created:
3 years, 10 months ago by Fady Samuel Modified:
3 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org, kylechar Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSurfaces: Allow parent to refer to child surfaces that don't exist yet.
With the introduction of direct client-to-client resize and surface
synchronization, the parent may submit a CompositorFrame referring to
a child that does not yet exist. This should be OK. There will be a
dangling reference for a little while but eventually it will either
resolve, or the parent is malicious and will be killed and its entire
subtree will be reclaimed.
If the child submits a CompositorFrame for the first time and there's
already a reference to it hanging off the parent then we should not
add a temporary reference hanging off the root.
This CL tests that both cases are handled.
BUG=672062
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2685393003
Cr-Commit-Position: refs/heads/master@{#449870}
Committed: https://chromium.googlesource.com/chromium/src/+/adb877a00fb19240fd50186c21bcc2b790446805
Patch Set 1 #Patch Set 2 : Fixed compile issue (uploaded too soon) #Patch Set 3 : Added unit test #
Total comments: 11
Patch Set 4 : Addressed comments and removed a now invalid unit test #Patch Set 5 : Added a TODO to cleanup tests so that they don't create empty sets #
Total comments: 2
Patch Set 6 : Created bug for TODO #Patch Set 7 : Rebased #
Messages
Total messages: 36 (24 generated)
Description was changed from ========== WIP: Allow parent to refer to child surfaces that don't exist yet. BUG= ========== to ========== WIP: Allow parent to refer to child surfaces that don't exist yet. 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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
Description was changed from ========== WIP: Allow parent to refer to child surfaces that don't exist yet. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Surfaces: Allow parent to refer to child surfaces that don't exist yet. With the introduction of direct client-to-client resize and surface synchronization, the parent may submit a CompositorFrame referring to a child that does not yet exist. This should be OK. There will be a dangling reference for a little while but eventually it will either resolve, or the parent is malicious and will be killed and its entire subtree will be reclaimed. If the child submits a CompositorFrame for the first time and there's already a reference to it hanging off the parent then we should not add a temporary reference hanging off the root. This CL tests that both cases are handled. BUG=672062 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
fsamuel@chromium.org changed reviewers: + vmpstr@chromium.org
+kylechar@, rjkroege@ FYI +vmpstr@ for review.
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...
I assume the ids here are solid in the sense that we don't want to add a danging reference, and then have a different surface bind to it? https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:84: const SurfaceManager::SurfaceIdSet& GetReferencesFrom( GetChildReferences? https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:90: std::vector<LocalSurfaceId> GetTempReferencesFor( GetTempReferences? https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:476: UnorderedElementsAre(child_id)); Does this mean there's just one element and it's child_id? It's kind of weird to use EXPECT_THAT for that :P https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:480: EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(child_id)); same here
On 2017/02/10 19:14:31, Fady Samuel wrote: > +kylechar@, rjkroege@ FYI > +vmpstr@ for review. question in passing: what happens if a malicious client submits many CFs before any embedder submits a CF that references them? Is there logic to track that a client is "spammy" and if not, perhaps there should be?
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:476: UnorderedElementsAre(child_id)); On 2017/02/10 19:36:00, vmpstr wrote: > Does this mean there's just one element and it's child_id? It's kind of weird to > use EXPECT_THAT for that :P I like EXPECT_THAT, so I'm kind of biased, but it seems less verbose than writing something like: EXPECT_EQ(std::unordered_set<SurfaceId>{child_id}, parent_surface()->blocking_surfaces_for_testing()); It's also tolerant to changing the container type. https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:583: bool has_real_reference = child_to_parent_refs_.find(surface_info.id()) != It's a map from SurfaceId -> unordered_set<SurfaceId>. You're only checking a set exists. It could be an empty set, so 0 references.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/10 19:36:59, rjkroege wrote: > On 2017/02/10 19:14:31, Fady Samuel wrote: > > +kylechar@, rjkroege@ FYI > > +vmpstr@ for review. > > question in passing: what happens if a malicious client submits many CFs before > any embedder submits a CF that references them? Is there logic to track that a > client is "spammy" and if not, perhaps there should be? We keep a a vector of temporary references and I believe as soon as a parent embeds one of those child IDs, the previous IDs are cleaned up. kylechar@ wrote that code.
PTAL Vlad! Thanks https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:84: const SurfaceManager::SurfaceIdSet& GetReferencesFrom( On 2017/02/10 19:36:00, vmpstr wrote: > GetChildReferences? Done. https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:90: std::vector<LocalSurfaceId> GetTempReferencesFor( On 2017/02/10 19:36:00, vmpstr wrote: > GetTempReferences? Done. https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:476: UnorderedElementsAre(child_id)); On 2017/02/10 19:51:27, kylechar wrote: > On 2017/02/10 19:36:00, vmpstr wrote: > > Does this mean there's just one element and it's child_id? It's kind of weird > to > > use EXPECT_THAT for that :P > > I like EXPECT_THAT, so I'm kind of biased, but it seems less verbose than > writing something like: > > EXPECT_EQ(std::unordered_set<SurfaceId>{child_id}, > parent_surface()->blocking_surfaces_for_testing()); > > It's also tolerant to changing the container type. I like EXPECT_THAT too. Learned it from Kyle's code and adopted it :-) https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:480: EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(child_id)); On 2017/02/10 19:36:00, vmpstr wrote: > same here Acknowledged. https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2685393003/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:583: bool has_real_reference = child_to_parent_refs_.find(surface_info.id()) != On 2017/02/10 19:51:27, kylechar wrote: > It's a map from SurfaceId -> unordered_set<SurfaceId>. You're only checking a > set exists. It could be an empty set, so 0 references. Done.
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
PTAL Vlad
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2685393003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2685393003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:586: // use array notation into maps in tests. file a bug to keep track plz
I need to rebase this when my other patch lands so no point in putting it in the CQ just yet. https://codereview.chromium.org/2685393003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2685393003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:586: // use array notation into maps in tests. On 2017/02/10 22:25:23, vmpstr wrote: > file a bug to keep track plz Done.
lgtm
The CQ bit was checked by fsamuel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2685393003/#ps120001 (title: "Rebased")
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": 120001, "attempt_start_ts": 1486829528229790,
"parent_rev": "5940469d7d03df91984f64476b9ffdd34abff931", "commit_rev":
"adb877a00fb19240fd50186c21bcc2b790446805"}
Message was sent while issue was closed.
Description was changed from ========== Surfaces: Allow parent to refer to child surfaces that don't exist yet. With the introduction of direct client-to-client resize and surface synchronization, the parent may submit a CompositorFrame referring to a child that does not yet exist. This should be OK. There will be a dangling reference for a little while but eventually it will either resolve, or the parent is malicious and will be killed and its entire subtree will be reclaimed. If the child submits a CompositorFrame for the first time and there's already a reference to it hanging off the parent then we should not add a temporary reference hanging off the root. This CL tests that both cases are handled. BUG=672062 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Surfaces: Allow parent to refer to child surfaces that don't exist yet. With the introduction of direct client-to-client resize and surface synchronization, the parent may submit a CompositorFrame referring to a child that does not yet exist. This should be OK. There will be a dangling reference for a little while but eventually it will either resolve, or the parent is malicious and will be killed and its entire subtree will be reclaimed. If the child submits a CompositorFrame for the first time and there's already a reference to it hanging off the parent then we should not add a temporary reference hanging off the root. This CL tests that both cases are handled. BUG=672062 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2685393003 Cr-Commit-Position: refs/heads/master@{#449870} Committed: https://chromium.googlesource.com/chromium/src/+/adb877a00fb19240fd50186c21bc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/adb877a00fb19240fd50186c21bc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
