Closed Bug 1401471 Opened 2 years ago Closed 2 years ago

Data race in reading mChannelOffset outside the cache monitor

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/dom/media/MediaCache.cpp#1441

It is not safe to read mChannelOffset outside the cache monitor because mChannelOffset might be being updated in NotifyDataReceived() off the main thread.

We should always acquire the monitor before accessing mChannelOffset.
Assignee: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Attachment #8910741 - Flags: review?(gsquelart)
Attachment #8910742 - Flags: review?(gsquelart)
Attachment #8910743 - Flags: review?(gsquelart)
Attachment #8910744 - Flags: review?(gsquelart)
Comment on attachment 8910741 [details]
Bug 1401471. P1 - make StreamAction a struct so we can associate data with each action in the future.

https://reviewboard.mozilla.org/r/182192/#review187720
Attachment #8910741 - Flags: review?(gsquelart) → review+
Comment on attachment 8910742 [details]
Bug 1401471. P2 - remove SEEK_AND_RESUME.

https://reviewboard.mozilla.org/r/182194/#review187722
Attachment #8910742 - Flags: review?(gsquelart) → review+
Comment on attachment 8910742 [details]
Bug 1401471. P2 - remove SEEK_AND_RESUME.

https://reviewboard.mozilla.org/r/182194/#review187728

::: dom/media/MediaCache.cpp:1131
(Diff revision 1)
>    struct StreamAction
>    {
>      enum
>      {
>        NONE,
>        SEEK,
> -      SEEK_AND_RESUME,
>        RESUME,
>        SUSPEND
>      } mTag = NONE;
> +    bool mResume = false;
>    };

Now that you're adding more tag-specific data, I'm thinking `Variant` may be a better choice?
(Though only one tag carries data at the moment, so it wouldn't actually help yet.)

So up to you if you want to change now, or it can be done if a future bug if we need to store data with other tags.

But at least, could you please add a comment above 'mResume', to indicate that it's only relevant in the SEEK case?
Comment on attachment 8910743 [details]
Bug 1401471. P3 - store the seek target in StreamAction so we won't need to read mChannelOffset outside the cache monitor.

https://reviewboard.mozilla.org/r/182196/#review187726
Attachment #8910743 - Flags: review?(gsquelart) → review+
Comment on attachment 8910744 [details]
Bug 1401471. P4 - always access mChannelOffset within the cache monitor.

https://reviewboard.mozilla.org/r/182198/#review187730
Attachment #8910744 - Flags: review?(gsquelart) → review+
Comment on attachment 8910742 [details]
Bug 1401471. P2 - remove SEEK_AND_RESUME.

https://reviewboard.mozilla.org/r/182194/#review187728

> Now that you're adding more tag-specific data, I'm thinking `Variant` may be a better choice?
> (Though only one tag carries data at the moment, so it wouldn't actually help yet.)
> 
> So up to you if you want to change now, or it can be done if a future bug if we need to store data with other tags.
> 
> But at least, could you please add a comment above 'mResume', to indicate that it's only relevant in the SEEK case?

I will add a comment. I would like to avoid the complexity of Variant<> for the redundancy is acceptable given we have only a few members. I wish we had something like Rust enum which allows you to associate data with each enum variant.
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff9dbb6a6692
P1 - make StreamAction a struct so we can associate data with each action in the future. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f871c6aa4d90
P2 - remove SEEK_AND_RESUME. r=gerald
https://hg.mozilla.org/integration/autoland/rev/554875cb6a2e
P3 - store the seek target in StreamAction so we won't need to read mChannelOffset outside the cache monitor. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6a2c85349226
P4 - always access mChannelOffset within the cache monitor. r=gerald
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fec2e3f42ddf
Backed out 4 changesets because it depends on 1401461 which is being backed out
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6726a06cf080
P1 - make StreamAction a struct so we can associate data with each action in the future. r=gerald
https://hg.mozilla.org/integration/autoland/rev/aa7c1fc82e56
P2 - remove SEEK_AND_RESUME. r=gerald
https://hg.mozilla.org/integration/autoland/rev/2682041ff5dc
P3 - store the seek target in StreamAction so we won't need to read mChannelOffset outside the cache monitor. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f62add483860
P4 - always access mChannelOffset within the cache monitor. r=gerald
Blocks: 1428688
No longer blocks: 1428688
You need to log in before you can comment on or make changes to this bug.