Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387333003:
cc: Set sync token on resource even if context is lost. (Closed)
Descriptioncc: Set sync token on resource even if context is lost.
If the gpu process crashes the worker context will return null sync
tokens which the resource write lock ignores. It's possible for the
compositor context to not detect the gpu process crash until much later.
Until then the scheduler might initiate a draw which will crash when
DCHECKing that resources being sent have sync tokens.
This CL also enables sync tokens for LayerTreeTests by default and
includes some minor cleanup in LayerTreeHostTestContext.
R=brianderson@chromium.org,danakj@chromium.org
BUG=645736
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/e9fd952d2b376783ae04864e5fcdec647cf2942e
Cr-Commit-Position: refs/heads/master@{#423720}
Patch Set 1 #
Total comments: 2
Patch Set 2 : test #
Total comments: 17
Patch Set 3 : danakj's review #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 34 (20 generated)
Description was changed from ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=piman@chromium.org,brianderson@chromium.org,danakj@chromium.org BUG=645736 ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=piman@chromium.org,brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
PTAL
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 ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=piman@chromium.org,brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=piman@chromium.org,brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
sunnyps@chromium.org changed reviewers: - piman@chromium.org
Description was changed from ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=piman@chromium.org,brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we unit test? https://codereview.chromium.org/2387333003/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2387333003/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.cc:1127: if (set_sync_token_) { Explain this in a comment please. (Basically what you have in the CL description.)
Description was changed from ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. This CL also enables sync tokens for LayerTreeTests by default and includes some minor cleanup in LayerTreeHostTestContext. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Added a test which required enabling sync tokens in tests (see thread on graphics-dev). Also did some minor cleanup in LayerTreeHostTestContext. PTAL https://codereview.chromium.org/2387333003/diff/1/cc/resources/resource_provi... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2387333003/diff/1/cc/resources/resource_provi... cc/resources/resource_provider.cc:1127: if (set_sync_token_) { On 2016/10/05 01:44:05, danakj wrote: > Explain this in a comment please. (Basically what you have in the CL > description.) I removed the DCHECK here because the compositor context isn't necessarily lost if the compositor and worker aren't in a share group.
The CQ bit was checked by sunnyps@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...
https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... cc/resources/resource_provider.cc:1104: set_sync_token_(false), nit: has_sync_token_ https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... cc/resources/resource_provider.cc:1127: if (set_sync_token_) Can you leave a comment saying why you're not checking HasData() tho for future code wayfarers? https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:69: capabilities_.delegated_sync_points_required = true; Can you leave a comment explaining why this is being set true https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:85: // We want the Display's OutputSurface to hear about lost context, and since and when this shares https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.h (right): https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.h:100: bool display_context_shared_with_compositor_ = false; nit: group with other bool bound_? maybe above it because it is finalized earlier? also this can be const if you init it as a constructor initializer which looks valid https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (left): https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:79: virtual std::unique_ptr<TestWebGraphicsContext3D> CreateContext3d() { Oh cool nothing overrides this anymore. https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:84: std::unique_ptr<TestWebGraphicsContext3D> context3d = nit: compositor_context3d https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1620: // Paint non-solid color. client_.set_fill_with_nonsolid_color(true) is less code https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1640: gl->LoseContextCHROMIUM(GL_GUILTY_CONTEXT_RESET_ARB, Can you leave some comments in this test saying what it is verifying? Like the context is lost, ok, what is the expected result vs what could go wrong?
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 sunnyps@chromium.org to run a CQ dry run
Addressed comments, PTAL. https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... cc/resources/resource_provider.cc:1104: set_sync_token_(false), On 2016/10/06 20:50:35, danakj wrote: > nit: has_sync_token_ Done. https://codereview.chromium.org/2387333003/diff/20001/cc/resources/resource_p... cc/resources/resource_provider.cc:1127: if (set_sync_token_) On 2016/10/06 20:50:35, danakj wrote: > Can you leave a comment saying why you're not checking HasData() tho for future > code wayfarers? Done. https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:69: capabilities_.delegated_sync_points_required = true; On 2016/10/06 20:50:35, danakj wrote: > Can you leave a comment explaining why this is being set true Done. https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:85: // We want the Display's OutputSurface to hear about lost context, and since On 2016/10/06 20:50:35, danakj wrote: > and when this shares Done. https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.h (right): https://codereview.chromium.org/2387333003/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.h:100: bool display_context_shared_with_compositor_ = false; On 2016/10/06 20:50:35, danakj wrote: > nit: group with other bool bound_? maybe above it because it is finalized > earlier? > > also this can be const if you init it as a constructor initializer which looks > valid Done. https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:84: std::unique_ptr<TestWebGraphicsContext3D> context3d = On 2016/10/06 20:50:35, danakj wrote: > nit: compositor_context3d Done. https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1620: // Paint non-solid color. On 2016/10/06 20:50:35, danakj wrote: > client_.set_fill_with_nonsolid_color(true) is less code Done. https://codereview.chromium.org/2387333003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1640: gl->LoseContextCHROMIUM(GL_GUILTY_CONTEXT_RESET_ARB, On 2016/10/06 20:50:35, danakj wrote: > Can you leave some comments in this test saying what it is verifying? Like the > context is lost, ok, what is the expected result vs what could go wrong? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The code LGTM, but one q. https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1618: // state e.g. null sync tokens but that shouldn't cause the draw to fail. High level question, should we be looking for a GL error on the worker context and not drawing? Or what do you think the best thing to be doing in this situation is?
Thanks! https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1618: // state e.g. null sync tokens but that shouldn't cause the draw to fail. On 2016/10/06 22:09:04, danakj wrote: > High level question, should we be looking for a GL error on the worker context > and not drawing? Or what do you think the best thing to be doing in this > situation is? In prod worker context loss implies compositor context loss because they're in a share group. In test worker and compositor contexts aren't in a share group which is why the DCHECK(resource_provider_->IsGLContextLost()) wasn't working. We could check the worker context instead but even that's an implementation detail that the resource provider shouldn't worry about. We could abort the draw if worker context is lost (by explicitly checking instead of a callback) but since the context can be lost at any time it's better to make the code safe and wait for a callback. As part of enabling async worker context we'll have to plumb worker context loss to the compositor somehow. I don't know if this means binding the worker context to compositor thread or if there's some other way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:1618: // state e.g. null sync tokens but that shouldn't cause the draw to fail. On 2016/10/06 22:29:21, sunnyps wrote: > On 2016/10/06 22:09:04, danakj wrote: > > High level question, should we be looking for a GL error on the worker context > > and not drawing? Or what do you think the best thing to be doing in this > > situation is? > > In prod worker context loss implies compositor context loss because they're in a > share group. In test worker and compositor contexts aren't in a share group > which is why the DCHECK(resource_provider_->IsGLContextLost()) wasn't working. They could be put into a share group (I thought I had done that but maybe I only did it for pixel tests). > We could check the worker context instead but even that's an implementation > detail that the resource provider shouldn't worry about. I agree. > We could abort the draw if worker context is lost (by explicitly checking > instead of a callback) but since the context can be lost at any time it's better > to make the code safe and wait for a callback. Yeah I'm really wondering what it means to have worker context lost and the compositor doesn't know. It's going to draw black tiles essentially, and think that the resources are complete so it won't recreate them or anything. So if worker context is lost and we're trying to support compositor context /not/ being lost too, then I feel like we need to check and release the output surface anyway at some point here or risk really bad stuff. > As part of enabling async worker context we'll have to plumb worker context loss > to the compositor somehow. I don't know if this means binding the worker context > to compositor thread or if there's some other way. This is the heart of what I am wondering about. Should our unit tests be testing non-share-group contexts if we're not using them yet and don't have code to deal with that situation fully formed?
On 2016/10/06 22:47:42, danakj wrote: > https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_unittest_context.cc (right): > > https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_unittest_context.cc:1618: // state e.g. null sync > tokens but that shouldn't cause the draw to fail. > On 2016/10/06 22:29:21, sunnyps wrote: > > On 2016/10/06 22:09:04, danakj wrote: > > > High level question, should we be looking for a GL error on the worker > context > > > and not drawing? Or what do you think the best thing to be doing in this > > > situation is? > > > > In prod worker context loss implies compositor context loss because they're in > a > > share group. In test worker and compositor contexts aren't in a share group > > which is why the DCHECK(resource_provider_->IsGLContextLost()) wasn't working. > > They could be put into a share group (I thought I had done that but maybe I only > did it for pixel tests). > > > We could check the worker context instead but even that's an implementation > > detail that the resource provider shouldn't worry about. > > I agree. > > > We could abort the draw if worker context is lost (by explicitly checking > > instead of a callback) but since the context can be lost at any time it's > better > > to make the code safe and wait for a callback. > > Yeah I'm really wondering what it means to have worker context lost and the > compositor doesn't know. It's going to draw black tiles essentially, and think > that the resources are complete so it won't recreate them or anything. So if > worker context is lost and we're trying to support compositor context /not/ > being lost too, then I feel like we need to check and release the output surface > anyway at some point here or risk really bad stuff. > > > As part of enabling async worker context we'll have to plumb worker context > loss > > to the compositor somehow. I don't know if this means binding the worker > context > > to compositor thread or if there's some other way. > > This is the heart of what I am wondering about. Should our unit tests be testing > non-share-group contexts if we're not using them yet and don't have code to deal > with that situation fully formed? Even though we're not dealing with non-share-group contexts the compositor context loss callback can occur much later than worker context loss (which causes null sync tokens). In the tests though the only way to simulate that is to use non-share-group contexts because share group test contexts propagate context loss to all contexts immediately.
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/2387333003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:100: TestContextProvider::Create(std::move(compositor_context3d)), Can you explicitly point out that the context is not in a share group with the worker and why? It's accidental in the base class but here we would actually not want to change it or at least we'd have to think about it.
On Thu, Oct 6, 2016 at 3:54 PM, <sunnyps@chromium.org> wrote: > On 2016/10/06 22:47:42, danakj wrote: > > > https://codereview.chromium.org/2387333003/diff/40001/cc/ > trees/layer_tree_host_unittest_context.cc > > File cc/trees/layer_tree_host_unittest_context.cc (right): > > > > > https://codereview.chromium.org/2387333003/diff/40001/cc/ > trees/layer_tree_host_unittest_context.cc#newcode1618 > > cc/trees/layer_tree_host_unittest_context.cc:1618: // state e.g. null > sync > > tokens but that shouldn't cause the draw to fail. > > On 2016/10/06 22:29:21, sunnyps wrote: > > > On 2016/10/06 22:09:04, danakj wrote: > > > > High level question, should we be looking for a GL error on the > worker > > context > > > > and not drawing? Or what do you think the best thing to be doing in > this > > > > situation is? > > > > > > In prod worker context loss implies compositor context loss because > they're > in > > a > > > share group. In test worker and compositor contexts aren't in a share > group > > > which is why the DCHECK(resource_provider_->IsGLContextLost()) wasn't > working. > > > > They could be put into a share group (I thought I had done that but > maybe I > only > > did it for pixel tests). > > > > > We could check the worker context instead but even that's an > implementation > > > detail that the resource provider shouldn't worry about. > > > > I agree. > > > > > We could abort the draw if worker context is lost (by explicitly > checking > > > instead of a callback) but since the context can be lost at any time > it's > > better > > > to make the code safe and wait for a callback. > > > > Yeah I'm really wondering what it means to have worker context lost and > the > > compositor doesn't know. It's going to draw black tiles essentially, and > think > > that the resources are complete so it won't recreate them or anything. > So if > > worker context is lost and we're trying to support compositor context > /not/ > > being lost too, then I feel like we need to check and release the output > surface > > anyway at some point here or risk really bad stuff. > > > > > As part of enabling async worker context we'll have to plumb worker > context > > loss > > > to the compositor somehow. I don't know if this means binding the > worker > > context > > > to compositor thread or if there's some other way. > > > > This is the heart of what I am wondering about. Should our unit tests be > testing > > non-share-group contexts if we're not using them yet and don't have code > to > deal > > with that situation fully formed? > > Even though we're not dealing with non-share-group contexts the compositor > context loss callback can occur much later than worker context loss (which > causes null sync tokens). In the tests though the only way to simulate > that is > to use non-share-group contexts because share group test contexts propagate > context loss to all contexts immediately. > Ah okay, gotcha. So for now assumption is that in this situation, the compositor context will definitely be lost soon, and drawing won't work/be visible anyways. You could simulate this more closely by making delays/preventing callbacks in TestContextProvider if you want also, if that feels truer to life, or if not then separate context groups work for this I guess too! > > https://codereview.chromium.org/2387333003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. This CL also enables sync tokens for LayerTreeTests by default and includes some minor cleanup in LayerTreeHostTestContext. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. This CL also enables sync tokens for LayerTreeTests by default and includes some minor cleanup in LayerTreeHostTestContext. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. This CL also enables sync tokens for LayerTreeTests by default and includes some minor cleanup in LayerTreeHostTestContext. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Set sync token on resource even if context is lost. If the gpu process crashes the worker context will return null sync tokens which the resource write lock ignores. It's possible for the compositor context to not detect the gpu process crash until much later. Until then the scheduler might initiate a draw which will crash when DCHECKing that resources being sent have sync tokens. This CL also enables sync tokens for LayerTreeTests by default and includes some minor cleanup in LayerTreeHostTestContext. R=brianderson@chromium.org,danakj@chromium.org BUG=645736 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/e9fd952d2b376783ae04864e5fcdec647cf2942e Cr-Commit-Position: refs/heads/master@{#423720} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e9fd952d2b376783ae04864e5fcdec647cf2942e Cr-Commit-Position: refs/heads/master@{#423720} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
