466607 - Shutdown crash due to non-HTML <video> element

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase with <video> (can cause shutdown crash)

Steps to reproduce:
1. Load the testcase.
2. Wait a few hundred milliseconds.
3. Quit Firefox with Cmd+Q.

Result: Crash on shutdown, 50% of the time.  Most of the crashes happen during XPCWrappedNative::FlatJSObjectFinalized (during GC), but sometimes they happen during nsHTMLMediaElement::Freeze instead.  Both types of crashes appear [sg:critical].

The second crash signature makes me suspicious: why is an nsHTMLMediaElement is being created here, given that the "video" element isn't in the HTML namespace?

Comment 2

[Daniel Holbert [:dholbert]]

This happens on Linux as well, though it's a load-time crash rather than a shutdown crash.  (Crash still only happens ~50% of the time, though)

I initially filed bug 467621 for the linux load-time crashes, but then I noticed both bugs' crashes are within nsHTMLMediaElement::Freeze, so it's probably the same root cause.

Crash reports for Linux crashes:
 * In [ nsGenericElement::HasAttributes ]:
http://crash-stats.mozilla.com/report/index/0d42a2da-d995-46ad-9119-05daf2081202
http://crash-stats.mozilla.com/report/index/32d7ade1-86e5-4a77-9e8d-431472081202
http://crash-stats.mozilla.com/report/index/10f891fb-46f2-4cba-b97d-a04182081202

 * In [ @0xaf4b3aa4 ]:
http://crash-stats.mozilla.com/report/index/9796f2b7-522a-4623-b8a4-71e432081202

Comment 3

[Daniel Holbert [:dholbert]]

Just caught the crash in GDB.  It looks like we're calling StopMediaInstance() with aContent pointing to a nsXMLElement, but that function static_casts it to a nsHTMLMediaElement.  And then we lose.

URL: http://tinyurl.com/6m5lpo

Comment 4

[Daniel Holbert [:dholbert]]

So basically, we're assuming that all elements with tag name = "video" are nsHTMLMediaElements, which isn't true in this case.


Specifically, we do this:

 * Call EnumeratePlugins(domDoc, NS_LITERAL_STRING("video"), StopMediaInstance);
 * EnumeratePlugins dutifully looks up all DOM nodes that have tag name = "video", which in this case includes a non-HTML element.  It calls StopMediaInstance on all these elements.
 * StopMediaInstance kills us because it assumes its input to be a nsHTMLMediaElement, which in this case it isn't.

(It looks we could probably hit this bug with <audio>, too, FWIW.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase with <audio> (can cause shutdown crash)

This testcase crashes for me some of the time too, just like the first one.

It just crashed during GC for me -- not during nsHTMLMediaElement::Freeze.  I'm guessing this is because refcounting can get screwed up when we cast a non-HTML-element to be a nsHTMLMediaElement. (which we do here, as mentioned above)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: backtrace of GC crash on <audio> testcase

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch fixes the issue by tweaking StopMediaInstance (and the related StartMediaInstance) to only static_cast if the node is HTML. (as reported by "IsNodeOfType(nsINode::eHTML)")

If that check succeeds, we know it's (a) a HTML element, and (b) it's tag is either "video" or "audio" (otherwise we wouldn't be inside this function), which AFAIK means it's safe to assume it's a nsHTMLMediaElement.

Comment 8

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

We don't need those warnings. Also, I think it would be slightly better to QI to nsIDOMHTMLMediaElement instead of doing the IsNodeOfType check.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v2

With warning removed, & using QI instead of IsNodeOfType.

Comment 10

[Daniel Holbert [:dholbert]]

patch v2 pushed to mozilla-central as changeset d5969f0d3fd9

Comment 11

[Daniel Holbert [:dholbert]]

pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/eb24d49bb082

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204034421

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090204 Minefield/3.2a1pre ID:20090204032912

Comment 13

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/7dcc98765d59