Closed
Bug 1385092
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::dom::TextTrackManager::UpdateCueDisplay
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: marcia, Assigned: bechen)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
alwu
:
review+
|
Details |
3.28 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-c87b1ddc-29ba-41c7-8ed7-41ae30170727. ============================================================= Not sure where to bucket this - crashes started using 20170727100347: http://bit.ly/2vNcpwx Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8400551c2e39f24c75a009ebed496c7acd7bf47&tochange=e5693cea1ec944ca077c7a46c5f127c828a90f1b
Reporter | ||
Comment 1•7 years ago
|
||
As discussed in triage, I think this might be in the wrong component. It also is very small volume with just a handful of crashes.
Component: DOM → Audio/Video: Playback
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•7 years ago
|
||
Apparently the |StaticRefPtr<nsIWebVTTParserWrapper> TextTrackManager::sParserWrapper| was destroyed by |ClearOnShutdown|. Then trigger the TextTrackManager::TimeMarchesOn().
Assignee: nobody → bechen
Flags: needinfo?(bechen)
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8894821 [details] Bug 1385092 - Null check for the sParserWrapper. https://reviewboard.mozilla.org/r/165978/#review171544 r+ with handling following issues. ::: dom/html/TextTrackManager.h:156 (Diff revision 1) > bool TrackIsDefault(TextTrack* aTextTrack); > > void ReportTelemetryForTrack(TextTrack* aTextTrack) const; > void ReportTelemetryForCue(); > > + bool IsShutdown(); nit : bool IsShutdown() const; ::: dom/html/TextTrackManager.h:179 (Diff revision 1) > { > MOZ_ASSERT(NS_IsMainThread()); > if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) { > nsContentUtils::UnregisterShutdownObserver(this); > mManager->NotifyShutdown(); > } If the event "NS_XPCOM_SHUTDOWN_OBSERVER_ID" might be lost, we can remove the proxy and check the |sParserWrapper| to decide whether we can dispatch the task. ::: dom/html/TextTrackManager.cpp:133 (Diff revision 1) > mPendingTextTracks = new TextTrackList(window, this); > > if (!sParserWrapper) { > nsCOMPtr<nsIWebVTTParserWrapper> parserWrapper = > do_CreateInstance(NS_WEBVTTPARSERWRAPPER_CONTRACTID); > sParserWrapper = parserWrapper; Add MOZ_ASSERT(parserWrapper) here.
Attachment #8894821 -
Flags: review?(alwu) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ffe95a3da3aa Null check for the sParserWrapper. r=alwu
Keywords: checkin-needed
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffe95a3da3aa
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 10•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: The bug exist from the beginning of TextTrackManager.cpp. [User impact if declined]: Low possibility crash when closing the browser which is playing a video with subtitle through webvtt. [Is this code covered by automated tests?]: No, because the shutdown procedure only verified when the webvtt testcase was locate on the end of the testing chunk. [Has the fix been verified in Nightly?]: No, can not reproduce. [Needs manual test from QE? If yes, steps to reproduce]: No, can not reproduce. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: No. [Why is the change risky/not risky?]: Only null checking. Low risk patch. [String changes made/needed]: none
Flags: needinfo?(bechen)
Attachment #8896857 -
Flags: approval-mozilla-beta?
Comment 11•7 years ago
|
||
Comment on attachment 8896857 [details] [diff] [review] bug1385092beta.patch Crash fix, let's take it for beta 3.
Attachment #8896857 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/690335e86039
You need to log in
before you can comment on or make changes to this bug.
Description
•