359516 - "ASSERTION: unbalanced Will/DidModify calls" and scary crash involving DOMNodeRemoved and SVG filters [@ nsSVGUtils::RemoveObserver]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Loading the testcase causes:

###!!! ASSERTION: unbalanced Will/DidModify calls: 'mModifyNestCount>0', file /Users/admin/trunk/mozilla/content/svg/content/src/nsSVGValue.cpp, line 92

Reloading or quitting then causes a crash.  The top of the stack looks like:

Cannot access memory at address 0xdddddddd
0   CallQueryInterface<nsISupports, nsISVGValue>(nsISupports*, nsISVGValue**)
1   nsSVGUtils::RemoveObserver
2   FilterPropertyDtor
3   nsPropertyTable::PropertyList::DeletePropertyFor
...

Accessing memory at 0xdddddddd means the code read a pointer from a deleted object and then dereferenced it, so it's likely to be a security hole.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

In an opt buiild, reloading triggers "pure virtual method called", a strange type of crash that doesn't trigger a normal crash dialog.

Comment 3

[Jesse Ruderman]

CCing people from bug 349880 and bug 344749, fixed crash bugs with the same stack signature, [@ nsSVGUtils::RemoveObserver].

Comment 4

[Robert Longson [:longsonr]]

In RemoveChildAt in the following function the existing filter frame is destroyed and a new one created.

nsresult
nsSVGFilterElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
{
  WillModify();
  nsresult rv = nsSVGFilterElementBase::RemoveChildAt(aIndex, aNotify);
  DidModify();

  return rv;
}

Thus WillModify and DidModify are called on different objects and then it all screws up from there. When the new filter frame is destroyed you get no notification and the path geometry frame crashes tries to clean reference the deleted filter frame.

Having said all this, I don't know how to fix it.

Comment 5

[tor]

Attachment: possible approach to fix

Comment 6

[tor]

Attachment: update to tip

Comment 7

[tor]

Attachment: minor update

Replaces hand-wired observer scheme for filters with nsIMutationObserver.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 246852 [details] [diff] [review]
minor update

Please don't call AddRef() and Release() directly, use NS_ADDREF/NS_RELEASE, or even better nsCOMPtrs.

>Index: layout/svg/base/src/nsSVGUtils.cpp

