Crash in [@ mozilla::dom::CompareSimpleTextTrackEvents::LessThan]
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr60 | --- | unaffected |
| firefox66 | --- | unaffected |
| firefox67 | blocking | fixed |
| firefox68 | + | fixed |
People
(Reporter: dholbert, Assigned: alwu)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(4 files)
This bug is for crash report bp-4c1631e7-6b4d-492b-8803-262c00190320.
Top 10 frames of crashing thread:
0 libxul.so mozilla::dom::CompareSimpleTextTrackEvents::LessThan const xpcom/ds/nsTArray.h:490
1 libxul.so bool mozilla::BinarySearchIf<RefPtr<mozilla::dom::SimpleTextTrackEvent> const*, unsigned long nsTArray_Impl<RefPtr<mozilla::dom::SimpleTextTrackEvent>, nsTArrayInfallibleAllocator>::IndexOfFirstElementGt<mozilla::dom::SimpleTextTrackEvent*&, mozilla::dom::CompareSimpleTextTrackEvents> xpcom/ds/nsTArray.h:820
2 libxul.so RefPtr<mozilla::dom::SimpleTextTrackEvent>* nsTArray_Impl<RefPtr<mozilla::dom::SimpleTextTrackEvent>, nsTArrayInfallibleAllocator>::InsertElementSorted<mozilla::dom::SimpleTextTrackEvent*&, mozilla::dom::CompareSimpleTextTrackEvents, nsTArrayInfallibleAllocator> xpcom/ds/nsTArray.h:1512
3 libxul.so mozilla::dom::TextTrackManager::TimeMarchesOn dom/html/TextTrackManager.cpp:775
4 libxul.so mozilla::dom::TextTrackManager::NotifyCueRemoved dom/html/TextTrackManager.cpp:297
5 libxul.so mozilla::dom::TextTrack::SetMode dom/html/HTMLMediaElement.h:650
6 libxul.so mozilla::dom::TextTrack_Binding::set_mode dom/bindings/TextTrackBinding.cpp:302
7 libxul.so bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy> dom/bindings/BindingUtils.cpp:3097
8 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:442
9 libxul.so js::CallSetter js/src/vm/Interpreter.cpp:589
I'm pretty sure this was a crash in a new tab that was opened with this URL:
https://twitter.com/SkyandTelescope/status/1108463880408387584
...though I can't trigger any more crashes with that URL, after the one visit that did crash. (i.e. this was a one-off for me).
This has the same signature as bug 1533909, but that bug was closed as fixed a week ago, and my crash that I'm reporting here was with today's Nightly (20190320112939)
|
Reporter | |
Comment 1•5 years ago
•
|
||
alwu, maybe you can take a look?
If I click on the crash signature at the top of this bug report, I see quite a few crash reports since bug 1533909 landed (including 3 crash reports from recent Nightly which were submitted today, of which I am one). So it looks like there are other ways of triggering this crash, which bug 1533909's patches didn't fully address.
I can reliable hit what looks like this crash by scrolling down on this URL: https://twitter.com/hashtag/pwn2own?src=hash&lang=en
I tested this in an asan debug build, and I noticed that I was hitting this assert: https://searchfox.org/mozilla-central/source/dom/html/TextTrackManager.cpp#537.
Here's a stack trace from the assertion being triggered, if it helps at all. Probably redundant since you have a crash already, but just in case.
(BTW to trigger this, I had to click play on a video, and then scroll the video off screen, so I'm guessing this has something to do with the video being removed? Im assuming twitter delete the dom node once you scroll past it a certain distance.)
|
Assignee | |
Comment 6•5 years ago
|
||
Sure, I'll take a look at this.
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
|
||
Comment 8•5 years ago
|
||
Easy to repro for me on twitter here -
https://twitter.com/hashtag/Pwn2Own?src=hash
just load the page and slowly scroll down. Windows 7, 64-bit nightly.
|
|
||
Comment 9•5 years ago
|
||
(In reply to Paul Theriault [:pauljt] from comment #2)
I can reliable hit what looks like this crash by scrolling down on this URL:
https://twitter.com/hashtag/pwn2own?src=hash&lang=en
:) we're all hanging out on the same twitter page today.
|
|
Assignee | |
Comment 10•5 years ago
|
||
|
|
Assignee | |
Comment 11•5 years ago
|
||
According to the spec [1], current cues and other cues should only contain cues from hidden or showing text tracks.
In this patch, text track would be responsible to add current cues and other cues to the cues list by calling GetCurrentCuesAndOtherCues().
If the text track is disable, then it won't add any cues to the cues list.
In addition, in order to reduce the size of other cues (as actually we don't need to process all cues in the other cues), we use the time interval to only get the cues which are overlapping with the time interval.
[1] https://html.spec.whatwg.org/multipage/media.html#time-marches-on
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 12•5 years ago
|
||
Cue might have negative length because user can set cue's end time via TextTrackCue's API and the spec doesn't have strong restriction that the end time should be equal or larger than the start time of the cue.
As the negative length cue won't be displayed, we have no need to add it to the other cues.
[1] https://html.spec.whatwg.org/multipage/media.html#dom-texttrackcue-endtime
|
||
Comment 13•5 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/13329efc586a part1 : add debug logs. r=jya https://hg.mozilla.org/integration/autoland/rev/b15b634fa407 part2 : let track track handle adding `current cue` and `other cue` if it's not disable. r=jya https://hg.mozilla.org/integration/autoland/rev/c49d99af4cad part3 : do not collect negative length cue for other cues. r=jya
|
||
Comment 14•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/13329efc586a
https://hg.mozilla.org/mozilla-central/rev/b15b634fa407
https://hg.mozilla.org/mozilla-central/rev/c49d99af4cad
Hi Alastor, since 67 is marked as affected, should we consider uplifting this to Beta67?
|
|
Assignee | |
Comment 16•5 years ago
|
||
Comment on attachment 9052790 [details]
Bug 1537554 - part1 : add debug logs.
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: None
- User impact if declined: the page might crash if the site is using webvtt and changes the text track's mode
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This is internal algorithm changed, we filter out some cues which should not be in the cue list because they're from the disable track. The cue from disabled track didn't be rendered on the screen, so it won't take a visual impact to users.
- String changes made/needed: none
- Do you want to request approval of these patches as well?: on, on
|
|
||
Comment 17•5 years ago
|
||
Comment on attachment 9052790 [details]
Bug 1537554 - part1 : add debug logs.
Fix for a 67 A/V top crash, uplift approved for 67 beta 6, thanks!
Note to sheriffs, there are 3 patches to uplift (see comment #14)
|
||
Comment 18•5 years ago
|
||
| bugherder uplift | ||
|
||
Updated•5 years ago
|
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16152 for changes under testing/web-platform/tests
|
||
Comment 20•4 years ago
|
||
Please specify a root cause for this bug. See :tmaity for more information.
|
|
Assignee | |
Updated•4 years ago
|
Description
•