Fixing GraphicsContext state checks to support oilpan

Source/core/html/canvas/CanvasRenderingContext2D.cpp

Index: Source/core/html/canvas/CanvasRenderingContext2D.cpp
diff --git a/Source/core/html/canvas/CanvasRenderingContext2D.cpp b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
index 8456aba1db2b4de549857a809bf166dec4e1618a..1d019b94ba6dbf5646937aefe1220ec66dc967be 100644
--- a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
+++ b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
@@ -102,23 +102,21 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, co
 
 void CanvasRenderingContext2D::unwindStateStack()
 {
-    // Ensure that the state stack in the ImageBuffer's context
-    // is cleared before destruction, to avoid assertions in the
-    // GraphicsContext dtor.
-    if (size_t stackSize = m_stateStack.size()) {

[f(malita), 2014/05/27 20:28:22]

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.

[Justin Novosad, 2014/05/27 22:25:25]

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.

-        if (GraphicsContext* context = canvas()->existingDrawingContext()) {
-            while (--stackSize)
-                context->restore();
-        }
+    if (GraphicsContext* context = canvas()->existingDrawingContext()) {
+        context->unwindStateStack();
     }
 }
 
 CanvasRenderingContext2D::~CanvasRenderingContext2D()
 {
-#if !ENABLE(OILPAN)
+}
+
+void CanvasRenderingContext2D::validateStateStack()
+{
 #if !ASSERT_DISABLED
-    unwindStateStack();
-#endif
+    GraphicsContext* context = canvas()->existingDrawingContext();
+    if (context && !context->contextDisabled())
+        ASSERT(context->saveCount() == m_stateStack.size());
 #endif
 }
 
@@ -226,10 +224,15 @@ void CanvasRenderingContext2D::dispatchContextRestoredEvent(Timer<CanvasRenderin
 
 void CanvasRenderingContext2D::reset()
 {
+    validateStateStack();
     unwindStateStack();
+    // Replace the save() held by HTMLCanvasElement::m_contextStateSaver

[Stephen White, 2014/05/27 18:11:32]

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)?

+    if (GraphicsContext* context = canvas()->existingDrawingContext())
+        context->save();
     m_stateStack.resize(1);
     m_stateStack.first() = adoptPtrWillBeNoop(new State());
     m_path.clear();
+    validateStateStack();
 }
 
 // Important: Several of these properties are also stored in GraphicsContext's
@@ -348,6 +351,7 @@ void CanvasRenderingContext2D::State::fontsNeedUpdate(CSSFontSelector* fontSelec
 
 void CanvasRenderingContext2D::realizeSaves()
 {
+    validateStateStack();
     if (state().m_unrealizedSaveCount) {
         ASSERT(m_stateStack.size() >= 1);
         // Reduce the current state's unrealized count by one now,
@@ -360,13 +364,16 @@ void CanvasRenderingContext2D::realizeSaves()
         // turn necessary to support correct resizing and unwinding of the stack).
         m_stateStack.last()->m_unrealizedSaveCount = 0;
         GraphicsContext* context = drawingContext();
+        ASSERT(context);
         if (context)
             context->save();
+        validateStateStack();
     }
 }
 
 void CanvasRenderingContext2D::restore()
 {
+    validateStateStack();
     if (state().m_unrealizedSaveCount) {
         // We never realized the save, so just record that it was unnecessary.
         --m_stateStack.last()->m_unrealizedSaveCount;
@@ -381,6 +388,7 @@ void CanvasRenderingContext2D::restore()
     GraphicsContext* c = drawingContext();
     if (c)
         c->restore();
+    validateStateStack();
 }
 
 CanvasStyle* CanvasRenderingContext2D::strokeStyle() const
@@ -1240,6 +1248,7 @@ void CanvasRenderingContext2D::clearRect(float x, float y, float width, float he
     if (saved)
         context->restore();
 
+    validateStateStack();
     didDraw(dirtyRect);
 }
 
@@ -1535,6 +1544,7 @@ void CanvasRenderingContext2D::drawVideo(HTMLVideoElement* video, FloatRect srcR
     c->translate(-srcRect.x(), -srcRect.y());
     video->paintCurrentFrameInContext(c, IntRect(IntPoint(), IntSize(video->videoWidth(), video->videoHeight())));
     stateSaver.restore();
+    validateStateStack();
 }
 
 void CanvasRenderingContext2D::drawImageFromRect(HTMLImageElement* image,
@@ -2372,7 +2382,7 @@ void CanvasRenderingContext2D::drawFocusRing(const Path& path)
     c->setCompositeOperation(CompositeSourceOver, blink::WebBlendModeNormal);
     c->drawFocusRing(path, focusRingWidth, focusRingOutline, focusRingColor);
     c->restore();
-
+    validateStateStack();
     didDraw(dirtyRect);
 }