1319486 - SEGV near null [@ GetTextTrackSource]

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a null-deref in mozilla-central rev 0534254e9a40.

==23528==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000bc (pc 0x7fcdffc0df97 bp 0x7ffdd85fd6c0 sp 0x7ffdd85fd670 T0)
    #0 0x7fcdffc0df96 in GetTextTrackSource src/obj-firefox/dist/include/mozilla/dom/TextTrack.h:112:12
    #1 0x7fcdffc0df96 in LessThan src/dom/html/TextTrackManager.cpp:59
    #2 0x7fcdffc0df96 in operator()<RefPtr<mozilla::dom::TextTrack> > src/obj-firefox/dist/include/nsTArray.h:813
    #3 0x7fcdffc0df96 in BinarySearchIf<nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator>, detail::ItemComparatorFirstElementGT<mozilla::dom::TextTrack *&, mozilla::dom::CompareTextTracks> > src/obj-firefox/dist/include/mozilla/BinarySearch.h:80
    #4 0x7fcdffc0df96 in unsigned long nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator>::IndexOfFirstElementGt<mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks>(mozilla::dom::TextTrack*& const&, mozilla::dom::CompareTextTracks const&) const src/obj-firefox/dist/include/nsTArray.h:1462
    #5 0x7fcdffc0dd15 in RefPtr<mozilla::dom::TextTrack>* nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator>::InsertElementSorted<mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks, nsTArrayInfallibleAllocator>(mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks const&) src/obj-firefox/dist/include/nsTArray.h:1481:24
    #6 0x7fcdffbe3f6d in AddTextTrack src/dom/html/TextTrackManager.cpp:543:7
    #7 0x7fcdffbe3f6d in mozilla::dom::TextTrackManager::TimeMarchesOn() src/dom/html/TextTrackManager.cpp:735

Comment 1

[Benjamin Chen [:bechen]]

https://html.spec.whatwg.org/multipage/embedded-content.html#dom-texttrack-addcue

Looks like we miss the step 3.
The testcase add the same TextTrackCue to different TextTracks. When adding the cue to another TextTrack, we need to remove the association from the previous TextTrack.

Comment 2

[Benjamin Chen [:bechen]]

Attachment: Bug 1319486 - Remove the old relation when adding cue to a new TextTrack.

Review commit: https://reviewboard.mozilla.org/r/95008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95008/

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8813561 [details]
Bug 1319486 - Remove the old relation when adding cue to a new TextTrack.

https://reviewboard.mozilla.org/r/95008/#review95356

::: dom/media/TextTrackCue.cpp:78
(Diff revision 1)
>                             ErrorResult& aRv)
>    : DOMEventTargetHelper(aOwnerWindow)
>    , mText(aText)
>    , mStartTime(aStartTime)
>    , mEndTime(aEndTime)
> +  , mTrack(nullptr)

Is this necessary? mTrack is a `RefPtr<TextTrack>` so its constructor should initialize it to null.

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8813561 [details]
Bug 1319486 - Remove the old relation when adding cue to a new TextTrack.

https://reviewboard.mozilla.org/r/95008/#review95356

> Is this necessary? mTrack is a `RefPtr<TextTrack>` so its constructor should initialize it to null.

You are right, I'll rollback the change.

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8813561 [details]
Bug 1319486 - Remove the old relation when adding cue to a new TextTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95008/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/508d7a0ac7a0
Remove the old relation when adding cue to a new TextTrack. r=rillian

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/508d7a0ac7a0

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug1319486.aurora.patch

Approval Request Comment
[Feature/Bug causing the regression]: 833385
[User impact if declined]: crash as the testcase.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes, manual test.
[Needs manual test from QE? If yes, steps to reproduce]: not sure, reproduce step is just "open the testcase"
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple fix.
[String changes made/needed]: no

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8815180 [details] [diff] [review]
bug1319486.aurora.patch

crash fix, take in aurora52

Any reason the test case can't/shouldn't be turned into an automated test somehow?

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3742cb8f4dd6

Comment 11

[Benjamin Chen [:bechen]]

(In reply to Julien Cristau [:jcristau] from comment #9)
> Comment on attachment 8815180 [details] [diff] [review]
> bug1319486.aurora.patch
> 
> crash fix, take in aurora52
> 
> Any reason the test case can't/shouldn't be turned into an automated test
> somehow?

I'll add the testcase into crashtest later.

Comment 12

[Benjamin Chen [:bechen]]

Attachment: bug1319486crashtest.patch

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8815989 [details] [diff] [review]
bug1319486crashtest.patch

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

::: dom/media/test/crashtests/1319486.html
@@ +19,5 @@
> +  tt2.removeCue(cue);
> +  tt1.addCue(new VTTCue(0.7, 2, ""));
> +});
> +</script>
> +<video id=v1></video>

These should go before the <scritp> tag.

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug1319486crashtest.patch

Comment 15

[Benjamin Chen [:bechen]]

Please help to checkin the bug1319486crashtest.patch, thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5006c51caf6
Add crashtest. r=jwwang

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8d91cc43916

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c5006c51caf6