Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(143)

Issue 2100353002: cc: DCHECK that the OutputSurface was released in ~LayerTreeHostImpl(). (Closed)

Created:
4 years, 5 months ago by danakj
Modified:
4 years, 5 months ago
Reviewers:
jaydasika, enne (OOO)
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, jam, darin-cc_chromium.org, blink-reviews, piman+watch_chromium.org, cc-bugs_chromium.org, jochen+watch_chromium.org, ajuma, jaydasika, piman, vmpstr
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: DCHECK that the OutputSurface was released in ~LayerTreeHostImpl(). The proxy will release the output surface before destroying the LayerTreeHostImpl (and things it depends on). So instead of trying to destroy it again in the destructor, just DCHECK that it's gone. Problem is lots of cc unit tests weren't doing the same, so add a bunch of ReleaseOutputSurface() calls to tests. Mostly they are done in test harnesses like FakeLayerTreeHostImpl, except for the LayerTreeHostImpl tests themselves, since they have a real class (and rightfully so). They get a bunch of manual calls. This exposed another problem. When the output surface is gone, the tile manager is also gone. But if we didn't succeed to release the tiles on layers at the time we destroyed the output surface, then they leak. Turns out a bunch of tests were putting layers in the LayerTreeImpl without getting them into the layer_list_ correctly, so they don't get iterated by ReleaseResources. So this adds a bunch of calls to BuildLayerListForTesting() or BuildLayerListAndPropertyTreesForTesting(). BUG=311404 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/ee6547a2a5d5543ee3fd30749978734b1fd6b17b Cr-Commit-Position: refs/heads/master@{#403516}

Patch Set 1 #

Patch Set 2 : released-outputsurface: . #

Total comments: 3

Patch Set 3 : released-outputsurface: dep #

Patch Set 4 : released-outputsurface: rebase #

Total comments: 2

Patch Set 5 : released-outputsurface: rebase #

Total comments: 1

Patch Set 6 : released-outputsurface: rebase-on-2106273002 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -9 lines) Patch
M cc/layers/picture_layer_unittest.cc View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M cc/test/fake_layer_tree_host_impl.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M cc/test/layer_test_common.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 2 chunks +3 lines, -5 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 13 chunks +25 lines, -3 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 35 (14 generated)
danakj
4 years, 5 months ago (2016-06-27 22:04:35 UTC) #2
danakj
4 years, 5 months ago (2016-06-27 22:04:42 UTC) #4
danakj
Er, this patch got some weird things in it by accident.. sec..
4 years, 5 months ago (2016-06-27 22:05:50 UTC) #6
danakj
ok that's better.
4 years, 5 months ago (2016-06-27 22:06:56 UTC) #7
enne (OOO)
https://codereview.chromium.org/2100353002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2100353002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode2724 cc/trees/layer_tree_host_impl_unittest.cc:2724: host_impl_->ReleaseOutputSurface(); What's with this setup function and some tests ...
4 years, 5 months ago (2016-06-27 22:22:21 UTC) #8
danakj
https://codereview.chromium.org/2100353002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2100353002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode2724 cc/trees/layer_tree_host_impl_unittest.cc:2724: host_impl_->ReleaseOutputSurface(); On 2016/06/27 22:22:20, enne wrote: > What's with ...
4 years, 5 months ago (2016-06-27 22:24:36 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2100353002/60001
4 years, 5 months ago (2016-06-27 22:26:01 UTC) #11
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/236308)
4 years, 5 months ago (2016-06-27 23:28:57 UTC) #13
jaydasika
https://codereview.chromium.org/2100353002/diff/60001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2100353002/diff/60001/cc/trees/layer_tree_impl.cc#newcode114 cc/trees/layer_tree_impl.cc:114: DCHECK_EQ(!root_layer_, LayerListIsEmpty()); This line is no longer valid. root_layer_ ...
4 years, 5 months ago (2016-06-28 17:10:47 UTC) #16
danakj
I've rebased on https://codereview.chromium.org/2106273002/ to help me fix the ASAN leaks. https://codereview.chromium.org/2100353002/diff/60001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): ...
4 years, 5 months ago (2016-06-29 23:06:25 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2100353002/100001
4 years, 5 months ago (2016-06-29 23:07:04 UTC) #20
danakj
https://codereview.chromium.org/2100353002/diff/80001/cc/trees/layer_tree_host_impl_unittest.cc File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2100353002/diff/80001/cc/trees/layer_tree_host_impl_unittest.cc#newcode7644 cc/trees/layer_tree_host_impl_unittest.cc:7644: host_impl_->active_tree()->SetRootLayerFromLayerListForTesting(); This is the magic change that fixes ASAN.
4 years, 5 months ago (2016-06-29 23:49:41 UTC) #21
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-30 00:11:07 UTC) #23
danakj
enne: PTAL
4 years, 5 months ago (2016-07-01 02:11:52 UTC) #24
enne (OOO)
Oh! Sorry, I thought I had already given this an lgtm.
4 years, 5 months ago (2016-07-01 16:57:36 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2100353002/100001
4 years, 5 months ago (2016-07-01 18:54:32 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/97543)
4 years, 5 months ago (2016-07-01 20:07:22 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2100353002/100001
4 years, 5 months ago (2016-07-01 20:11:27 UTC) #31
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 5 months ago (2016-07-01 20:42:14 UTC) #32
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-01 20:42:49 UTC) #33
commit-bot: I haz the power
4 years, 5 months ago (2016-07-01 20:44:35 UTC) #35
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/ee6547a2a5d5543ee3fd30749978734b1fd6b17b
Cr-Commit-Position: refs/heads/master@{#403516}

Powered by Google App Engine
This is Rietveld 408576698