Closed Bug 1537554 Opened 8 months ago Closed 8 months ago

Crash in [@ mozilla::dom::CompareSimpleTextTrackEvents::LessThan]

Categories

(Core :: Audio/Video: Playback, defect, P1, critical)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla68
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)

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.

Flags: needinfo?(alwu)

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.)

Sure, I'll take a look at this.

Assignee: nobody → alwu
Flags: needinfo?(alwu)

Also crashes on windows.

OS: Linux → All
Rank: 15
Priority: -- → P2

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.

(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.

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

Priority: P2 → P1

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

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

Hi Alastor, since 67 is marked as affected, should we consider uplifting this to Beta67?

Flags: needinfo?(alwu)

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
Flags: needinfo?(alwu)
Attachment #9052790 - Flags: approval-mozilla-beta?

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)

Attachment #9052790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16152 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.