369438 - Crash after removing filter [@ nsSVGUtils::PaintChildWithEffects]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes on reload)

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdddddde9

Thread 0 Crashed:
0   libgklayout.dylib        	0x188b3534 nsSVGUtils::PaintChildWithEffects(nsSVGRenderState*, nsRect*, nsIFrame*) + 682 (nsSVGUtils.cpp:939)
1   libgklayout.dylib        	0x188a67a2 nsSVGOuterSVGFrame::Paint(nsIRenderingContext&, nsRect const&, nsPoint) + 338 (nsSVGOuterSVGFrame.cpp:492)
2   libgklayout.dylib        	0x188a6866 nsDisplaySVG::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 74 (nsSVGOuterSVGFrame.cpp:402)
3   libgklayout.dylib        	0x183be8b5 nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const + 61 (nsDisplayList.cpp:297)
4   libgklayout.dylib        	0x183bfcd5 nsDisplayWrapList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 41 (nsDisplayList.cpp:705)
5   libgklayout.dylib        	0x183c08b6 nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 128 (nsDisplayList.cpp:937)
6   libgklayout.dylib        	0x183be8b5 nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const + 61 (nsDisplayList.cpp:297)
7   libgklayout.dylib        	0x183d7dfe nsLayoutUtils::PaintFrame(nsIRenderingContext*, nsIFrame*, nsRegion const&, unsigned) + 732 (nsLayoutUtils.cpp:815)
8   libgklayout.dylib        	0x183e2149 PresShell::Paint(nsIView*, nsIRenderingContext*, nsRegion const&) + 449 (nsPresShell.cpp:5125)
9   libgklayout.dylib        	0x1878b7ad nsViewManager::RenderViews(nsView*, nsIRenderingContext&, nsRegion const&, nsIDrawingSurface*) + 217 (nsViewManager.cpp:816)
...


A debug build on Intel Mac crashes reliably.  An opt build on Windows XP also crashes, but less reliably.

Comment 1

[Jesse Ruderman]

Attachment: testcase 2 (crashes on load)

This testcase does more DOM manipulation and causes a crash immediately rather than on reload.  It crashes in a different place: [@ nsSVGFilterProperty::DoUpdate], also deref'ing 0xddddd*.

Comment 2

[chris hofmann]

tor, any ideas on a fix?

Comment 3

[tor]

I know roughly what's happening, but have been busy this last week tracking down large SVG regressions caused by the reflow and incremental xml landings.

Comment 4

[tor]

Attachment: observe the document, instead of the filter element

Comment 5

[chris hofmann]

jonas, can you review?

Comment 6

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

Comment on attachment 255151 [details] [diff] [review]
observe the document, instead of the filter element

You can still keep this as an nsIMutationObserver. nsIDocumentObserver is only needed if you need the extra notifications defined there which doesn't seem to be the case here?

I.e. you can still call AddMutationObserver on a document

Comment 7

[tor]

Attachment: back to mutation observer

Comment 8

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

Comment on attachment 256920 [details] [diff] [review]
back to mutation observer

