Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155123003:
cc: Fix use-after-frees for LayerTreeHostAnimationTest. (Closed)
Descriptioncc: Fix use-after-frees for LayerTreeHostAnimationTest.
This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test
directly by only doing the animations check a single time.
While trying to understand the crash (I still don't), I uncovered some
maybe questionable things in LayerTreeTest, so I tried to clean that up
a bit:
- Do not go through layer_tree_host() from the compositor thread to get
the compositor thread task runner. Instead store it at test startup on
the main thread.
- Stop allowing the task runner provider to be null in tests, just don't
allow tests to call into layer_tree_host() after the test has stopped.
R=enne, vmpstr
BUG=629167
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
Committed: https://crrev.com/833b2e5026669ee6c7167cdff371452ab47cd2dc
Cr-Commit-Position: refs/heads/master@{#406176}
Patch Set 1 #
Total comments: 4
Patch Set 2 : test-task-runner-gone: todo #Patch Set 3 : test-task-runner-gone: =nullptr #
Total comments: 2
Messages
Total messages: 23 (13 generated)
Description was changed from ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. - To do the above reliably on all platforms, delete the LayerTreeHost after setting the member variable to null, since the std::unique_ptr is not required to do this (and doesn't on Windows). R=enne, vmpstr BUG=629167 ========== to ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. - To do the above reliably on all platforms, delete the LayerTreeHost after setting the member variable to null, since the std::unique_ptr is not required to do this (and doesn't on Windows). R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by danakj@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/2155123003/diff/1/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc#... cc/test/layer_tree_test.cc:927: // do this for us (*ahem* MSVC) since it is not required by the standard. Is the concern that something might access layer_tree_host_ in its dtor? Also, FWIW here's what unique_ptr::reset does according to cppreference: ... Given current_ptr, the pointer that was managed by *this, performs the following actions, in this order: - Saves a copy of the current pointer old_ptr = current_ptr - Overwrites the current pointer with the argument current_ptr = ptr - If the old pointer was non-empty, deletes the previously managed object if(old_ptr != nullptr) get_deleter()(old_ptr). I _think_ the code you have here is doing just that (unless I'm missing something obvious)? And if so, is that equivalent to layer_tree_host_ = nullptr? :) (since operator=(nullptr_t) is defined as "Effectively the same as calling reset()"?
https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc#... cc/test/layer_tree_test.cc:728: layer_tree_host_->task_runner_provider()->MainThreadTaskRunner(); Also, this is probably a minor point, but this is creating a scoped_refptr out of a raw pointer, which is something that I'd want to ideally avoid (since it makes transition to shared_ptr harder in the future). I'm not sure whether task_runner_provider() should be returning scoped_refptrs or what, but can you at least leave a TODO(vmpstr) here?
PTAL https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc#... cc/test/layer_tree_test.cc:728: layer_tree_host_->task_runner_provider()->MainThreadTaskRunner(); On 2016/07/18 23:57:26, vmpstr wrote: > Also, this is probably a minor point, but this is creating a scoped_refptr out > of a raw pointer, which is something that I'd want to ideally avoid (since it > makes transition to shared_ptr harder in the future). > > I'm not sure whether task_runner_provider() should be returning scoped_refptrs > or what, but can you at least leave a TODO(vmpstr) here? Ya cc is a mess of raw pointers for task runners all over the place that become scoped_refptrs. I think some of them could stay raw pointers, and with some work more of them could stay that way forever. https://codereview.chromium.org/2155123003/diff/1/cc/test/layer_tree_test.cc#... cc/test/layer_tree_test.cc:927: // do this for us (*ahem* MSVC) since it is not required by the standard. On 2016/07/18 23:26:12, vmpstr wrote: > Is the concern that something might access layer_tree_host_ in its dtor? Yes. The LTH destructor joins the compositor thread which could go here, or could call things that end up in TestHooks and tests could naively get back in here. It's happened at least twice before. > Also, FWIW here's what unique_ptr::reset does according to cppreference: > > ... Given current_ptr, the pointer that was managed by *this, performs the > following actions, in this order: > - Saves a copy of the current pointer old_ptr = current_ptr > - Overwrites the current pointer with the argument current_ptr = ptr > - If the old pointer was non-empty, deletes the previously managed object > if(old_ptr != nullptr) get_deleter()(old_ptr). > > I _think_ the code you have here is doing just that (unless I'm missing > something obvious)? And if so, is that equivalent to layer_tree_host_ = nullptr? > :) (since operator=(nullptr_t) is defined as "Effectively the same as calling > reset()"? Yep you're right, I continue to be amazed by the crash then.
The CQ bit was checked by danakj@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...
oops didn't upload. now it did.
Patchset #3 (id:40001) has been deleted
Description was changed from ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. - To do the above reliably on all platforms, delete the LayerTreeHost after setting the member variable to null, since the std::unique_ptr is not required to do this (and doesn't on Windows). R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by danakj@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...
lgtm with a question https://codereview.chromium.org/2155123003/diff/60001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (left): https://codereview.chromium.org/2155123003/diff/60001/cc/test/layer_tree_test... cc/test/layer_tree_test.cc:971: return remote_client_layer_tree_host_ This says that in a remote test case, remote_client_lth might not exist. The change is saying that it must exist. Is this an intentional change?
https://codereview.chromium.org/2155123003/diff/60001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (left): https://codereview.chromium.org/2155123003/diff/60001/cc/test/layer_tree_test... cc/test/layer_tree_test.cc:971: return remote_client_layer_tree_host_ On 2016/07/19 00:10:50, vmpstr wrote: > This says that in a remote test case, remote_client_lth might not exist. The > change is saying that it must exist. Is this an intentional change? I read it as saying the same thing as below which ways the task runner may exist. The ImplThreadTaskRunner() function would DCHECK the result was not null, now we dcheck inside here instead and only call this when it makes sense.
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== cc: Fix use-after-frees for LayerTreeHostAnimationTest. This fixes the LayerTreeHostAnimationTestAddAnimationAfterAnimating test directly by only doing the animations check a single time. While trying to understand the crash (I still don't), I uncovered some maybe questionable things in LayerTreeTest, so I tried to clean that up a bit: - Do not go through layer_tree_host() from the compositor thread to get the compositor thread task runner. Instead store it at test startup on the main thread. - Stop allowing the task runner provider to be null in tests, just don't allow tests to call into layer_tree_host() after the test has stopped. R=enne, vmpstr BUG=629167 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel Committed: https://crrev.com/833b2e5026669ee6c7167cdff371452ab47cd2dc Cr-Commit-Position: refs/heads/master@{#406176} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/833b2e5026669ee6c7167cdff371452ab47cd2dc Cr-Commit-Position: refs/heads/master@{#406176} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