>+class nsSVGFilterProperty : public nsStubMutationObserver {
...
>+  nsCOMPtr<nsIContent> mObservedFilter;
>   nsISVGFilterFrame *mFilter;
>+  nsIFrame *mFrame;  // frame being filtered
>+  nsRect mFilterRect;

Since you're holding a strong reference to mObservedFilter NodeWillBeDestroyed will never be called. And are you sure this won't cause circular references?

with that fixed r/sr=me. Though you may want to get input from someone that knows this code better than I do.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 246852 [details] [diff] [review]
minor update

>Index: layout/svg/base/src/nsSVGFilterFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGFilterFrame.cpp,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 nsSVGFilterFrame.cpp
>--- layout/svg/base/src/nsSVGFilterFrame.cpp	27 Nov 2006 17:30:56 -0000	1.24
>+++ layout/svg/base/src/nsSVGFilterFrame.cpp	28 Nov 2006 20:59:05 -0000
>@@ -48,26 +48,22 @@

...

>    */
>   virtual nsIAtom* GetType() const;
> 
> private:
>@@ -108,21 +93,17 @@ private:
> private:
>   nsCOMPtr<nsIDOMSVGAnimatedEnumeration> mFilterUnits;
>   nsCOMPtr<nsIDOMSVGAnimatedEnumeration> mPrimitiveUnits;
>   nsCOMPtr<nsIDOMSVGAnimatedInteger> mFilterResX;
>   nsCOMPtr<nsIDOMSVGAnimatedInteger> mFilterResY;

These member variables could go now as we are not tracking them couldn't they. We could get the values from the element as and when necessary as we do in nsSVGMaskFrame.

Comment 10

[tor]

(In reply to comment #9)
> >   nsCOMPtr<nsIDOMSVGAnimatedEnumeration> mFilterUnits;
> >   nsCOMPtr<nsIDOMSVGAnimatedEnumeration> mPrimitiveUnits;
> >   nsCOMPtr<nsIDOMSVGAnimatedInteger> mFilterResX;
> >   nsCOMPtr<nsIDOMSVGAnimatedInteger> mFilterResY;
> 
> These member variables could go now as we are not tracking them couldn't they.
> We could get the values from the element as and when necessary as we do in
> nsSVGMaskFrame.

I'd like to do that in a followup bug.

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #10)
> I'd like to do that in a followup bug.
> 

I have no problem with that for what it's worth.

Comment 12

[tor]

(In reply to comment #8)
> (From update of attachment 246852 [details] [diff] [review])
> Please don't call AddRef() and Release() directly, use NS_ADDREF/NS_RELEASE, or
> even better nsCOMPtrs.

Switched for NS_ADDREF/NS_RELEASE - not sure what you meant by the comment of using nsCOMPtrs, as this object is being stored as a nsFrame property, which aren't refcounted.

> >Index: layout/svg/base/src/nsSVGUtils.cpp
> 
> >+class nsSVGFilterProperty : public nsStubMutationObserver {
> ...
> >+  nsCOMPtr<nsIContent> mObservedFilter;
> >   nsISVGFilterFrame *mFilter;
> >+  nsIFrame *mFrame;  // frame being filtered
> >+  nsRect mFilterRect;
> 
> Since you're holding a strong reference to mObservedFilter NodeWillBeDestroyed
> will never be called. And are you sure this won't cause circular references?

Yes, I think that might create a circular ref - switched to a weak ref.

> with that fixed r/sr=me. Though you may want to get input from someone that
> knows this code better than I do.

I plan on getting a second review from someone more familiar with the svg code.

Comment 13

[tor]

Attachment: update to tip, don't addref/release directly, switch to weak ref

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 250507 [details] [diff] [review]
update to tip, don't addref/release directly, switch to weak ref

Does nsSVGFilterProperty not need to listen to CharacterDataChanged? It's called whenever the data in a textnode (or similar) is changed.

Use nsWeakRef rather than nsCOMPtr<nsIWeakReference>.

Is this going to add weak references to a lot of nodes? There are a couple of objects allocated for each weak reference to content nodes so it's not free. But if this is only one per filter or some such it should be fine.

You can use the already existing nsPropertyTable::SupportsDtorFunc function rather than creating a new FilterPropertyDtor. You just have to make sure that you're setting a valid nsISupports pointer. This happens to already be the case, but it'd be nice with an explicit cast to nsISupports before setting the property.

r/sr=sicking with that, though I'm leaving the r field for someone better at svg to look at this.

Comment 15

[tor]

(In reply to comment #14)
> (From update of attachment 250507 [details] [diff] [review])
> Does nsSVGFilterProperty not need to listen to CharacterDataChanged? It's
> called whenever the data in a textnode (or similar) is changed.

No, filters don't depend on character data, just the attributes of the various filter elements.

> Is this going to add weak references to a lot of nodes? There are a couple of
> objects allocated for each weak reference to content nodes so it's not free.
> But if this is only one per filter or some such it should be fine.

It's one weak reference per use of a filter.  Filters aren't heavily uses, and tend to be applied to containers of objects rather than individual primitives, so there won't be too many weakrefs around.

Comment 16

[tor]

Attachment: update to tip, use nsWeakPtr and nsPropertyTable::SupportsDtorFunc

Comment 17

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

How do you protect against nsSVGFilterProperty using a stale/dead mFrame, if the frame is destroyed/reconstructed while the content is still alive? Seems to me that we'll still hold a refcount to the nsSVGFilterProperty (because it's still listening for mutations) so it won't be destroyed.

Also,
+  mFrame->AddStateBits(NS_STATE_SVG_FILTERED);

I think this should be set in nsSVGUtils, because that's where we test for the property. It seems better to keep those operations together.

Comment 18

[tor]

Attachment: update per comments

Moved the RemoveMutationObserver to nsSVGUtils::StyleEffects (need to rename that method one of these days), which is called by all SVG frames' Destroy method.

Also moved the AddStateBits per comment.

Comment 19

[tor]

Checked in.

Comment 20

[Samuel Sidler (old account; do not CC)]

Since a fix for this has already been landed, it probably doesn't need to block 1.9. I tested this in a 1.8 branch debug build and didn't see the assertion.

Comment 21

[Daniel Veditz [:dveditz]]

1.8 branch did not support SVG Filters.

Comment 22

[Jesse Ruderman]

Crashtest checked in.