Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293963009:
Fixing GraphicsContext state checks to support oilpan (Closed)
|
Created:
6 years, 7 months ago by Justin Novosad Modified:
6 years, 6 months ago CC:
blink-reviews, jamesr, krit, blink-reviews-html_chromium.org, jbroman, danakj, dglazkov+blink, Rik, Stephen Chennney, aandrey+blink_chromium.org, pdr., rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionFixing GraphicsContext state checks to support oilpan
Debug assertions in the GraphicsContext destructor were
dependent on object destruction order, which made them
incompatible with oilpan. The state validation logic
was refactored to make the state validation checks more
useful.
BUG=370793
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174981
Patch Set 1 #Patch Set 2 : updated test expectations #
Total comments: 11
Patch Set 3 : clarified comments #Patch Set 4 : response to review comments #
Total comments: 4
Patch Set 5 : removed change to test expectations #
Messages
Total messages: 25 (0 generated)
PTAL
LGTM from an Oilpan perspective. Thanks for doing this junov! https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:501: if (is3D()) { Is it possible to get here where m_context has not been created yet? Line 524 below checks for 'if (m_context)' which indicates that it is possible. Is it OK to treat this as a 2D CanvasElement if m_context is 0 here? In other words: is it OK to go through the steps below that sets things up for a 2D CanvasElement and then later create a m_context which is 3D?
https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:501: if (is3D()) { On 2014/05/22 05:53:03, Mads Ager (chromium) wrote: > Is it possible to get here where m_context has not been created yet? Not supposed to. Line 524 > below checks for 'if (m_context)' which indicates that it is possible. Is it OK > to treat this as a 2D CanvasElement if m_context is 0 here? In other words: is > it OK to go through the steps below that sets things up for a 2D CanvasElement > and then later create a m_context which is 3D? That check was added very recently in Blink r173363. I would argue that it is probably not necessary
Still need approval from OWNER
Stephen C. or Florin should probably look at the GraphicsContext changes. To be honest, I get lost in the twisty maze of deferred saves we have going in here, between CanvasRenderingContext2D, GraphicsContext and Skia. https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:229: // Replace the save() held by HTMLCanvasElement::m_contextStateSaver Maybe it's jetlag, but I don't understand this comment, or the two lines of code below it. How was this not causing problems before? It seems like we would previously have had a mismatched number of saved states in GraphicsContext vs CRC2D, after a reset, no? If so, could we add a test for that (if it's possible)?
On 2014/05/27 18:11:31, Stephen White wrote: > Stephen C. or Florin should probably look at the GraphicsContext changes. To be > honest, I get lost in the twisty maze of deferred saves we have going in here, > between CanvasRenderingContext2D, GraphicsContext and Skia. > > https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): > > https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... > Source/core/html/canvas/CanvasRenderingContext2D.cpp:229: // Replace the save() > held by HTMLCanvasElement::m_contextStateSaver > Maybe it's jetlag, but I don't understand this comment, or the two lines of code > below it. The issue is that we just unwond the entire state stack, but not all of the states on the stack were owned by the CanvasRenderingContext2D. The first state push was owned by HTMLCanvasElement::m_contextStateSaver, so we have to put it back. > How was this not causing problems before? It seems like we would > previously have had a mismatched number of saved states in GraphicsContext vs > CRC2D, after a reset, no? > > If so, could we add a test for that (if it's possible)? We have tests already. All those calls to validateStateStack verify that the saveCount is consistent with the state of the GraphicsContext, which is how I managed to isolate this problem with reset. Before, we were only doing checks at destruction time, which was ineffective because of the stack unwinding that happened in the destructor, and the code would just fail silently when the stack underflowed. I think that before this patch it may have been possible to leak state across a reset, but I was unable to construct a layout test to demonstrate that bug.
I meant a layout test that fails 100% of the time without the code change, and passes 100% of the time with it. I understand that that could be difficult to write in this case, though, so LGTM.
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/293963009/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/html/HTMLCanvasElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/html/HTMLCanvasElement.cpp
Hunk #1 FAILED at 97.
Hunk #2 succeeded at 515 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
Source/core/html/HTMLCanvasElement.cpp.rej
Patch: Source/core/html/HTMLCanvasElement.cpp
Index: Source/core/html/HTMLCanvasElement.cpp
diff --git a/Source/core/html/HTMLCanvasElement.cpp
b/Source/core/html/HTMLCanvasElement.cpp
index
88f05be55e4d05be983401dbf35925bde4185a00..466788272dc19f3fde9044ed0db6c295d71232ac
100644
--- a/Source/core/html/HTMLCanvasElement.cpp
+++ b/Source/core/html/HTMLCanvasElement.cpp
@@ -97,7 +97,9 @@ HTMLCanvasElement::~HTMLCanvasElement()
(*it)->canvasDestroyed(this);
#if !ENABLE(OILPAN)
- m_context.clear(); // Ensure this goes away before the ImageBuffer.
+ // Ensure these go away before the ImageBuffer.
+ m_contextStateSaver.clear();
+ m_context.clear();
#endif
}
@@ -514,6 +516,9 @@ void HTMLCanvasElement::createImageBufferInternal()
// See CanvasRenderingContext2D::State::State() for more information.
m_imageBuffer->context()->setMiterLimit(10);
m_imageBuffer->context()->setStrokeThickness(1);
+#if !ASSERT_DISABLED
+ m_imageBuffer->context()->disableDestructionChecks(); // 2D canvas is
allowed to leave context in an unfinalized state.
+#endif
m_contextStateSaver = adoptPtr(new
GraphicsContextStateSaver(*m_imageBuffer->context()));
if (m_context)
https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:520: m_imageBuffer->context()->disableDestructionChecks(); // 2D canvas is allowed to leave context in an unfinalized state. If we have exclusive control of the initialization path, I would prefer this to be a GraphicsContext constructor flag. But it's unclear whether that's feasible. https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:108: if (size_t stackSize = m_stateStack.size()) { Since we're asserting that m_stateStack.size() == context->saveCount() below, doesn't this old approach still work? We could avoid adding GC::unwindStateStack() if it did. https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... File Source/platform/graphics/GraphicsContext.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.cpp:164: restore(); You can probably drive this off m_canvasStateStack (less tricky): while (!m_canvasStateStack.isEmpty()) restore(); ASSERT(!m_paintStateIndex); ASSERT(!m_paintState->saveCount()); https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.h:117: unsigned saveCount() { return m_saveCount; } return m_canvasStateStack.size()? https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.h:521: unsigned m_saveCount; Can we use m_canvasStateStack.size() instead?
https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:520: m_imageBuffer->context()->disableDestructionChecks(); // 2D canvas is allowed to leave context in an unfinalized state. On 2014/05/27 20:28:22, Florin Malita wrote: > If we have exclusive control of the initialization path, I would prefer this to > be a GraphicsContext constructor flag. But it's unclear whether that's feasible. Right no ImageBuffer is not explicitly aware that it is owned by a 2D canvas, but we could still have a construction flag relayed through ImageBuffer. https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left): https://codereview.chromium.org/293963009/diff/20001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:108: if (size_t stackSize = m_stateStack.size()) { On 2014/05/27 20:28:22, Florin Malita wrote: > Since we're asserting that m_stateStack.size() == context->saveCount() below, > doesn't this old approach still work? We could avoid adding > GC::unwindStateStack() if it did. Good point. GC::unwindStateStack was my fist attempt at fixing this bug, but on its own it was not enough to ensure the coherence of the state stack. I will delete it. https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/293963009/diff/20001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.h:521: unsigned m_saveCount; On 2014/05/27 20:28:22, Florin Malita wrote: > Can we use m_canvasStateStack.size() instead? I guess. I am being prematurely concerned about interfering with a hypothetical future optimization that would introduce unrealized saves to m_canvasStateStack. :-)
On 2014/05/27 22:25:25, junov wrote: > > Right now ImageBuffer is not explicitly aware that it is owned by a 2D canvas, > but we could still have a construction flag relayed through ImageBuffer. > On second though, adding a new flags argument to the constructors of ImageBuffer and GraphicsContext for an option that is only relevant for debug builds seems yucky. I think I'd rather leave it like this.
New patch.
DBC https://codereview.chromium.org/293963009/diff/60001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/293963009/diff/60001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:102: m_contextStateSaver.clear(); This should be outside the #if !ENABLE(OILPAN) since it is an own ptr in both builds. A quick question regarding the comment above: what issue can arise if the ImageBuffer is torn down before the CanvasRenderingContext? In the oilpan build the HTMLCanvasElement (and via an OwnPtr ImageBuffer) can be torn down in the same GC round as the CanvasRenderingContext which means their finalization order is not known.
It still bothers me that we have to do this, but GC lgtm.
https://codereview.chromium.org/293963009/diff/60001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/293963009/diff/60001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:231: unwindStateStack(); Just so I understand: there was no bug in reset(), and the existing unwindStateStack() leaves the stack at a count of 1. So this patch now simply re-enables the asserts for OILPAN builds, but disables them for canvas in all cases. Is that correct? https://codereview.chromium.org/293963009/diff/60001/Source/platform/graphics... File Source/platform/graphics/GraphicsContext.cpp (right): https://codereview.chromium.org/293963009/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.cpp:148: #if !ASSERT_DISABLED UberNit: isn't this unnecessary, strictly speaking, since all of the ASSERTS will expand to whitespace in Release builds anyway?
On 2014/05/28 06:17:26, zerny-chromium wrote: > DBC > > https://codereview.chromium.org/293963009/diff/60001/Source/core/html/HTMLCan... > File Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/293963009/diff/60001/Source/core/html/HTMLCan... > Source/core/html/HTMLCanvasElement.cpp:102: m_contextStateSaver.clear(); > This should be outside the #if !ENABLE(OILPAN) since it is an own ptr in both > builds. The clear is going to happen anyways when the OwnPtr destructor. It is just that the non-oilpan build has checks that are sensitive to object destruction order, so we clear explicitly to control the order. > > A quick question regarding the comment above: what issue can arise if the > ImageBuffer is torn down before the CanvasRenderingContext? There is no problem with that. It happens all the time (e.g. when you resize a canvas). The CanvasRenderingContext knows how to deal with it. > > In the oilpan build the HTMLCanvasElement (and via an OwnPtr ImageBuffer) can be > torn down in the same GC round as the CanvasRenderingContext which means their > finalization order is not known. Exactly, which is why the finalization checks in GraphicsContext (owned by ImageBuffer) have to be removed from oilpan builds in the case of 2D canvases. This patch adds alternate debug validations in CanvasRenderingContext2D to compensate.
https://codereview.chromium.org/293963009/diff/60001/Source/platform/graphics... File Source/platform/graphics/GraphicsContext.cpp (right): https://codereview.chromium.org/293963009/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContext.cpp:148: #if !ASSERT_DISABLED On 2014/05/28 13:23:34, Stephen White wrote: > UberNit: isn't this unnecessary, strictly speaking, since all of the ASSERTS > will expand to whitespace in Release builds anyway? You would think. But m_disableDestructionChecks is not defined in release builds.
The CQ bit was checked by junov@chromium.org
The CQ bit was unchecked by junov@chromium.org
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/293963009/80001
Message was sent while issue was closed.
Change committed as 174981 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
