711866 - Crash when multiple source tag are loading a MP4 file and close the window.

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Mario Gomes]

Attachment: crash.zip

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.12 Safari/535.11

Steps to reproduce:

Reproduce:
1. descompact  the file attached and open crash crash.html
2. wait five segunds.
3. close the window.
4. See the crash 

Stacktrace:
(1740.1504): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00000000 ebx=00000000 ecx=00792344 edx=00792344 esi=03032d40 edi=00000000
eip=53c909ac esp=0026e6b0 ebp=0026e6cc iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010246
xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0xc7:
53c909ac 3b4354          cmp     eax,dword ptr [ebx+54h] ds:0023:00000054=????????
0:000> .exr -1
ExceptionAddress: 53c909ac (xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0x000000c7)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000054
Attempt to read from address 00000054
0:000> .lastevent
Last event: 1740.1504: Access violation - code c0000005 (first chance)
  debugger time: Sun Dec 18 19:09:45.048 2011 (UTC - 2:00)
0:000> k
ChildEBP RetAddr  
0026e6cc 534b3892 xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0xc7 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\html\content\src\nshtmlmediaelement.cpp @ 295]
0026e6e8 532a8c60 xul!nsBaseChannel::OnStartRequest+0x84 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsbasechannel.cpp @ 712]
0026e700 532ae7c2 xul!nsInputStreamPump::OnStateStart+0x2c [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsinputstreampump.cpp @ 445]
0026e714 532b5863 xul!nsInputStreamPump::OnInputStreamReady+0x56 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsinputstreampump.cpp @ 397]
0026e724 5335f3fa xul!nsInputStreamReadyEvent::Run+0x1d [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\io\nsstreamutils.cpp @ 115]
0026e748 534ddd59 xul!nsThread::ProcessNextEvent+0x15a [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\threads\nsthread.cpp @ 637]
0026e78c 534f964a xul!nsThread::GetObserver+0x25 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\threads\nsthread.cpp @ 705]
0026e7a0 534e8cf2 xul!nsCacheService::Shutdown+0xac [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\cache\nscacheservice.cpp @ 1143]
0026e8cc 533f390e xul!nsCacheProfilePrefObserver::Observe+0xaa [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\cache\nscacheservice.cpp @ 386]
0026e928 533f3894 xul!nsObserverList::NotifyObservers+0x6e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\ds\nsobserverlist.cpp @ 130]
0026e940 534cfe95 xul!nsObserverService::NotifyObservers+0x74 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\ds\nsobserverservice.cpp @ 182]
0026e97c 5351719a xul!mozilla::ShutdownXPCOM+0x91 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\build\nsxpcominit.cpp @ 597]
0026e990 5346e64f xul!ScopedXPCOMStartup::~ScopedXPCOMStartup+0x54 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nsapprunner.cpp @ 1084]
0026ed18 00ee1812 xul!XRE_main+0xe5d [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nsapprunner.cpp @ 3578]
0026f7f0 00ee1b72 firefox!wmain+0x812 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nswindowswmain.cpp @ 107]
0026f830 76d6ed6c firefox!__tmainCRTStartup+0x152 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\memory\jemalloc\crtsrc\crtexe.c @ 591]
0026f83c 77a937f5 kernel32!BaseThreadInitThunk+0xe
0026f87c 77a937c8 ntdll!__RtlUserThreadStart+0x70
0026f894 00000000 ntdll!_RtlUserThreadStart+0x1b
0:000> dv /v
0026e6d4            this = 0x03032d40
0026e6d8        aRequest = 0x0300e97c
0026e6dc        aContext = 0x00000000
0026e6c4          status = 0x26e6dc
0026e6c8         channel = class nsCOMPtr<nsIChannel>
0026e6c0         element = class nsRefPtr<nsHTMLMediaElement>
0:000> dt /v element
Local var [AddrFlags 90  AddrOff fffffff4  Reg/Val ebp (8)] @ 0x26e6c0 Type nsRefPtr<nsHTMLMediaElement>
class nsRefPtr<nsHTMLMediaElement>, 24 elements, 0x4 bytes
   <function> nsRefPtr<nsHTMLMediaElement>::assign_with_AddRef     void (
	nsHTMLMediaElement*)+26e6c0
   <function> begin_assignment     void** ( void )+26e6c0
   <function> nsRefPtr<nsHTMLMediaElement>::assign_assuming_AddRef     void (
	nsHTMLMediaElement*)+26e6c0
   +0x000 mRawPtr          : 0x02d251a0 class nsHTMLMediaElement, 185 elements, 0xc8 bytes
   element_type class nsHTMLMediaElement, 185 elements, 0xc8 bytes
   <function> nsRefPtr<nsHTMLMediaElement>::~nsRefPtr<nsHTMLMediaElement>     void ( void )+26e6c0
   <function> nsRefPtr<nsHTMLMediaElement>     void (
	nsCOMPtr_helper*)+26e6c0
   <function> nsRefPtr<nsHTMLMediaElement>::nsRefPtr<nsHTMLMediaElement>     void (
	nsHTMLMediaElement*)+26e6c0
   <function> nsRefPtr<nsHTMLMediaElement>     void (
	nsRefPtr<nsHTMLMediaElement>*)+26e6c0
   <function> nsRefPtr<nsHTMLMediaElement>     void ( void )+26e6c0
   <function> operator=     nsRefPtr<nsHTMLMediaElement>* (
	nsCOMPtr_helper*)+26e6c0
   <function> operator=     nsRefPtr<nsHTMLMediaElement>* (
	nsHTMLMediaElement*)+26e6c0
   <function> operator=     nsRefPtr<nsHTMLMediaElement>* (
	nsRefPtr<nsHTMLMediaElement>*)+26e6c0
   <function> swap     void (
	nsHTMLMediaElement**)+26e6c0
   <function> swap     void (
	nsRefPtr<nsHTMLMediaElement>*)+26e6c0
   <function> forget     already_AddRefed<nsHTMLMediaElement> ( void )+26e6c0
   <function> get     nsHTMLMediaElement* ( void )+26e6c0
   <function> operator class nsHTMLMediaElement *     nsHTMLMediaElement* ( void )+26e6c0
   <function> operator->     nsHTMLMediaElement* ( void )+26e6c0
   <function> get_address     nsRefPtr<nsHTMLMediaElement>* ( void )+26e6c0
   <function> get_address     nsRefPtr<nsHTMLMediaElement>* ( void )+26e6c0
   <function> operator*     nsHTMLMediaElement* ( void )+26e6c0
   <function> StartAssignment     nsHTMLMediaElement** ( void )+26e6c0
   <function> __vecDelDtor     void* (
	unsigned int)+26e6c0
