Closed
Bug 1138557
Opened 10 years ago
Closed 10 years ago
crash in IsMediaSourceURI(nsIURI*)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: kbrosnan, Assigned: jwwang)
References
Details
(Keywords: crash, topcrash-android-armv7)
Crash Data
Attachments
(1 file, 1 obsolete file)
897 bytes,
patch
|
karlt
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[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@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@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@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@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@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@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@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@0x3b9b7a
49 data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@0x3b9b7a
50 base.apk base.apk@0x179af66
51 dalvik-main space (deleted) dalvik-main space (deleted)@0x166e0e
52 data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@0x3b9b7a
53 libart.so libart.so@0x212051
54 data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@0x3b9b7a
55 libc.so libc.so@0x11d7f
56 libc++.so libc++.so@0x49f43
57 data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex data@app@org.mozilla.firefox_beta-1@base.apk@classes.dex@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
Blocks: 868866
Flags: needinfo?(jwwang)
Comment 1•10 years ago
|
||
Presumably mLoadingSrc is null.
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8571782 -
Flags: review?(karlt)
Assignee | ||
Comment 7•10 years ago
|
||
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...
Attachment #8571782 -
Flags: review?(karlt)
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
(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?
Flags: needinfo?(karlt)
Comment 10•10 years ago
|
||
(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.
Flags: needinfo?(karlt)
Assignee | ||
Comment 11•10 years ago
|
||
Add null check.
Attachment #8571782 -
Attachment is obsolete: true
Attachment #8571884 -
Flags: review?(karlt)
(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•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8571884 -
Flags: review?(karlt) → review+
Comment 15•10 years ago
|
||
Keywords: checkin-needed
Comment 16•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 17•10 years ago
|
||
Tracking topcrash on 37 and 38.
jwwang - Given the simple fix, can you please request uplift before Beta 4 (gtb Mon, Mar 9)?
Assignee | ||
Comment 18•10 years ago
|
||
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
Flags: needinfo?(jwwang)
Attachment #8571884 -
Flags: approval-mozilla-beta?
Attachment #8571884 -
Flags: approval-mozilla-aurora?
Comment 19•10 years ago
|
||
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+
Attachment #8571884 -
Flags: approval-mozilla-beta?
Attachment #8571884 -
Flags: approval-mozilla-beta+
Attachment #8571884 -
Flags: approval-mozilla-aurora?
Attachment #8571884 -
Flags: approval-mozilla-aurora+
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•