Crash in nsINodeInfo::NodeInfoManager() called from LookupMediaElementURITable

VERIFIED FIXED in Firefox 12

Status

()

Core
Audio/Video
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

({crash, testcase})

Trunk
mozilla12
x86_64
Windows 7
crash, testcase
Points:
---

Firefox Tracking Flags

(firefox10 unaffected, firefox11 affected, firefox12 verified)

Details

(Whiteboard: [qa!], crash signature)

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
To crash browser: load attached testcase, reload page (press F5).

Stack trace:

xul.dll!nsINodeInfo::NodeInfoManager()  Line 218 + 0x3 bytes	C++
xul.dll!nsINode::NodePrincipal()  Line 693 + 0x12 bytes	C++
xul.dll!nsHTMLMediaElement::LookupMediaElementURITable(nsIURI * aURI)  Line 1367 + 0x8 bytes	C++
xul.dll!nsHTMLMediaElement::LoadResource()  Line 943 + 0x1d bytes	C++
xul.dll!nsHTMLMediaElement::SelectResource()  Line 698 + 0xb bytes	C++
xul.dll!nsSyncSection::Run()  Line 601	C++
xul.dll!nsBaseAppShell::RunSyncSections()  Line 358	C++
xul.dll!nsBaseAppShell::AfterProcessNextEvent(nsIThreadInternal * thr, unsigned int recursionDepth)  Line 369	C++
xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 675	C++
xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, bool mayWait)  Line 245 + 0x17 bytes	C++
xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 110 + 0xe bytes	C++
xul.dll!MessageLoop::RunInternal()  Line 209	C++
xul.dll!MessageLoop::RunHandler()  Line 202	C++
xul.dll!MessageLoop::Run()  Line 176	C++
xul.dll!nsBaseAppShell::Run()  Line 191	C++
xul.dll!nsAppShell::Run()  Line 258 + 0x9 bytes	C++
xul.dll!nsAppStartup::Run()  Line 220 + 0x1c bytes	C++
xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData)  Line 3523 + 0x25 bytes	C++
firefox.exe!do_main(const char * exePath, int argc, char * * argv)  Line 201 + 0x13 bytes	C++
firefox.exe!NS_internal_main(int argc, char * * argv)  Line 287 + 0x14 bytes	C++
firefox.exe!wmain(int argc, wchar_t * * argv)  Line 107 + 0xd bytes	C++
firefox.exe!__tmainCRTStartup()  Line 552 + 0x19 bytes	C
firefox.exe!wmainCRTStartup()  Line 371	C
kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

Looks like elem->NodePrincipal() [1] is returning null.

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1367
(Assignee)

Comment 1

6 years ago
(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #0)
> Looks like elem->NodePrincipal() [1] is returning null.

... because elem's memory has been released... Maybe when the previous document was GC'd?
elem->NodePrincipal() should never return null on trunk, if |elem| is not garbage memory yet.

At what point do we remove eleents from gElementTable?  Sounds like we failed to do that here for an element that was going away?
nsHTMLMediaElement::~nsHTMLMediaElement removes it, but only if mDecoder is not null (an element should only be in the table while mDecoder is not null). We should probably just remove that condition. Although I thought everywhere that sets mDecoder to null also calls RemoveMediaElementFromURITable.

Another possibility is that mLoadingSrc changes while the element is in the hashtable so that we can't find it to remove it. But mLoadingSrc should not change while mDecoder is not null.
(Assignee)

Updated

6 years ago
Assignee: nobody → chris

Comment 4

6 years ago
(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #0)
> To crash browser: load attached testcase, reload page (press F5).
I see no attachment.
Severity: normal → critical
Keywords: crash
(Assignee)

Comment 5

6 years ago
Created attachment 586917 [details]
Testcase

Comment 6

6 years ago
Here are my crash reports:
bp-273843f0-30b7-43dd-a4d8-545f82120109 32-bit build
bp-bfb013e4-4b01-4308-9e44-af3772120109 64-bit build
Crash Signature: [@ nsINode::NodePrincipal() ] [@ google_breakpad::ExceptionHandler::HandlePureVirtualCall()]
Keywords: testcase
(Assignee)

Comment 7

6 years ago
Created attachment 587451 [details] [diff] [review]
Patch

If a media was played before nsHTMLMediaElement::FinishDecoderSetup() run, FinishDecoderSetup() can fail if its call to nsBuiltinDecoder::Play() fails. nsBuiltinDecoder::Play() can fail if the thread limit is exceeded. FinishDecoderSetup() always adds the element to the media element table, but if FinishDecoderSetup() fails, it doesn't remove the element from the table. If a failing call to FinishDecoderSetup() came while trying to initialize a cloned decoder, we'll fall back to creating a non-cloned decoder, and so we'll end up calling FinishDecoderSetup() again then, and add the element to the table twice. The destructor only removes the element from the table once, leaving a pointer to the now freed memory in the table, which causes this crash when we try to use that memory.

This patch changes FinishDecoderSetup() to remove the media element from the table upon failure, and adds assertions to guard against this case.

I don't think we can easily make a crash test for this, my testcase requires reloading the page...
Attachment #587451 - Flags: review?(roc)
Comment on attachment 587451 [details] [diff] [review]
Patch

Review of attachment 587451 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1336,5 @@
> +      count++;
> +    }
> +  }
> +  return count;
> +}

make this #ifdef DEBUG
Attachment #587451 - Flags: review?(roc) → review+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/79eb51bafb9a

We should try to get this on Aurora too, the crash will be occurring there too.
status-firefox10: --- → unaffected
status-firefox11: --- → affected
status-firefox12: --- → affected
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/79eb51bafb9a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

5 years ago
Comment on attachment 587451 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 703379
User impact if declined: Potential crashes, seems low volume though, I'm not aware of it occurring in the wild, only in my testcase - but that doesn't mean it won't occur.
Testing completed (on m-c, etc.): Patch has been on m-c since 2012-01-11
Risk to taking this patch (and alternatives if risky): Low risk, it's a simple fix.
Attachment #587451 - Flags: approval-mozilla-aurora?
Comment on attachment 587451 [details] [diff] [review]
Patch

[Triage Comment]
Crash volume isn't high enough to meet the criteria for aurora. Let's reconsider if this becomes a significant crasher in Beta 11.
Attachment #587451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 587451 [details] [diff] [review]
Patch

Whoops cpearce caught my mistake. That was supposed to be an a-.
Attachment #587451 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-

Updated

5 years ago
status-firefox12: affected → ---
(Assignee)

Updated

5 years ago
status-firefox12: --- → fixed
Whiteboard: [qa+]
No crashes occur. Verified fixed on Firefox 12b3:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Status: RESOLVED → VERIFIED
status-firefox12: fixed → verified
Whiteboard: [qa+] → [qa!]
No longer blocks: 703379
Blocks: 703379
You need to log in before you can comment on or make changes to this bug.