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...
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt]
: Jet Villegas (:jet)
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image Jonathan Watt [:jwatt] 2012-07-19 12:31:36 PDT
After Matt's patch 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:

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:

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 User image Jonathan Watt [:jwatt] 2012-07-19 12:33:33 PDT
Created attachment 643975 [details] [diff] [review]
Comment 2 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-19 12:55:22 PDT
Comment on attachment 643975 [details] [diff] [review]

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 User image Jonathan Watt [:jwatt] 2012-07-19 13:06:42 PDT
Created attachment 643985 [details] [diff] [review]
Comment 5 User image Ed Morley [:emorley] 2012-07-20 06:42:00 PDT

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