1138557 - crash in IsMediaSourceURI(nsIURI*)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: New crash, topcrash, seems to be cross platform

This bug was filed from the Socorro interface and is 
report bp-9a3f22fe-0a4b-4b8c-be0c-4787b2150228.
=============================================================

0 	libxul.so	IsMediaSourceURI(nsIURI*)	dom/base/nsHostObjectProtocolHandler.h
1 	libxul.so	mozilla::dom::HTMLMediaElement::CheckProgress(bool)	dom/html/HTMLMediaElement.cpp
2 	libxul.so	nsTimerImpl::Fire()	xpcom/threads/nsTimerImpl.cpp
3 	libxul.so	nsTimerEvent::Run()	xpcom/threads/nsTimerImpl.cpp
4 	libxul.so	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp
5 	libxul.so	NS_ProcessNextEvent(nsIThread*, bool)	xpcom/glue/nsThreadUtils.cpp
6 	libxul.so	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)	ipc/glue/MessagePump.cpp
7 	libxul.so	MessageLoop::RunInternal()	ipc/chromium/src/base/message_loop.cc
8 	libxul.so	MessageLoop::Run()	ipc/chromium/src/base/message_loop.cc
9 	libxul.so	nsBaseAppShell::Run()	widget/nsBaseAppShell.cpp
10 	libxul.so	nsAppStartup::Run()	toolkit/components/startup/nsAppStartup.cpp
11 	libxul.so	XREMain::XRE_mainRun()	toolkit/xre/nsAppRunner.cpp
12 	libxul.so	XREMain::XRE_main(int, char**, nsXREAppData const*)	toolkit/xre/nsAppRunner.cpp
13 	libxul.so	XRE_main	toolkit/xre/nsAppRunner.cpp
14 	libxul.so	GeckoStart	toolkit/xre/nsAndroidStartup.cpp
15 	libmozglue.so	Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun	mozglue/android/APKOpen.cpp
16 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x85e54d	
17 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x8f0e	
18 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x1b1fe	
19 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x209e	
20 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x2f705e	
21 	dalvik-allocspace main rosalloc space 1 mark-bitmap 2 (deleted)	dalvik-allocspace main rosalloc space 1 mark-bitmap 2 (deleted)@0x3a806e	
22 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x18e8e	
23 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x209e	
24 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x20be	
25 	dalvik-main space (deleted)	dalvik-main space (deleted)@0xae	
26 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x20de	
27 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x8676e5	
28 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x18e8e	
29 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x2f705e	
30 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x2c1de	
31 	libart.so	libart.so@0xa2ded	
32 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x18e8e	
33 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x2c1de	
34 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x1b0c3e	
35 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x1a82e	
36 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x20fe	
37 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x1907e	
38 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x20de	
39 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x883e69	
40 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x1a82e	
41 		@0x5b240ffe	
42 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x20fe	
43 	libart.so	libart.so@0xa1a57	
44 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x166e0e	
45 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x1a82e	
46 	dalvik-alloc space (deleted)	dalvik-alloc space (deleted)@0x1a82e	
47 	libart.so	libart.so@0x1d738f	
48 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x3b9b7a	
49 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x3b9b7a	
50 	base.apk	base.apk@0x179af66	
51 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x166e0e	
52 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x3b9b7a	
53 	libart.so	libart.so@0x212051	
54 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x3b9b7a	
55 	libc.so	libc.so@0x11d7f	
56 	libc++.so	libc++.so@0x49f43	
57 	data@[e-mail][e-mail]	data@[e-mail][e-mail]@0x3b9b7a	
58 	dalvik-main space (deleted)	dalvik-main space (deleted)@0x166e0e	
59 	base.apk	base.apk@0x179af66	
60 	base.apk	base.apk@0x179af66	
61 	libart.so	libart.so@0x23e89d	
62 	dalvik-allocspace main rosalloc space mark-bitmap 3 (deleted)	dalvik-allocspace main rosalloc space mark-bitmap 3 (deleted)@0x570545	
63 	libart.so	libart.so@0x227a3d	
64 	libart.so	libart.so@0x2edb3e	
65 	libart.so	libart.so@0x2b1eee	
66 	libart.so	libart.so@0x2b17ba	
67 	libart.so	libart.so@0x2b1eee	
68 	libart.so	libart.so@0x2b16da	
69 	libart.so	libart.so@0x2b16ee	
70 	libc.so	libc.so@0x18e17	
71 	DroidSansFallback.ttf	DroidSansFallback.ttf@0x370708	
72 	base.apk	base.apk@0x179af66	
73 	libc.so	libc.so@0x15b8b	
74 	libc.so	libc.so@0x15bab	
75 	libc.so	libc.so@0x15b8b	
76 	libc.so	libc.so@0x15b8b	
77 	libc.so	libc.so@0x13b83	
78 	base.apk	base.apk@0x179af66

