Closed Bug 1336345 Opened 3 years ago Closed 3 years ago

Let MediaShutdownManager register shutdown blocker on startup instead of on demand

Categories

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

defect

Tracking

()

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

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

Per bug 1320346 comment 16, sometimes AddBlocker() returns NS_ERROR_XPC_CI_RETURNED_FAILURE for unknown reason. I will do the registration on startup so it is only done once for the entire Firefox session hoping that will reduce the chance of NS_ERROR_XPC_CI_RETURNED_FAILURE.
Assignee: nobody → jwwang
Blocks: 1320346
Priority: -- → P1
Attachment #8833838 - Flags: review?(gsquelart)
Attachment #8833839 - Flags: review?(gsquelart)
Comment on attachment 8833838 [details]
Bug 1336345. Part 1 - register shutdown blocker only once when the 1st MediaDecoder is created.

https://reviewboard.mozilla.org/r/109964/#review110932

r+ with suggestion:

::: dom/media/MediaShutdownManager.cpp
(Diff revision 1)
> -  if (!sInstance) {
> -    sInstance = new MediaShutdownManager();
> -  }
>    return *sInstance;

It would be cheap and it may be worth adding a `MOZ_ASSERT(&*sInstance);` to more easily debug too-early or too-late accesses.
Attachment #8833838 - Flags: review?(gsquelart) → review+
Comment on attachment 8833838 [details]
Bug 1336345. Part 1 - register shutdown blocker only once when the 1st MediaDecoder is created.

https://reviewboard.mozilla.org/r/109964/#review110932

> It would be cheap and it may be worth adding a `MOZ_ASSERT(&*sInstance);` to more easily debug too-early or too-late accesses.

The syntax is kinda weird. Since sInstance is a StaticRefPtr<>, MOZ_ASSERT(sInstance) should do it.
Thanks for the review!
Comment on attachment 8833838 [details]
Bug 1336345. Part 1 - register shutdown blocker only once when the 1st MediaDecoder is created.

https://reviewboard.mozilla.org/r/109964/#review110932

> The syntax is kinda weird. Since sInstance is a StaticRefPtr<>, MOZ_ASSERT(sInstance) should do it.

I know, I initially wrote it as the more common `!!sInstance`. But then I "cleverly" thought we could exercise the '*sInstance' done in the return after it!
Anyway, if StaticRefPtr allows for just `MOZ_ASSERT(sInstance)`, that's great, please go for it.
That's a good thinking! However, I would prefer to let the compiler do the clever job to reuse |*sInstance| and keep the syntax clear when possible.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05e8e7d0f430
Part 1 - register shutdown blocker on startup. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c4247e22a8ff
Part 2 - some code cleanup. r=gerald
(In reply to Phil Ringnalda (:philor) from comment #12)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/6c56e3e2d667
> for gtest timeouts,

MediaShutdownManager::BlockShutdown() is never called during gtest tear-down. I think this is because gtest doesn't run JS code at all. I will call MediaShutdownManager::InitStatics() upon the 1st instantiation of MediaDecoder instead.
Hi Gerald,
Per comment 13, can you review P1 again? Thanks!
Flags: needinfo?(gsquelart)
Comment on attachment 8833838 [details]
Bug 1336345. Part 1 - register shutdown blocker only once when the 1st MediaDecoder is created.

Looks good to me again!

However, your Try failed in test_eme_non_mse_fails.html twice (I've just requested a couple more runs), you may want to investigate that before autolanding again...
Flags: needinfo?(jwwang)
(In reply to Gerald Squelart [:gerald] from comment #19)
> However, your Try failed in test_eme_non_mse_fails.html twice (I've just
> requested a couple more runs), you may want to investigate that before
> autolanding again...

Which try log do you mean?
Flags: needinfo?(jwwang)
Do you mean  https://treeherder.mozilla.org/#/jobs?repo=try&revision=503142ab23f44d556db97c447dfd0380a3fcc408?
The failures come from the changes I added to debug recent timeouts in test_seek-*.
Flags: needinfo?(gsquelart)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/370b66055c09
Part 1 - register shutdown blocker only once when the 1st MediaDecoder is created. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d729059d550a
Part 2 - some code cleanup. r=gerald
https://hg.mozilla.org/mozilla-central/rev/370b66055c09
https://hg.mozilla.org/mozilla-central/rev/d729059d550a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8843800 [details] [diff] [review]
1336345_aurora_53_fix.patch

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:This patch reduces the crash in bug 1320346.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[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?]:low risk.
[Why is the change risky/not risky?]:the change is simple and has been exercised in Central for 1 month without causing obvious regression.
[String changes made/needed]:none
Attachment #8843800 - Flags: review+
Attachment #8843800 - Flags: approval-mozilla-aurora?
Attachment #8843800 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8843800 [details] [diff] [review]
1336345_aurora_53_fix.patch

Mitgation for a shutdown crash, let's take this for beta 2.
Attachment #8843800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
JW, for the fixes to MediaShutdown crashes here and in bugs 1336356 and 1343787, can you help us judge if the fix has been effective in beta 53?  You might also have a look at crash-stats on Wednesday (after beta 2 has been released for a day) to see if any new crashes come up that could be related. Thanks!
Flags: needinfo?(jwwang)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)
> JW, for the fixes to MediaShutdown crashes here and in bugs 1336356 and
> 1343787, can you help us judge if the fix has been effective in beta 53? 
> You might also have a look at crash-stats on Wednesday (after beta 2 has
> been released for a day) to see if any new crashes come up that could be
> related. Thanks!

I think so.

https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.0a1&version=54.0a2&version=54.0a1&version=53.0b&version=53.0b1&signature=mozilla%3A%3AMediaShutdownManager%3A%3ARegister&date=%3E%3D2017-02-13T06%3A51%3A15.000Z&date=%3C2017-03-13T06%3A51%3A15.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

The last occurrence is on 53.0b1.
Flags: needinfo?(jwwang)
Setting qe-verify- based on JW Wang's assessment on manual testing needs (see Comment 25) and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8847914 [details] [diff] [review]
1336345_esr52_fix.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): Low. The change is simple and has landed to 54 and 53.
String or UUID changes made by this patch: none
Attachment #8847914 - Flags: approval-mozilla-esr52?
Comment on attachment 8847914 [details] [diff] [review]
1336345_esr52_fix.patch

crash fix, esr52+
Attachment #8847914 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
See Also: → 1373914
You need to log in before you can comment on or make changes to this bug.