This fault is easy to reproduce with the Olympic home page but also on a number of others. If you start typing to get a drop down then mouse over the File, Edit, History menu headings then it will fail. This scenario with needsGroup, is2D and clipIsEmpty only appears to occur with the current implementation of Azure/Cairo content rendering on Linux (yet to be landed). Not sure why however the logic is clearly unbalanced in this case. The attached patch needs more work to make the save/restore functionality more robust.

Comment on attachment 649514 [details] [diff] [review] Fix save/restore imbalance Refer to 780792 to see where the bug was introduced.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #1) > Refer to 780792 to see where the bug was introduced. Wrong bug number?

And have you pushed this to Try?

Sorry. See bug 757380. I will push it to try.

Comment on attachment 649514 [details] [diff] [review] Fix save/restore imbalance Review of attachment 649514 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this, in the code preceding this change we are painting to groupTarget even in the clipIsEmpty case, so it feels like we need to do something with groupTarget (i.e., do the PopGroupToSource). But you are quite right that we are unbalanced if we do that. Perhaps if the clip is empty, then all those drawing calls do nothing, in which case, can we avoid the calls? (Can we assert that some how?) ::: gfx/layers/basic/BasicLayerManager.cpp @@ +832,5 @@ > > if (needsGroup) { > bool blitComplete = false; > if (is2D) { > + if (!clipIsEmpty) { Can we hoist the clipIsEmpty check up a level since we do the same check on both branches? Or add it to the if and else checks so we don't introduce another level of nesting? (The logic in this method is getting ridiculously complex, yuck).

Oleg - do you have any input on comment 5?

Try run for 0e2322163407 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0e2322163407 Results (out of 51 total builds): success: 9 failure: 42 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-0e2322163407

Try run for 50d538c2fe49 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=50d538c2fe49 Results (out of 190 total builds): success: 166 warnings: 21 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-50d538c2fe49

I think the if-paths after the painting block should mirror the if-paths before the painting block. Before the painting block we have: if (!is2D && !clipIsEmpty) { untransformedSurface = gfxPlatform::GetPlatform()->CreateOffscreenSurface(gfxIntSize(bounds.width, bounds.height), gfxASurface::CONTENT_COLOR_ALPHA); if (!untransformedSurface) { if (pushedTargetOpaqueRect) { if (aTarget->IsCairo()) { currentSurface->SetOpaqueRect(gfxRect(0, 0, 0, 0)); } else { dt->SetOpaqueRect(IntRect()); } } NS_ASSERTION(needsSaveRestore, "Should always need to restore with 3d transforms!"); aTarget->Restore(); return; } untransformedSurface->SetDeviceOffset(gfxPoint(-bounds.x, -bounds.y)); groupTarget = new gfxContext(untransformedSurface); } else if (needsGroup && !clipIsEmpty) { groupTarget = PushGroupForLayer(aTarget, aLayer, aLayer->GetEffectiveVisibleRegion(), &needsClipToVisibleRegion); } else { groupTarget = aTarget; } So I think after the painting block we should have if (!is2D && !clipIsEmpty) { ... do the 3D stuff that calls Transform3D ... } else if (needsGroup && !clipIsEmpty) { ... do the PopGroupToSourceWithCachedSurface .... } else { ... don't need to do anything since we drew to aTarget directly ... }

Try run for 7cddfb4b6440 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7cddfb4b6440 Results (out of 251 total builds): success: 232 warnings: 16 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-7cddfb4b6440

Comment on attachment 658225 [details] [diff] [review] Fixed the imbalance by refectoring BasicLayerManager::PaintLayer() Review of attachment 658225 [details] [diff] [review]: ----------------------------------------------------------------- I really like this approach. ::: gfx/layers/basic/BasicLayerManager.cpp @@ +100,5 @@ > return nsIntRect(r.X(), r.Y(), r.Width(), r.Height()); > } > > +class PaintContext { > +public: Document this thing @@ +124,5 @@ > + // Applies the effective 2D transform and returns true if it is a 2D > + // transform. If it's a 3D transform then it applies an identity and returns > + // false. > + bool > + Apply2DTransform() Declarations put return value on same line as function name ::: gfx/layers/basic/BasicLayers.h @@ +160,4 @@ > }; > TransactionPhase mPhase; > > + void PaintSelfOrChildren(PaintContext& aPaintContext, gfxContext* aGroupTarget); Document what this does @@ +161,5 @@ > TransactionPhase mPhase; > > + void PaintSelfOrChildren(PaintContext& aPaintContext, gfxContext* aGroupTarget); > + > + void FlushGroup(PaintContext& aPaintContext, bool aNeedsClipToVisibleRegion); And this

Try run for 7cddfb4b6440 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7cddfb4b6440 Results (out of 254 total builds): success: 233 warnings: 18 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-7cddfb4b6440

Added comments and moved the transform restoration into the PaintContext where the transform gets modified. This keeps the setting and restoring of the transform together.

Linux only Try: https://tbpl.mozilla.org/?tree=Try&rev=689fa5064108

Try run for 7cddfb4b6440 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7cddfb4b6440 Results (out of 256 total builds): success: 234 warnings: 19 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-7cddfb4b6440

Try run for 383cc2ea3a77 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=383cc2ea3a77 Results (out of 127 total builds): exception: 23 success: 27 warnings: 41 failure: 36 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-383cc2ea3a77

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #19) > https://tbpl.mozilla.org/?tree=Try&rev=8736ca8c9e1d Green on Try. https://hg.mozilla.org/integration/mozilla-inbound/rev/78f957de5c9d