605264 - Crash [@ strcmp | nsHTMLMediaElement::Observe] with QI to nsIObserver

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

2400	nsresult nsHTMLMediaElement::Observe(nsISupports* aSubject,
2401	                                     const char* aTopic, const PRUnichar* aData)
2402	{
2403	  if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {

aTopic is null.

Comment 1

[Jesse Ruderman]

Breakpad can't handle this crash :(

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is fixed by Bug 605271.

Comment 3

[Jesse Ruderman]

Attachment: crash report (mac, debug build of firefox, breakpad)

Comment 4

[Jesse Ruderman]

Attachment: gdb output

Comment 5

[Jesse Ruderman]

Nominating -- the incomplete stacks for this bug force me to over-ignore when fuzzing, and this won't be fixed by bug 605271 in time for Firefox 4.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Stop script from calling nsHTMLMediaElement::Observe.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 503030 [details] [diff] [review]
Stop script from calling nsHTMLMediaElement::Observe.

This is ugly, but it gets the job done.  Is getting XPConnect at xpcom-shutdown safe?  I doubt we have test code in the tree to exercise this at xpcom-shutdown.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 503030 [details] [diff] [review]
Stop script from calling nsHTMLMediaElement::Observe.

- In nsHTMLMediaElement::Observe():

+  // Since this is exposed to (malicious) JS, verify that we aren't being
+  // called from script.  We only have to worry about being called directly,
+  // so this is safe.
+  nsAXPCNativeCallContext* ncc = nsnull;
+  nsContentUtils::XPConnect()->GetCurrentNativeCallContext(&ncc);
+  if (ncc)
+    return NS_ERROR_FAILURE;

How about doing what nsImageLoadingContent does in cases like this, which is to call nsContentUtils::IsCallerChrome(), which I believe is shutdown safe. I suspect your version is as well, but I can't say with 100% certainty.

r=jst with that.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Made changes suggested by jst and pushed.

http://hg.mozilla.org/mozilla-central/rev/652d012f9dc8