595401 - Crash [@ nsSVGRenderingObserver::InvalidateViaReferencedElement] with background:url, zoom, memory-pressure

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires domFuzzLite extension)

1. Install https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Start Firefox
3. Load the testcase
4. Quit Firefox

Result: Crash [@ nsSVGRenderingObserver::InvalidateViaReferencedElement]

Replacing the memory-pressure call with GC/CC/both makes the bug go away, so this might involve code that specifically watches for memory-pressure events.

Comment 1

[Jesse Ruderman]

Testcase also has to be local.

Comment 2

[Jesse Ruderman]

Attachment: crash report

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, in particular the imagelib memory-pressure stuff is on the stack in that crash report...

Comment 4

[Daniel Holbert [:dholbert]]

I hit a crash with the same or a similar signature at some point yesterday, when quitting my Firefox nightly build.

Comment 5

[Jesse Ruderman]

No, the Observe on the stack is xpcom-shutdown.  The crash happens well after the memory-pressure event is fired.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> No, the Observe on the stack is xpcom-shutdown

Oh, I see.  Well, our list of memory-pressure observers is:

1) nsZipReaderCache (throws away flushable zips)
2) imgLoader (calls MinimizeCaches).
3) DOM code that triggers a cycle collection on memory pressure (amusing, since
   the collection algorithm is memory-hungry).
4) String bundle (throws away parsed data).
5) Device context (compacts the font cache).
6) XBL service (clears an internal cache of JSClasses).
7) Session history (evicts everything from bfcache).

My money is on #2 being involved here.

Comment 7

[Daniel Holbert [:dholbert]]

So just for some background, here's what's supposed to be happening here:
* We have a VectorImage object (which exposes the imgIContainer interface)
 - The VectorImage owns a SVGDocumentWrapper, which encapsulates the helper SVG document
 - The VectorImage also owns an SVGRenderingObserver, which is registered to watch our helper SVG document for changes (so it can invalidate clients of the image)

At XPCOM Shutdown:
* The SVGDocumentWrapper kills off its encapsulated document (because otherwise it sticks around until imagelib shutdown-time, which is long after we expect all of our documents to be cleaned up)
* The document notifies our VectorImage's nsSVGRenderingObserver that it's going away.
* But in this case, it looks like that nsSVGRenderingObserver isn't there anymore.

So I think what's going on is that the VectorImage (and its nsSVGRenderingObserver) is dying before its SVGDocumentWrapper dies.  That can happen in theory, because VectorImage has a nsRefPtr to its SVGDocumentWrapper, so if someone else retains a pointer to the SVGDocumentWrapper, then it'd last longer than the VectorImage.

Right now I think the only way for someone else to own a pointer to our SVGDocumentWrapper is via VectorImage::ExtractFrame (used by moz-image-rect), which generates a second VectorImage that shares our helper-document.  I could definitely see that causing problems with our nsSVGRenderingObserver notifications.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The crash address is 0x98 == 152.  Assuming this is all 32-bit (which I think is a good assumption), 152 == 38 * 4.  And the entry at index 38 (0-based, etc) of the vtable of nsSVGRenderingObserver happens to be the one for DoUpdate().  Which is called by InvalidateViaReferencedElement.

So sounds like our vtable pointer is null.  Which means the nsSVGRenderingObserver is dead.

Comment 9

[Jesse Ruderman]

The crash report is from a 64-bit build.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Is it a build with symbols?  And on which OS?  My "64-bit" Mac builds seem to have 32-bit pointers in vtables....

Comment 11

[Daniel Holbert [:dholbert]]

Hm, so I'd thought this was just a case of "we need to unregister our nsSVGRenderingObserver when our VectorImage dies".  But AFAICT that should already be happening...  The SVGRootRenderingObserver destructor calls "StopListening", which should take us out of the observer list.

(SVGRootRenderingObserver is the concrete implementation of nsSVGRenderingObserver that's probably in play here)

I have to go, but I can look into this more on Monday.

Comment 12

[Jesse Ruderman]

It's a tinderbox build, so it has breakpad symbols but probably not native symbols.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix

Yay - this is a simple one-line fix. (plus a few-line sanity-check NS_ABORT_IF_FALSE)

This is stupid -- the SVGRootRenderingObserver constructor can add us to the observer list, but it wasn't noting that in the "mInObserverList" flag.  As a result, it might never remove itself from that list, which means we can get callbacks even after the SVGRootRenderingObserver is dead. 

With this patch, the SVGRootRenderingObserver destructor's call to "StopListening" will now see a "true" value in |mInObserverList|, and it'll remove us from the observer list.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix for landing (noted review & a=blocking)

Here's the fix for landing, with review & blocking status noted in commit message. (I'm hoping someone can take this as a ridealong, since it's trivial & hence doesn't need its own push.)

Comment 15

[Daniel Holbert [:dholbert]]

ehsan landed this: http://hg.mozilla.org/mozilla-central/rev/2fb5fb896436

Comment 16

[(no longer active)]

Why don't we have a test case here?

Comment 17

[Daniel Holbert [:dholbert]]

Because the only testcase we have so far requires the installation of a custom extension.

It'd probably be possible to come up with a testcase that doesn't require the extension, and I'd like to do that at some point (thanks for ticking the "in-testsuite" flag as a reminder), but for now, my time is better spent fixing other svg-as-image bugs before beta6 hits code-freeze.