Closed Bug 943729 Opened 6 years ago Closed 6 years ago

Crash in TimeRanges::TimeRange

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: ggrisco, Assigned: smaug)

References

Details

(Keywords: crash, Whiteboard: [CR 581640])

Attachments

(4 files, 1 obsolete file)

Test Steps:
1. Enable auto answer and enable data connection on phone.
2.  Receive MT calls and MT SMS continuously.
3. After night run seen 3 crashes on device.


[@ mozilla::dom::TimeRanges::TimeRange* nsTArray_Impl<mozilla::dom::TimeRanges::TimeRange, nsTArrayInfallibleAllocator>::AppendElement<mozilla::dom::TimeRanges::TimeRange>(mozilla::dom::TimeRanges::TimeRange const&) | mozilla::dom::TimeRanges::Add(double, double) | mozilla::dom::HTMLMediaElement::SetCurrentTime(double, mozilla::ErrorResult&) | mozilla::dom::HTMLMediaElement::SetCurrentTime(double) ]
Attached file EXTRA file attachment
This crash was seen as a part of our recent testing and suggest that Moz block on it for v1.2 to improve MTBF
blocking-b2g: --- → koi?
Mike,

Please assign the bug.
Flags: needinfo?(mlee)
Johnny, can you find someone in the DOM team to own this?
Component: General → DOM
Flags: needinfo?(mlee)
Keywords: crash
Product: Firefox OS → Core
Johnny, can you find someone in the DOM team to own this?
Flags: needinfo?(jst)
Null mPlayed ?
Assignee: nobody → bugs
Flags: needinfo?(jst)
I think this should be fine. Make sure to not access
mPlayed after Unlink.
(The stack trace hints we have a runnable running even after unlink)
Attachment #8341216 - Flags: review?(giles)
Comment on attachment 8341216 [details] [diff] [review]
mplayed_crash.diff

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

Thanks for the patch. Please address the comment and resubmit.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1323,5 @@
>    if (mCurrentPlayRangeStart != -1.0) {
>      double rangeEndTime = CurrentTime();
>      LOG(PR_LOG_DEBUG, ("%p Adding \'played\' a range : [%f, %f]", this, mCurrentPlayRangeStart, rangeEndTime));
>      // Multiple seek without playing, or seek while playing.
> +    if (mCurrentPlayRangeStart != rangeEndTime && mPlayed) {

I think it would be better to log, throw and return in a separate conditional, like we do for !mDecoder) below. This guards the crash, but it's less obvious why the check is there.

Not being able to update the time ranges on SetCurrentTime is an invalid state. I suppose your diagnosis is that it's an invalid state because we've got a runner in the queue after shutdown, and that's exposing an implementation bug to content. So maybe don't throw, but please do log so we know what's up with the check.
Attachment #8341216 - Flags: review?(giles) → review-
Not after shutdown, but after unlink.
Attached patch v2 (obsolete) — Splinter Review
Attachment #8341312 - Flags: review?(giles)
Attached patch v2.1Splinter Review
This might actually compile :)
Attachment #8341312 - Attachment is obsolete: true
Attachment #8341312 - Flags: review?(giles)
Attachment #8341319 - Flags: review?(giles)
Comment on attachment 8341319 [details] [diff] [review]
v2.1

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

thanks!
Attachment #8341319 - Flags: review?(giles) → review+
blocking-b2g: koi? → koi+
https://hg.mozilla.org/mozilla-central/rev/cfb79089beba
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.