Closed Bug 1319486 Opened 4 years ago Closed 4 years ago

SEGV near null [@ GetTextTrackSource]

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: truber, Assigned: bechen)

References

Details

(Keywords: crash, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file 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
Assignee: nobody → 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 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.
Attachment #8813561 - Flags: review?(giles) → review+
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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/508d7a0ac7a0
Remove the old relation when adding cue to a new TextTrack. r=rillian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/508d7a0ac7a0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8815180 - Flags: approval-mozilla-aurora?
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?
Attachment #8815180 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Attached patch bug1319486crashtest.patch (obsolete) — Splinter Review
Attachment #8815989 - Flags: review?(jwwang)
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.
Attachment #8815989 - Flags: review?(jwwang) → review+
Attachment #8815989 - Attachment is obsolete: true
Attachment #8816009 - Flags: review+
Please help to checkin the bug1319486crashtest.patch, thanks.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.