615423 - Shutdown crash [@ mozilla::imagelib::SVGDocumentWrapper::StartAnimation]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

Comment 1

[Jesse Ruderman]

Attachment: inner.svg (part of testcase)

Comment 2

[Jesse Ruderman]

Attachment: c.svg (part of testcase)

Comment 3

[Jesse Ruderman]

Steps to reproduce:
1. Save inner.svg and c.svg to the same directory.
2. Load c.svg.
3. Quit Firefox.

Result: Crash during shutdown 
[@ mozilla::imagelib::SVGDocumentWrapper::StartAnimation]

Comment 4

[Jesse Ruderman]

bp-bfe4f4b6-6db4-4fa7-844e-83fff2101129 (no other reports with this signature).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix?

Looks like we just need to handle the case where document teardown triggers a spurious (ignorable) StartAnimation call after XPCOM shutdown.

This patch should fix it. (just adding a null-check before the null-deref that's crashing here.)

Comment 6

[Daniel Holbert [:dholbert]]

(Will request review after I verify that this fixes it & write an automated test.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's the fix again.  I verified that it fixes the crash.

It turns out that creating an automated test for this is non-trivial, though... The attached testcase only crashes if you quit Firefox (run mozilla::ShutdownXPCOM) *while* viewing the testcase.  As soon as you navigate to a different page (which the crashtest suite does many times before shutdown), you're safe from crashing.

dbaron says that this might be testable with an xpcshell test, but that this would be a fair amount of work to write.  So, I'm tempted to skip the automated test for this bug, since this is just a null-check to prevent a null-deref at shutdown (which only happens with a semi-crazy fuzzer testcase)...

Comment 8

[Daniel Holbert [:dholbert]]

(btw, new fix is identical to old fix - I just tweaked the comment slightly)

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to comment #7)
> dbaron says that this might be testable with an xpcshell test, but that this
> would be a fair amount of work to write.  So, I'm tempted to skip the automated
> test for this bug, since this is just a null-check

Yeah, don't spend lots of time on that. Let's land this and move on to more pressing things.

Comment 10

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/bc5f97f02556