775697 - Cancel layer transactions that haven't ended after nsSVGFilterFrame::PaintFilteredFrame calls

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

After Matt's patch http://hg.mozilla.org/mozilla-central/rev/8a93aee3c7be from bug 539356 to make SVG effects painting to use a LayerManager transaction, the patches in bug 614732 to switch us to using display lists for SVG made us start hitting the "Died during transaction?" assertion in BasicLayerManager::~BasicLayerManager: http://hg.mozilla.org/mozilla-central/annotate/01929e390ba5/gfx/layers/basic/BasicLayerManager.cpp#l124

The problem is that the patches in 614732 change things so that we take the nsSVGIntegrationUtils code paths for painting SVG filters, and after Matt's patch nsSVGIntegrationUtils::PaintFramesWithEffects calls nsSVGFilterFrame::PaintFilteredFrame and assumes that the paint callback that it passes in will be called and call EndTransaction here:

http://hg.mozilla.org/mozilla-central/annotate/01929e390ba5/layout/svg/base/src/nsSVGIntegrationUtils.cpp#l360

However, nsSVGFilterFrame::PaintFilteredFrame itself may return early if the nsAutoFilterInstance doesn't create an nsFilterInstance (that can happen for multiple valid reasons) or the nsSVGFilterInstance::Render() may not bother to call the callback if it's not needed (e.g. if none of the source image is required to paint the filter).

Roc suggests that the best way to handle this is simply to check if the transaction has ended after the nsSVGFilterFrame::PaintFilteredFrame call in nsSVGIntegrationUtils::PaintFramesWithEffects, and if not cancel/end it.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 643975 [details] [diff] [review]
patch

Review of attachment 643975 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +299,5 @@
>    virtual void EndTransaction(DrawThebesLayerCallback aCallback,
>                                void* aCallbackData,
>                                EndTransactionFlags aFlags = END_DEFAULT) = 0;
>  
> +  virtual void AbortTransaction() {}

We don't need this on all LayerManagers, since we only call it on BasicLayerManager.

::: gfx/layers/basic/BasicLayers.h
@@ +93,5 @@
>    virtual bool EndEmptyTransaction();
>    virtual void EndTransaction(DrawThebesLayerCallback aCallback,
>                                void* aCallbackData,
>                                EndTransactionFlags aFlags = END_DEFAULT);
> +  virtual void AbortTransaction();

this doesn't need to be virtual

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a52f4d713da9

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a52f4d713da9