Closed Bug 1286766 Opened 4 years ago Closed 4 years ago

Data race in sSuspendBackgroundVideoDelay (dom/media/MediaDecoderStateMachine.cpp)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

sSuspendBackgroundVideoDelay is read in VisibilityChanged() off the main thread and written in the every constructor of MDSM.
Assignee: nobody → jwwang
Priority: -- → P3
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

https://reviewboard.mozilla.org/r/64182/#review61224

LGTM as it solves the race.
But I think Dan should have a look too, as this change means the pref will only be read once per Firefox session.
Attachment #8770853 - Flags: review?(gsquelart) → review+
Do we have use cases that need this pref value to change on the fly?
https://reviewboard.mozilla.org/r/64182/#review61474

::: dom/media/MediaDecoderStateMachine.cpp:212
(Diff revision 1)
> -static TimeDuration sSuspendBackgroundVideoDelay;
> -
> +static TimeDuration
> +SuspendBackgroundVideoDelay()
> -static void
> -InitSuspendBackgroundPref()
>  {
> -  MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +  static TimeDuration sDelay = TimeDuration::FromMilliseconds(

Is it possible to remove static here? Can we only call MediaPrefs from the MainThread?

The reason is that I'd like to make it possible to change the value of the pref and have immediate effect, with out requiring a browser restart.

(This was uncovered in writing mochitests and trying to change the pref to different values for testing.)
PrefAddVarCache() employed by MediaPrefs changes |mValue| on the main thread whenever the pref changes. However, reading it off the main thread is a data race without synchronization.

In fact, MediaPrefs is race-free only when you never change the prefs.

Btw, do you mean your test needs the pref change to take effect for the same MDSM instance or a new MDSM object?
Flags: needinfo?(dglastonbury)
https://reviewboard.mozilla.org/r/64182/#review61474

> Is it possible to remove static here? Can we only call MediaPrefs from the MainThread?
> 
> The reason is that I'd like to make it possible to change the value of the pref and have immediate effect, with out requiring a browser restart.
> 
> (This was uncovered in writing mochitests and trying to change the pref to different values for testing.)

I make it a class member instead of a static so the pref change can take effect in the mochitest.
Attachment #8770853 - Flags: review+
(In reply to JW Wang [:jwwang] from comment #5)
> Btw, do you mean your test needs the pref change to take effect for the same
> MDSM instance or a new MDSM object?

I originally meant take effect with the same MDSM instance, but I think I can work with the time out taking effect on new MDSMs.
Flags: needinfo?(dglastonbury) → needinfo?(jwwang)
Per comment 5, there is no way for pref change to take effect off the main thread without incurring data race as far as current pref APIs are concerned. My last patch should be good enough to change the pref dynamically (in the beginning) for a mochitest. Can you review the patch for me? Thanks!
Flags: needinfo?(jwwang) → needinfo?(dglastonbury)
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

https://reviewboard.mozilla.org/r/64182/#review62440
Attachment #8770853 - Flags: review?(dglastonbury)
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

https://reviewboard.mozilla.org/r/64182/#review62454

![Ship it!](https://qph.ec.quoracdn.net/main-qimg-0bf1d3de309a89d9ed70ae91f6271a7f)
Attachment #8770853 - Flags: review+
Flags: needinfo?(dglastonbury)
Just found out we have Preferences::AddAtomicUintVarCache() to register an atomic to be modified when the pref value changes. We can use this API to implement the requirement of comment 8.
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

https://reviewboard.mozilla.org/r/64182/#review62460

Hi Gerald,
Can you review the patch again? Thanks!
Attachment #8770853 - Flags: review?(jwwang)
Btw, I have some problems in resetting review flags in mozreview.
Attachment #8770853 - Flags: review?(jwwang)
Attachment #8770853 - Flags: review+
Attachment #8770853 - Flags: review?(gsquelart)
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64182/diff/1-2/
Attachment #8770853 - Attachment description: Bug 1286766 - ensure static init happen only once on the main thread to avoid data race. → Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().
Attachment #8770853 - Flags: review?(gsquelart) → review+
Comment on attachment 8770853 [details]
Bug 1286766 - make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache().

https://reviewboard.mozilla.org/r/64182/#review62782
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4ec1b2cf832
make MediaPrefs::MDSMSuspendBackgroundVideoDelay() safe to read off main thread using Preferences::AddAtomicUintVarCache(). r=gerald,kamidphish
https://hg.mozilla.org/mozilla-central/rev/d4ec1b2cf832
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.