Closed
Bug 1333931
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::dom::CompareTextTracks::LessThan
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: bkelly, Assigned: rillian)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
kinetik
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
gchang
:
approval-mozilla-release-
|
Details |
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
Reporter | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
We can certainly nullcheck in the comparison, but I don't understand yet why we're seeing null track pointers in the first place...
Reporter | ||
Comment 4•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•7 years ago
|
||
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
Comment 10•7 years ago
|
||
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
status-firefox51:
--- → affected
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/931cbdebf471
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 12•7 years ago
|
||
Thanks! We should at least uplift this to FF52 and FF53.
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Comment 13•7 years ago
|
||
(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)
Comment 14•7 years ago
|
||
(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
Comment 15•7 years ago
|
||
Let's keep this on the radar as a possible 51 dot-release ride-along as well.
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
Comment 16•7 years ago
|
||
Tracking as new regression in 51 in case we need a dot release, and for 52/53 to make sure this gets uplifted.
Assignee | ||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b251eb90b7d
Comment 20•7 years ago
|
||
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+
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8e853e2c67d1
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/8e853e2c67d1
status-firefox-esr52:
--- → fixed
Reporter | ||
Comment 24•7 years ago
|
||
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 25•7 years ago
|
||
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-
Updated•7 years ago
|
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•