Closed Bug 1385092 Opened 7 years ago Closed 7 years ago

Crash in mozilla::dom::TextTrackManager::UpdateCueDisplay

Categories

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

Unspecified
Windows 10
defect

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)

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
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
Benjamin,
Can you check this bug?
Flags: needinfo?(bechen)
Priority: -- → P1
Apparently the |StaticRefPtr<nsIWebVTTParserWrapper> TextTrackManager::sParserWrapper| was destroyed by |ClearOnShutdown|. Then trigger the TextTrackManager::TimeMarchesOn().
Assignee: nobody → bechen
Flags: needinfo?(bechen)
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+
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
https://hg.mozilla.org/mozilla-central/rev/ffe95a3da3aa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Benjamin,
Can you request a uplift to beta?
Flags: needinfo?(bechen)
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 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+
You need to log in before you can comment on or make changes to this bug.