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

RESOLVED FIXED in mozilla17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 643975 [details] [diff] [review]
patch
Attachment #643975 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Blocks: 614732
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
(Assignee)

Comment 3

5 years ago
Created attachment 643985 [details] [diff] [review]
patch
Attachment #643975 - Attachment is obsolete: true
Attachment #643975 - Flags: review?(roc)
Attachment #643985 - Flags: review?(roc)
Attachment #643985 - Flags: review?(roc) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52f4d713da9
Target Milestone: --- → mozilla17

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a52f4d713da9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.