New issue
Advanced search Search tips

Issue 664357 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 653631



Sign in to add a comment

FilterOperations in a RenderPassDrawQuad become invalid when ListContainer moves the quad

Project Member Reported by ajuma@chromium.org, Nov 11 2016

Issue description

The implementation of ListContainer::EraseAndInvalidateAllPointers uses std::copy<char> (which I believe is effectively the same as memcpy) to move the contents after the erased element into the empty space created by the removal. RenderPassDrawQuads contain FilterOperations which contains an std::vector, so memcpy-ing a RenderPassDrawQuad means memcpy-ing a vector which I don't think is guaranteed to be safe.

I've run into a case where iterating over the resulting vector crashes on Windows debug (this caused http://crrev.com/2423483003 to get reverted). More specifically, LayerTreeHostImpl::RemoveRenderPasses removes a RenderPassDrawQuad from a quad_list, causing the next RPDQ in the list to get memcpy'd into the space previously occupied by the removed quad (this happens in ListContainerHelper::CharAllocator::Erase). After that, attempting to copy this RPDQ's filters will crash inside std::vector's copy constructor, while it's iterating over the vector (but only on Windows debug).

Discussing with weiliangc@, we concluded that changing RPDQ to have a unique_ptr<FilterOperations> rather than an actual FilterOperations might be the best fix (assuming that it's safe to memcpy a unique_ptr).

The crash being Windows-debug-only seems a bit surprising. jbroman's theory is that the implementation of std::vector there might have a debug-only check that involves a pointer from the vector's heap-allocated storage back into the vector instance.
 

Comment 1 by danakj@chromium.org, Nov 11 2016

DrawQuads are meant for IPC and were supposed to be pointer-free structs of data. FilterOperations violates that I guess by using a vector. Is there a way to go back to the original idea of DrawQuads here instead of making these pointery?

> (assuming that it's safe to memcpy a unique_ptr).

Not if the original unique_ptr remains.

Comment 2 by ajuma@chromium.org, Nov 11 2016

One way to avoid pointers in DrawQuads would be to have a vector of FilterOperations on RenderPass (storing only the non-empty FilterOperations), and store only the index into this vector on the RenderPassDrawQuad. (Actually, the indexing would have to be a bit better than that, since we'd still need to support deleting from the vector. So really we'd need a vector<pair<int, FilterOperations>>.) Does this sound better than adding pointers?

>> (assuming that it's safe to memcpy a unique_ptr).

> Not if the original unique_ptr remains.

In the use case here the original unique_ptr would no longer remain.

Comment 3 by danakj@chromium.org, Nov 11 2016

> One way to avoid pointers in DrawQuads would be to have a vector of
> FilterOperations on RenderPass (storing only the non-empty FilterOperations),
> and store only the index into this vector on the RenderPassDrawQuad. 

You can't reconstruct that on the other side of IPC

Comment 4 by danakj@chromium.org, Nov 11 2016

Cc: enne@chromium.org

Comment 5 by danakj@chromium.org, Nov 11 2016

> RenderPass

I read RenderSurface. That would work!
> have a vector of FilterOperations on RenderPass (storing only the non-empty
> FilterOperations), and store only the index into this vector on the
> RenderPassDrawQuad

Sounds like this is storing FilterOperations on the parent RenderPass? We could possibly just store the FilterOperations on RenderPass itself, and RenderPassDrawQuad already has RenderPassId?
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a26e584527e6781ad9ca7369f8245c99785cda47

commit a26e584527e6781ad9ca7369f8245c99785cda47
Author: ajuma <ajuma@chromium.org>
Date: Thu Dec 15 22:35:01 2016

cc: Move filters from RenderPassDrawQuad to RenderPass

This CL moves filters and background filters from RenderPassDrawQuad to
RenderPass. Quads are stored in a ListContainer, which uses memcpy to
shift over elements when an element in the middle is deleted. Since
FilterOperations contain a vector, they're not safe to memcpy (and
this memcpy triggers crashes with http://crrev.com/2423483003).

BUG= 664357 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2543473004
Cr-Commit-Position: refs/heads/master@{#438938}

[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/cc_param_traits.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/cc_param_traits_macros.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/cc_param_traits_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/quads.mojom
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/quads_struct_traits.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/quads_struct_traits.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/render_pass.mojom
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/render_pass_struct_traits.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/render_pass_struct_traits.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/ipc/struct_traits_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/layers/render_surface_impl.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/layers/render_surface_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/ca_layer_overlay.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/ca_layer_overlay.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/direct_renderer.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/direct_renderer.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/gl_renderer.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/gl_renderer.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/gl_renderer_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/overlay_processor.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/overlay_processor.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/overlay_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/renderer_pixeltest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/software_renderer.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/software_renderer.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/output/software_renderer_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/draw_quad_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/render_pass.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/render_pass.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/render_pass_draw_quad.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/render_pass_draw_quad.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/quads/render_pass_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/surfaces/surface_aggregator.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/surfaces/surface_aggregator.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/test/render_pass_test_utils.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/test/render_pass_test_utils.h
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/test/surface_aggregator_test_helpers.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/test/surface_hittest_test_helpers.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/components/exo/surface.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/content/browser/compositor/mus_browser_compositor_output_surface.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/a26e584527e6781ad9ca7369f8245c99785cda47/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp

Comment 8 by ajuma@chromium.org, Dec 16 2016

Status: Fixed (was: Assigned)

Sign in to add a comment