> nsSVGFilterProperty::nsSVGFilterProperty(nsISVGFilterFrame *aFilter,
>                                          nsIFrame *aFrame)
>   : mFilter(aFilter), mFrame(aFrame)
> {
>   mFilterRect = mFilter->GetInvalidationRegion(mFrame);
> 
>   nsIFrame *filter = nsnull;
>   CallQueryInterface(mFilter, &filter);
> 
>-  nsCOMPtr<nsIContent> filterContent = filter->GetContent();
>-  mObservedFilter = do_GetWeakReference(filterContent);
>+  mObservedFilter = filter->GetContent();
> 
>-  filterContent->AddMutationObserver(this);
>+  nsCOMPtr<nsIDocument> doc = mObservedFilter->GetOwnerDoc();
>+  if (doc) {
>+    doc->AddMutationObserver(this);
>+  }

You probably want to call GetCurrentDoc() here. Otherwise if mObservedFilter isn't a descendant of the document already by the time you get here you'll never get any notifications about it.

That said, what will happen if mObservedFilter is not in a document here? Can that ever happen? If so you probably need code to bail somehow. If that shouldn't happen you can remove the |if| and replace it with an assertion.



> nsSVGFilterProperty::RemoveMutationObserver()
> {
>-  nsCOMPtr<nsIContent> filter = do_QueryReferent(mObservedFilter);
>-  if (filter)
>-    filter->RemoveMutationObserver(this);
>+  nsCOMPtr<nsIDocument> doc = mObservedFilter->GetOwnerDoc();
>+  if (doc) {
>+    doc->RemoveMutationObserver(this);
>+  }
> }

Might be good to keep this one as GetOwnerDoc though so that you for sure remove the observer when the node is moved out of the tree (though I think GetCurrentDoc is safe to use too).

>+nsSVGFilterProperty::MaybeDoUpdate(nsIContent *aContent)
> {
>+  if (aContent != mObservedFilter &&
>+      !nsContentUtils::ContentIsDescendantOf(aContent, mObservedFilter)) {
>+    return;
>+  }

No need for the initial test. ContentIsDescendantOf will return true if the nodes are the same.

> nsSVGFilterProperty::ContentRemoved(nsIDocument *aDocument,
...
>+  if (aChild == mObservedFilter ||
>+      nsContentUtils::ContentIsDescendantOf(mObservedFilter, aChild)) {

Same thing here.

>+    nsSVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(mFrame);
>+    if (outerSVGFrame)
>+      outerSVGFrame->InvalidateRect(mFilterRect);
>+
>+    mFrame->RemoveStateBits(NS_STATE_SVG_FILTERED);
>+    mFrame->DeleteProperty(nsGkAtoms::filter);
>+    RemoveMutationObserver();
>+  } else {
>+    MaybeDoUpdate(aChild);

If you pass aContainer here you'll save yourself a couple of cycles.

r=me with that fixed or explained (in particular the first comment).

Comment 9

[tor]

(In reply to comment #8)
> > nsSVGFilterProperty::nsSVGFilterProperty(nsISVGFilterFrame *aFilter,
> >                                          nsIFrame *aFrame)
...
> >+  nsCOMPtr<nsIDocument> doc = mObservedFilter->GetOwnerDoc();
> >+  if (doc) {
> >+    doc->AddMutationObserver(this);
> >+  }
> 
> That said, what will happen if mObservedFilter is not in a document here? Can
> that ever happen? If so you probably need code to bail somehow. If that
> shouldn't happen you can remove the |if| and replace it with an assertion.

I think it should always be in a document - the filter being observed is found by looking up an uri (currently only looking for the uri in same document [nsSVGContentUtils::GetReferencedElement).  If I'm overlooking something related to XBL please let me know.

Comment 10

[tor]

Attachment: adjust per comments

Comment 11

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

Comment on attachment 256945 [details] [diff] [review]
adjust per comments

In XBL2 even things that's not in the content can be returned from a URI lookup. But we can deal with that at that point as I'm sure much will have to be changed then.

Comment 12

[chris hofmann]

roc, can you sr so we can get this knocked out?

Comment 13

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

Isn't this going to be terribly inefficient?

Can you describe what the problem with the old ownership model was and what the new model is?

Comment 14

[tor]

It's not really an ownership problem but rather a lack of appropriate messages to the mutation observer.  Observing an element, you aren't informed either when it or an ancestor is removed from the tree.

One way of getting around this is to observe the entire document.  Another way might be to try finding the appropriate element/frame each time we need to do something related to the filter (painting or a modification notification).  That would trade off one inefficiency for another.

Comment 15

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

How about adding the ability to detect tree unbinding by mutation observers? That seems generally useful and probably not hard to implement, to me. Jonas?

Comment 16

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

I commented on this in some other bug but I don't remember which now..

We could add such a notification. But what you probably want here is a notification of when the element associated with a particular ID changes, no?

If so we could add once bug 75435 is fixed.

Comment 17

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

I think that's a different issue. I think we want both.

Comment 18

[tor]

Attachment: fix approach using functionality from bug 373089

Comment 19

[tor]

Attachment: update for changed API

Comment 20

[tor]

Checked in.

Comment 21

[Daniel Veditz [:dveditz]]

filters are new in 1.9, right?

Comment 22

[tor]

(In reply to comment #21)
> filters are new in 1.9, right?

Correct, filter code is only in the trunk.

Comment 23

[Jesse Ruderman]

Crashtest checked in.