Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058213002:
Never destroy the root content layer unless leaving compositing mode. (Closed)
|
Created:
4 years, 6 months ago by chrishtr Modified:
4 years, 6 months ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNever destroy the root content layer unless leaving compositing mode.
BUG=617800
Committed: https://crrev.com/f214ea22ef9f0434500f0fa1266e8fa4741d7f7f
Cr-Commit-Position: refs/heads/master@{#399777}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 20 (8 generated)
Description was changed from ========== none BUG= ========== to ========== Never destroy the root content layer unless leaving compositing mode. BUG=617800 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org
Unfortunately, despite trying pretty hard, I was unable to find a reasonable test for this. It seems to require setting up a frame that somehow remains composited yet has no content.
https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:440: if (!childList.isEmpty()) { If we are leaving compositing, could we write: if (staleInCompositingMode()) destroyRootLayer() ? I am worried about leaving around stale root layers. Is this codepath hit in any layout test? If so, could you use that test to add an assert somewhere that ensures the root layer is destroyed?
https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:440: if (!childList.isEmpty()) { On 2016/06/10 at 21:07:08, pdr. wrote: > If we are leaving compositing, could we write: > if (staleInCompositingMode()) > destroyRootLayer() The bug is that in compositing mode we are destroying. We shouldn't do that. > > ? > > I am worried about leaving around stale root layers. Is this codepath hit in any layout test? If so, could you use that test to add an assert somewhere that ensures the root layer is destroyed? Yes it will stay around sometimes, but I don't think that will cause any issues. I will try running the layout tests with such an assert.
On 2016/06/10 at 21:20:01, chrishtr wrote: > https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): > > https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:440: if (!childList.isEmpty()) { > On 2016/06/10 at 21:07:08, pdr. wrote: > > If we are leaving compositing, could we write: > > if (staleInCompositingMode()) > > destroyRootLayer() > > The bug is that in compositing mode we are destroying. We shouldn't do that. > > > > > ? > > > > I am worried about leaving around stale root layers. Is this codepath hit in any layout test? If so, could you use that test to add an assert somewhere that ensures the root layer is destroyed? > > Yes it will stay around sometimes, but I don't think that will cause any issues. I will try running > the layout tests with such an assert. I tried, and nothing crashed: https://codereview.chromium.org/2063493002
On 2016/06/13 at 00:53:13, chrishtr wrote: > On 2016/06/10 at 21:20:01, chrishtr wrote: > > https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (right): > > > > https://codereview.chromium.org/2058213002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:440: if (!childList.isEmpty()) { > > On 2016/06/10 at 21:07:08, pdr. wrote: > > > If we are leaving compositing, could we write: > > > if (staleInCompositingMode()) > > > destroyRootLayer() > > > > The bug is that in compositing mode we are destroying. We shouldn't do that. > > > > > > > > ? > > > > > > I am worried about leaving around stale root layers. Is this codepath hit in any layout test? If so, could you use that test to add an assert somewhere that ensures the root layer is destroyed? > > > > Yes it will stay around sometimes, but I don't think that will cause any issues. I will try running > > the layout tests with such an assert. > > I tried, and nothing crashed: > > https://codereview.chromium.org/2063493002 LGTM to land but I'd prefer not to merge without a test.
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058213002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Never destroy the root content layer unless leaving compositing mode. BUG=617800 ========== to ========== Never destroy the root content layer unless leaving compositing mode. BUG=617800 Committed: https://crrev.com/f214ea22ef9f0434500f0fa1266e8fa4741d7f7f Cr-Commit-Position: refs/heads/master@{#399777} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f214ea22ef9f0434500f0fa1266e8fa4741d7f7f Cr-Commit-Position: refs/heads/master@{#399777} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
