571208 - [WebM] Crash when switching from 360p to 720p [@ nestegg_free_packet | PacketQueue::Reset()]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Open the URL for the Prince of Persia video using a nightly build from 6/10/2010
After a few secs switch from 360p to 720p - the video will then start over, and after 3secs or so.. I crashed.  

http://crash-stats.mozilla.com/report/index/83902a1d-b257-4c45-889c-20c232100610

After restarting browser from the crash, I again played the URL, and switched to 720p without issue. 

Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre ID:20100610035756

Win7 x64 AMD Phenom II Quad hd3200 on-board video, 8 gig RAM

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Crashed again, Started in 360p switched to 720p, and this time the video was lagging, not sure why as it played smoothly before. 

Trying to pause the video by clicking on the 'pause' button it seemed unresponsive, so I clicked several times eventually leading to the crash.

Comment 2

[timeless]

Signature	nestegg_free_packet | PacketQueue::Reset()
UUID	83902a1d-b257-4c45-889c-20c232100610
Time 	2010-06-10 05:05:48.888578
Uptime	490
Last Crash	157057 seconds before submission
Product	Firefox
Version	3.7a5pre
Build ID	20100610035756
Branch	1.9.3
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	AuthenticAMD family 16 model 2 stepping 3
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x10
User Comments	playing prince of persia webM 720p
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nestegg_free_packet 	media/libnestegg/src/nestegg.c:1853
1 	xul.dll 	PacketQueue::Reset 	content/media/webm/nsWebMReader.h:90
2 	xul.dll 	nsWebMReader::~nsWebMReader 	content/media/webm/nsWebMReader.cpp:130
3 	xul.dll 	nsWebMReader::`vector deleting destructor' 	
4 	xul.dll 	nsAutoPtr<nsBuiltinDecoderReader>::~nsAutoPtr<nsBuiltinDecoderReader> 	obj-firefox/dist/include/nsAutoPtr.h:104
5 	xul.dll 	nsBuiltinDecoderStateMachine::~nsBuiltinDecoderStateMachine 	content/media/nsBuiltinDecoderStateMachine.cpp:128
6 	xul.dll 	nsBuiltinDecoderStateMachine::`scalar deleting destructor' 	
7 	xul.dll 	nsRunnable::Release 	obj-firefox/xpcom/build/nsThreadUtils.cpp:55
8 	xul.dll 	nsRefPtr<nsAccessNode>::assign_assuming_AddRef 	obj-firefox/dist/include/nsAutoPtr.h:957
9 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	obj-firefox/xpcom/build/nsCOMPtr.cpp:89
10 	xul.dll 	nsBuiltinDecoder::Stop 	content/media/nsBuiltinDecoder.cpp:136
11 	xul.dll 	nsRunnableMethodImpl<void 	obj-firefox/dist/include/nsThreadUtils.h:347
12 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:547
13 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:118

Comment 3

[timeless]

so, this should mean that packet was 0, i tried wandering around but couldn't figure out who dropped the ball, i'm pretty sure nsDeque is innocent (I own it). It should mean that someone pushed a null pointer into the deque, which probably happens as a result of something jim did :)

Comment 4

[nemo]

http://crash-stats.mozilla.com/report/index/bp-99aff675-4ed0-4ea9-9086-e588d2100610

Wasn't switching - crashed while leaving a page.  I had been repeatedly entering and exiting videos, looking for how many were html5 (right clicking and checking for flash - I guess I should have just disabled the plugin).

Comment 5

[[not reading bugmail]]

(In reply to comment #1)
> Crashed again, Started in 360p switched to 720p, and this time the video was
> lagging, not sure why as it played smoothly before. 

Maybe it was dumped from the cache between runs?
 
> Trying to pause the video by clicking on the 'pause' button it seemed
> unresponsive

Seems like bug 566920

Comment 6

[Matthew Gregan [:kinetik]]

The logic here (my code, oops) looks suspicious: http://mxr.mozilla.org/mozilla-central/source/content/media/webm/nsWebMReader.cpp#526

Comment 7

[[not reading bugmail]]

FWIW, I didn't see anyone testing builds from the first batch of checkins yesterday talking about crashes, so its likely it came from the second set of patches last night.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Only push packet back on top of queue when we actually have one.  Don't bother trying to free the packet in a code path we know it's NULL.  Add an assert to the PacketQueue code that would've caught this earlier.

Comment 9

[Matthew Gregan [:kinetik]]

(In reply to comment #7)
> FWIW, I didn't see anyone testing builds from the first batch of checkins
> yesterday talking about crashes, so its likely it came from the second set of
> patches last night.

That's a good data point, thanks.  The suspicious code I just posted a fix for landed with the second batch (bug 568431).

Comment 10

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/eb396dc49f4a