Last Comment Bug 775697 - Cancel layer transactions that haven't ended after nsSVGFilterFrame::PaintFilteredFrame calls
: Cancel layer transactions that haven't ended after nsSVGFilterFrame::PaintFil...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 614732
  Show dependency treegraph
 
Reported: 2012-07-19 12:31 PDT by Jonathan Watt [:jwatt]
Modified: 2012-07-20 06:42 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.93 KB, patch)
2012-07-19 12:33 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (3.07 KB, patch)
2012-07-19 13:06 PDT, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2012-07-19 12:31:36 PDT
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] 2012-07-19 12:33:33 PDT
Created attachment 643975 [details] [diff] [review]
patch
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-19 12:55:22 PDT
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] 2012-07-19 13:06:42 PDT
Created attachment 643985 [details] [diff] [review]
patch
Comment 5 Ed Morley [:emorley] 2012-07-20 06:42:00 PDT
https://hg.mozilla.org/mozilla-central/rev/a52f4d713da9

Note You need to log in before you can comment on or make changes to this bug.