0:000> r
eax=00000000 ebx=00000000 ecx=00792344 edx=00792344 esi=03032d40 edi=00000000
eip=53c909ac esp=0026e6b0 ebp=0026e6cc iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010246
xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0xc7:
53c909ac 3b4354          cmp     eax,dword ptr [ebx+54h] ds:0023:00000054=????????
0:000> u
xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0xc7 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\html\content\src\nshtmlmediaelement.cpp @ 295]:
53c909ac 3b4354          cmp     eax,dword ptr [ebx+54h]
53c909af 59              pop     ecx
53c909b0 895df4          mov     dword ptr [ebp-0Ch],ebx
53c909b3 0f8448ffffff    je      xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0x1c (53c90901)
53c909b9 be02004b80      mov     esi,804B0002h
53c909be 8d45f4          lea     eax,[ebp-0Ch]
53c909c1 50              push    eax
53c909c2 e82b41a7ff      call    xul!nsRefPtr<`anonymous namespace'::SetRequestHeaderRunnable>::~nsRefPtr<`anonymous namespace'::SetRequestHeaderRunnable> (53704af2)
0:000> u
xul!nsHTMLMediaElement::MediaLoadListener::OnStartRequest+0xe2 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\html\content\src\nshtmlmediaelement.cpp @ 299]:
53c909c7 5f              pop     edi
53c909c8 8bc6            mov     eax,esi
53c909ca 5e              pop     esi
53c909cb 5b              pop     ebx
53c909cc c9              leave
53c909cd c20c00          ret     0Ch
xul!mozilla::ipc::DocumentRendererChild::RenderDocument [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\canvas\src\documentrendererchild.cpp @ 77]:
53c909d0 55              push    ebp
53c909d1 8bec            mov     ebp,esp

