Crash after removing filter [@ nsSVGUtils::PaintChildWithEffects]

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: jruderman, Assigned: tor)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] 1.9-only, crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
Created attachment 254111 [details]
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.
(Reporter)

Updated

12 years ago
Flags: blocking1.9?
Whiteboard: [sg:critical?]
(Reporter)

Comment 1

12 years ago
Created attachment 254130 [details]
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

12 years ago
tor, any ideas on a fix?
(Assignee)

Comment 3

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

Comment 4

12 years ago
Created attachment 255151 [details] [diff] [review]
observe the document, instead of the filter element
Assignee: general → tor
Status: NEW → ASSIGNED
Attachment #255151 - Flags: review?(jonas)

Comment 5

12 years ago
jonas, can you review?
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
(Assignee)

Comment 7

12 years ago
Created attachment 256920 [details] [diff] [review]
back to mutation observer
Attachment #255151 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #256920 - Flags: review?(jonas)
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).
Attachment #256920 - Flags: review?(jonas) → review+
(Assignee)

Comment 9

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

Comment 10

12 years ago
Created attachment 256945 [details] [diff] [review]
adjust per comments
Attachment #256920 - Attachment is obsolete: true
Attachment #256945 - Flags: review?(jonas)
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.
Attachment #256945 - Flags: review?(jonas) → review+
(Assignee)

Updated

12 years ago
Attachment #256945 - Flags: superreview?(roc)

Comment 12

12 years ago
roc, can you sr so we can get this knocked out?
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?
(Assignee)

Comment 14

12 years ago
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.
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?
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.
I think that's a different issue. I think we want both.
(Assignee)

Comment 18

12 years ago
Created attachment 257755 [details] [diff] [review]
fix approach using functionality from bug 373089
(Assignee)

Comment 19

12 years ago
Created attachment 257845 [details] [diff] [review]
update for changed API
Attachment #257755 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Depends on: 373089
(Assignee)

Updated

12 years ago
Attachment #256945 - Attachment is obsolete: true
Attachment #256945 - Flags: superreview?(roc)
(Assignee)

Updated

12 years ago
Attachment #257845 - Flags: review?(roc)
(Assignee)

Comment 20

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
filters are new in 1.9, right?
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] → [sg:critical?] 1.9-only
(Assignee)

Comment 22

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

Correct, filter code is only in the trunk.
Group: security
Flags: in-testsuite?
(Reporter)

Comment 23

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsSVGUtils::PaintChildWithEffects]
You need to log in before you can comment on or make changes to this bug.