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

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 final+)

Details

(crash signature)

Attachments

(4 attachments)

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.
Testcase also has to be local.
Posted file crash report
Yeah, in particular the imagelib memory-pressure stuff is on the stack in that crash report...
blocking2.0: --- → ?
I hit a crash with the same or a similar signature at some point yesterday, when quitting my Firefox nightly build.
OS: Mac OS X → All
Hardware: x86 → All
No, the Observe on the stack is xpcom-shutdown.  The crash happens well after the memory-pressure event is fired.
> 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.
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.
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.
The crash report is from a 64-bit build.
Is it a build with symbols?  And on which OS?  My "64-bit" Mac builds seem to have 32-bit pointers in vtables....
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.
It's a tinderbox build, so it has breakpad symbols but probably not native symbols.
Posted patch fixSplinter Review
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #474793 - Flags: review?(roc)
Blocks: 276431
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.)
ehsan landed this: http://hg.mozilla.org/mozilla-central/rev/2fb5fb896436
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Why don't we have a test case here?
Flags: in-testsuite?
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.
Crash Signature: [@ nsSVGRenderingObserver::InvalidateViaReferencedElement]
You need to log in before you can comment on or make changes to this bug.