Comment 1

[Boris Zbarsky [:bzbarsky]]

Presumably mLoadingSrc is null.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 39 since this is a topcrash.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

investigating...

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

I can't repro the crash. But I find the call flow below could result in the crash where mLoadingSrc is null in HTMLMediaElement::CheckProgress().

1. mLoadingSrc is reset in HTMLMediaElement::DecodeError() without calling StopProgress().
2. QueueLoadFromSourceTask() is called and network state is changed to NETWORK_LOADING. If network status was already NETWORK_LOADING, StopProgress() won't be called since there is no change in network state.
3. It is possible for CheckProgress() to fire before LoadFromSourceChildren() which set mLoadingSrc to a valid URI.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1138557_stop_timer_when_resetting_mLoadingSrc-v1.patch

See comment 4 for the root cause.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8571782 [details] [diff] [review]
1138557_stop_timer_when_resetting_mLoadingSrc-v1.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +2121,5 @@
>    if (mDecoder) {
>      ShutdownDecoder();
>    }
> +
> +  StopProgress();

StopProgress() already checks |mProgressTimer|.

@@ +3313,5 @@
>  
>    if (now - mDataTime >= TimeDuration::FromMilliseconds(STALL_MS)) {
>      DispatchAsyncEvent(NS_LITERAL_STRING("stalled"));
>  
> +    NS_ASSERTION(mLoadingSrc, "mLoadingSrc should be valid when timer started");

Since I am not 100% sure about the root cause, I add an assertion and safety check to make it less fatal. If we don't hit this assertion as time goes by, we can be sure the fix is right and remove this assertion and safety check below.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8571782 [details] [diff] [review]
1138557_stop_timer_when_resetting_mLoadingSrc-v1.patch

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

Found some errors...

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to JW Wang [:jwwang] from comment #4)
> I can't repro the crash. But I find the call flow below could result in the
> crash where mLoadingSrc is null in HTMLMediaElement::CheckProgress().
> 
> 1. mLoadingSrc is reset in HTMLMediaElement::DecodeError() without calling
> StopProgress().
> 2. QueueLoadFromSourceTask() is called and network state is changed to
> NETWORK_LOADING. If network status was already NETWORK_LOADING,
> StopProgress() won't be called since there is no change in network state.
> 3. It is possible for CheckProgress() to fire before
> LoadFromSourceChildren() which set mLoadingSrc to a valid URI.

I looked at that before commenting in bug 868866, and concluded it shouldn't be a problem because RunInStableState should ensure that the runnable is run before the event loop runs timer events.  However, OnProcessNextEvent() looks complicated enough that I don't think we can be sure of that.

We'll probably need to change much of the uri handling to fix Bug 1116382 anyway.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> I looked at that before commenting in bug 868866, and concluded it shouldn't
> be a problem because RunInStableState should ensure that the runnable is run
> before the event loop runs timer events.  However, OnProcessNextEvent()
> looks complicated enough that I don't think we can be sure of that.

I think the same way. If the problem lies in nsIThread or nsITimer, my patch will not work anyway. So I should just add null-check to avoid the crash. Thoughts?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to JW Wang [:jwwang] from comment #9)
> So I should just add null-check to avoid the crash.
> Thoughts?

I think that is fine, thanks.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1138557_add_null_check_mLoadingSrc-v1.patch

Add null check.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> because RunInStableState should ensure that the runnable is run
> before the event loop runs timer events. 
Is that true in case one spins event loop? Say sync XHR.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Olli Pettay [:smaug] from comment #12)
> (In reply to Karl Tomlinson (:karlt) from comment #8)
> > because RunInStableState should ensure that the runnable is run
> > before the event loop runs timer events. 
> Is that true in case one spins event loop? Say sync XHR.

I don't know, but I thought that was meant to be the case, because otherwise anything might happen before the runnable is run.  However I know of at least one situation where it is not - fetching drag data with gtk.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

A simple change requires no try log.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a11c5327652

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a11c5327652

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking topcrash on 37 and 38.

jwwang - Given the simple fix, can you please request uplift before Beta 4 (gtb Mon, Mar 9)?

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8571884 [details] [diff] [review]
1138557_add_null_check_mLoadingSrc-v1.patch

Approval Request Comment
[Feature/regressing bug #]:868866
[User impact if declined]:crash might be experienced
[Describe test coverage new/current, TreeHerder]:works fine on Central for a week
[Risks and why]: low, change is too simple
[String/UUID change made/needed]:none

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8571884 [details] [diff] [review]
1138557_add_null_check_mLoadingSrc-v1.patch

Simple crash fix that has been on m-c for a week. Beta+ Aurora+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/58b004077c10

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbef37fab945