610796 - Occasional shutdown crash in nsNodeUtils::LastRelease when closing page with SVG-as-an-image in list-style-image

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Load URL in a debug, libxul-enabled build
 2. Shift+reload
 3. Close Firefox right after the reload completes

ACTUAL RESULTS: Some of the time, you'll crash in nsNodeUtils::LastRelease, inside the NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS macro that gets invoked there.  The node in question that's going away is the root SVG node in a SVG image's helper document.

From attempting to reduce a testcase, it looks like this happens only/primarily for SVG images used as "list-style-image", but I'm not 100% sure of that.

After some debugging, I've found that the crash is caused by the following sequence of events:
 - We set up a SVGRootRenderingObserver (defined in VectorImage.cpp) to observe the root of our helper SVG document.

 - Browser is closed --> XPCOM shutdown, which includes these steps:
   (A) Our SVGDocumentWrapper severs its ties to its helper SVG document, in its xpcom-shutdown-observer method.

   (B) We tear down the outer document, which includes destroying our VectorImage, its SVGRootRenderingObserver, and its SVGDocumentWrapper.

     (B.i) The SVGRootRenderingObserver's destructor calls StopListening, which is supposed to tear down its observer relationship to the SVG root element.  HOWEVER, this uses on SVGRootRenderingObserver::GetTarget() to look up the root element, and that fails (returns null) because SVGDocumentWrapper dropped its reference to our helper SVG document in "(A)" above.  So we are unable to drop our observer relationship.

   (C) We tear down the helper document, and when we get to its root SVG node, we call LastRelease to notify its observers that it's going away.  However, its only observer -- the SVGRootRenderingObserver -- was already deleted (in B above).  So we crash.

Marking security-sensitive & [sg:critical?] initially, since this ends up calling a virtual function (NodeWillBeDestroyed) on a deleted pointer.  (Probably not exploitable since the deletion happens during XPCOM shutdown, and at that point the attacker doesn't have much opportunity to set up memory to his liking. But attackers are sneaky, so I'm hiding this to be on the safe side.)

Comment 1

[Daniel Holbert [:dholbert]]

The simplest fix for this would probably be to drop any SVGRootRenderingObservers from our SVG root element in part (A) above.

The goal of that step is to sever all ties between imagelib (VectorImage & SVGDocumentWrapper) and our helper document, and we're getting in trouble because we're potentially leaving this one bit of linkage around.

Comment 2

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

Sounds good.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix

This does what I suggested in Comment 1.  It just adds three methods, the first of which gets called at XPCOM-shutdown-time in the SVGDocumentWrapper::Observe method, on our helper-document's root SVG element.

That first method is:
 - nsSVGEffects::RemoveAllRenderingObservers() -- given an element, this looks up its rendering-observer list and calls...
 - nsSVGRenderingObserverList::RemoveAll() -- this clears the rendering observer list, and also notifies each observer with...
 - nsSVGRenderingObserver::NotifyEvictedFromRenderingObserverList() -- this lets the observer update its "mInObserverList" flag (to stay in a consistent state) and also unregister itself as a mutation-listener.

Comment 4

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

Comment on attachment 490697 [details] [diff] [review]
fix

+      nsSVGEffects::RemoveAllRenderingObservers(svgElem);

{}

+  if (mObservers.Count() == 0)
+    return;
+

We probably don't need this optimization --- just remove it.

Comment 5

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/5a5ead36d721

Comment 6

[Daniel Holbert [:dholbert]]

Turns out this breaks non-libxul builds with:
{
../src/libimglib2_s.a(SVGDocumentWrapper.o): In function `mozilla::imagelib::SVGDocumentWrapper::Observe(nsISupports*, char const*, unsigned short const*)':
/scratch/work/builds/mozilla-central/mozilla-central.10-10-06.16-14/obj/modules/libpr0n/src/../../../../mozilla/modules/libpr0n/src/SVGDocumentWrapper.cpp:305: undefined reference to `nsSVGEffects::RemoveAllRenderingObservers(mozilla::dom::Element*)'
/usr/bin/ld.bfd.real: libimglib2.so: hidden symbol `nsSVGEffects::RemoveAllRenderingObservers(mozilla::dom::Element*)' isn't defined
/usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output
collect2: ld returned 1 exit status
}

Backed out for now:
http://hg.mozilla.org/mozilla-central/rev/696391269159

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: followup to fix non-libxul build issue

This fix just replaces a cross-module call to a static method (which fails in non-libxul builds) with a virtual function call on an object pointer (which succeeds).

I don't know of a cleaner way to fix this.... Let me know if there's anything else I should try.  (I already tried adding THEBES_API and NS_EXPORT as prefixes to the nsSVGEffects::RemoveAllRenderingObservers declaration, since I've seen that fix some other linking issues in non-libxul builds, but that didn't help here.)

I'm intending this as a temporary hack, to be yanked out as soon as we remove the option for non-libxul builds.  (At that point, this followup patch could just be directly reverted.)

Comment 8

[Daniel Holbert [:dholbert]]

Re-landed main patch: http://hg.mozilla.org/mozilla-central/rev/1a816f05746d
and the followup too: http://hg.mozilla.org/mozilla-central/rev/36905ac2cef2

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> Created attachment 490972 [details] [diff] [review]
> followup to fix non-libxul build issue
[...]
> I'm intending this as a temporary hack, to be yanked out as soon as we remove
> the option for non-libxul builds.

Filed bug 612712 on reverting the followup once non-libxul builds are dead.