Comment 1

[Mario Gomes]

Source:
NS_IMETHODIMP nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
{
  nsContentUtils::UnregisterShutdownObserver(this);

  // The element is only needed until we've had a chance to call
  // InitializeDecoderForChannel. So make sure mElement is cleared here.
  nsRefPtr<nsHTMLMediaElement> element;
  element.swap(mElement);

  if (mLoadID != element->GetCurrentLoadID()) {
    // The channel has been cancelled before we had a chance to create
    // a decoder. Abort, don't dispatch an "error" event, as the new load
    // may not be in an error state.
    return NS_BINDING_ABORTED;
  }

Comment 2

[Mario Gomes]

A patch for this is some thing like this:

NS_IMETHODIMP nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
{
  nsContentUtils::UnregisterShutdownObserver(this);

  // The element is only needed until we've had a chance to call
  // InitializeDecoderForChannel. So make sure mElement is cleared here.
  nsRefPtr<nsHTMLMediaElement> element;
  element.swap(mElement);
 
 //Possible patch
 if(element == NULL)return; // If element is NULL, call return to finish the function;
  
  if (mLoadID != element->GetCurrentLoadID()) {
    // The channel has been cancelled before we had a chance to create
    // a decoder. Abort, don't dispatch an "error" event, as the new load
    // may not be in an error state.
    return NS_BINDING_ABORTED;
  }

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Patch is indeed trivial. The MediaLoadListener's shutdown observer sets MediaLoadListener::mElement to null, and we can crash when an already enqueued listener's OnStartRequest runs and null derefs. Guard against that case...

Comment 4

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

Comment on attachment 586312 [details] [diff] [review]
Patch

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

crashtest?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> crashtest?

We only crash if we exit before the media/page has finished loading with this testcase, so don't see how we could make a crashtest for it.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e534250c98b

Comment 7

[Ian Melven :imelven]

Marking fixed for edmorley

https://hg.mozilla.org/mozilla-central/rev/9e534250c98b

Comment 8

[Paul Silaghi, QA [:pauly]]

(In reply to Mario Gomes from comment #0)
> Steps to reproduce:
> 1. descompact  the file attached and open crash crash.html
> 2. wait five segunds.
> 3. close the window.
> 4. See the crash 

Tested on FF 12b3 on Win 7/64, Ubuntu 11.10 and Mac OS X 10.6.
No crashes occur, but the video won't start playing. In Google Chrome the video plays fine. What do you think?

Comment 9

[Mihai Morar, (:MihaiMorar)]

I have tested this issue on FF 12 release and FF 19.b2 on Windows 7 x64 and there are some things that seem weird:

I got no crash following STR from Comment 0, just a very long hang for both builds, which is normal seeing your page source for crash.html. As Paul said in Comment 8 video won't start playing, only maybe if you'll wait for a couple of hours, which is normal too.

This hang happens on Google Chrome and Opera too.

Seeing you page source for crash.html file, I am wondering who is going to call 100k times same source for a simple web page, and in plus a video one. That's why you got Acces Violation and Multiple attempts to read from address, since you close the page. What you tried to do is like carrying 20 people with a single bike. If you tried to make stress, load or performace tests you should had used not so heavy methods because even browsers have a limit.

A reasonable method would had been to call less sources for different devices. 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
(20120420145725)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
(20130116073211)

Comment 10

[u279076]

Thanks MarioMi.

Mario Gomes, can you please confirm this is no longer an issue for you as described?

Comment 11

[Mario Gomes]

Yes, I can confirm.

Comment 12

[u279076]

Thanks Mario.