Closed Bug 1138557 Opened 10 years ago Closed 10 years ago

crash in IsMediaSourceURI(nsIURI*)

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed

People

(Reporter: kbrosnan, Assigned: jwwang)

References

Details

(Keywords: crash, topcrash-android-armv7)

Crash Data

Attachments

(1 file, 1 obsolete file)

[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
Presumably mLoadingSrc is null.
Tracking for 39 since this is a topcrash.
investigating...
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
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.
See comment 4 for the root cause.
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)
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)
(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.
(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)
(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)
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.
(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.
Attachment #8571884 - Flags: review?(karlt) → review+
A simple change requires no try log.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Tracking topcrash on 37 and 38. jwwang - Given the simple fix, can you please request uplift before Beta 4 (gtb Mon, Mar 9)?
Flags: needinfo?(jwwang)
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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: