Closed Bug 1333931 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::CompareTextTracks::LessThan

Categories

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

x86
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 + wontfix
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: bkelly, Assigned: rillian)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-977735b0-f65e-4074-8c5b-e30cb2170125.
=============================================================

Looks like we are crashing when we try to insert a nullptr track or the list contains a nullptr track.

0 	xul.dll 	mozilla::dom::CompareTextTracks::LessThan(mozilla::dom::TextTrack*, mozilla::dom::TextTrack*) 	dom/html/TextTrackManager.cpp:59
1 	xul.dll 	mozilla::BinarySearchIf<nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator>, detail::ItemComparatorFirstElementGT<mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks> >(nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator> const&, unsigned int, unsigned int, detail::ItemComparatorFirstElementGT<mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks> const&, unsigned int*) 	obj-firefox/dist/include/mozilla/BinarySearch.h:80
2 	xul.dll 	nsTArray_Impl<RefPtr<mozilla::dom::TextTrack>, nsTArrayInfallibleAllocator>::InsertElementSorted<mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks, nsTArrayInfallibleAllocator>(mozilla::dom::TextTrack*&, mozilla::dom::CompareTextTracks const&) 	obj-firefox/dist/include/nsTArray.h:1481
3 	xul.dll 	mozilla::dom::TextTrackManager::TimeMarchesOn() 	dom/html/TextTrackManager.cpp:751
4 	xul.dll 	mozilla::dom::HTMLMediaElement::FireTimeUpdate(bool) 	dom/html/HTMLMediaElement.cpp:5597
Can you find someone to look at this?  Its not super frequent, but so far its the top crash for a "dom::" search in FF51 release channel.
Flags: needinfo?(overholt)
Ralph, can you take a look? Looks like you reviewed the code (at least the crashing line :) in bug 882664. Thanks.

http://searchfox.org/mozilla-central/source/dom/html/TextTrackManager.cpp#59
Flags: needinfo?(overholt) → needinfo?(giles)
We can certainly nullcheck in the comparison, but I don't understand yet why we're seeing null track pointers in the first place...
TextTrackCue appears to be cycle collected which can clear its mTrack member.  I think you at least need to nullptr check the track or handle the cycle collection clearing in another way.

Even if this isn't the case, I think we should add the nullptr check and MOZ_DIAGNOSTIC_ASSERT() calls to verify that its non-null where we think it should be.

We hit this crash 200+ times in the last 24 hours.
I guess that could happen if we still have an update task queued when the media element is garbage collected? Seems like that wouldn't be this common. Anyway, here's a patch the add the null check. Benjamin Chen is the expert here, but is away. Hopefully he can correct the analysis if necessary next week.
Assignee: nobody → giles
Flags: needinfo?(giles)
Comment on attachment 8832311 [details]
Bug 1333931 - Handle nullptr TextTrack objects in sorting.

https://reviewboard.mozilla.org/r/108654/#review109766
Attachment #8832311 - Flags: review?(kinetik) → review+
Windows test failures look like an infrastructure issues. Going to try landing.
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/931cbdebf471
Handle nullptr TextTrack objects in sorting. r=kinetik
Crash volume for signature 'mozilla::dom::CompareTextTracks::LessThan':
 - nightly (version 54): 0 crashes from 2017-01-23.
 - aurora  (version 53): 0 crashes from 2017-01-23.
 - beta    (version 52): 0 crashes from 2017-01-23.
 - release (version 51): 576 crashes from 2017-01-16.
 - esr     (version 45): 0 crashes from 2016-08-03.

Crash volume on the last weeks (Week N is from 01-30 to 02-05):
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0
 - aurora        0
 - beta          0
 - release     182       0
 - esr           0       0       0       0       0       0       0

Affected platforms: Windows, Linux

Crash rank on the last 7 days:
           Browser   Content   Plugin
 - nightly
 - aurora
 - beta
 - release #79       #32
 - esr
https://hg.mozilla.org/mozilla-central/rev/931cbdebf471
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Thanks!  We should at least uplift this to FF52 and FF53.
(In reply to Ben Kelly [:bkelly] from comment #12)
> Thanks!  We should at least uplift this to FF52 and FF53.

needinfoing Ralph just to ensure uplifting remains on his radar. And I will echo Ben's thanks for the patch here!
Flags: needinfo?(giles)
(In reply to Ralph Giles (:rillian) | needinfo me from comment #6)
> I guess that could happen if we still have an update task queued when the
> media element is garbage collected? Seems like that wouldn't be this common.
> Anyway, here's a patch the add the null check. Benjamin Chen is the expert
> here, but is away. Hopefully he can correct the analysis if necessary next
> week.

Is it possible that the TextTrackCue been unlink but the TextTrackManager has not been unlink yet?
If TextTrackManager been unlink, the function TextTrackManager::TimeMarchesOn() will early return here http://searchfox.org/mozilla-central/source/dom/html/TextTrackManager.cpp#599
Let's keep this on the radar as a possible 51 dot-release ride-along as well.
Tracking as new regression in 51 in case we need a dot release, and for 52/53 to make sure this gets uplifted.
Comment on attachment 8832311 [details]
Bug 1333931 - Handle nullptr TextTrack objects in sorting.

Approval Request Comment
[Feature/Bug causing the regression]: WebVTT (video subtitles)

[User impact if declined]: Crashes possible after loading a page with video subtitles.

[Is this code covered by automated tests?]: Normal behaviour is. We don't have an automated testcase for the crash issue.

[Has the fix been verified in Nightly?]: Yes. (There are no crash reports outside 51, but the change has shown no side-effects in Nightly.)

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: I do not think the change in risky.

[Why is the change risky/not risky?]:  It adds a simple null-check to protect against a crash, and a non-release assert to help track down similar issues on Nightly/Aurora. I expect no behaviour change with subtitle playback. If there is a regression, the change is simple to revert.

[String changes made/needed]: None.
Flags: needinfo?(giles)
Attachment #8832311 - Flags: approval-mozilla-release?
Attachment #8832311 - Flags: approval-mozilla-beta?
Attachment #8832311 - Flags: approval-mozilla-aurora?
Comment on attachment 8832311 [details]
Bug 1333931 - Handle nullptr TextTrack objects in sorting.

Take this patch in Aurora53 first and see if any side effect. Aurora53+.
Attachment #8832311 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8832311 [details]
Bug 1333931 - Handle nullptr TextTrack objects in sorting.

protect against null dereference, beta52+
Attachment #8832311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- per comment 17.
Flags: qe-verify-
Another variant of this crash.
Crash Signature: [@ mozilla::dom::CompareTextTracks::LessThan] → [@ mozilla::dom::CompareTextTracks::LessThan] [@ RefPtr<T>* nsTArray_Impl<T>::InsertElementSorted<T> | mozilla::dom::TextTrackManager::TimeMarchesOn ]
Comment on attachment 8832311 [details]
Bug 1333931 - Handle nullptr TextTrack objects in sorting.

We don't have then plan to have dot release for 51. Rel51-.
Attachment #8832311 - Flags: approval-mozilla-release? → approval-mozilla-release-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.