1166320 - [DeviceStorage] Fix thread safety design issues with volume service

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

Volume service is designed to be accessed off the main thread, but there are problems with the current implementation.

1) When a volume is removed via nsVolumeService::RemoveVolumeByName, the actual removal of the pointer from mVolumeArray is not protected by the array lock.

2) When a volume is updated via nsVolumeService::UpdateVolume, clients of the service may already hold a pointer to the volume object being updated and be concurrently accessing its properties, including non-primitives such as nsString. Rather than update the object in place and risk indeterminate behaviour, the volume service should store a new volume object while holding the array lock. This way the client will have a consistent (if stale) state and may retry as appropriate if the operation fails as a result (this race condition should already be considered).

As a result, from the perspective of the volume service client, a volume object's properties are immutable.

Operations such as mount, unmount and format, may only be completed in the parent process, and thus are not a concern for web workers.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1166320.patch, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e096e8f6c9

Comment 2

[Dave Hylands [:dhylands]]

Comment on attachment 8612299 [details] [diff] [review]
bug1166320.patch, v1

Review of attachment 8612299 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

::: dom/system/gonk/nsVolumeService.cpp
@@ +394,5 @@
> +  {
> +    MonitorAutoLock autoLock(mArrayMonitor);
> +    nsVolume::Array::index_type volIndex;
> +    nsRefPtr<nsVolume> vol = FindVolumeByName(aVolume->Name(), &volIndex);
> +    if (vol) {

nit: They seem to like early returns, so I would be inclined to change this to be:

> if (!vol) {
>   mVolumeArray.AppendElement(aVolume);
>   return;
> }

and then unindent the rest of this function.

@@ +495,5 @@
> +    nsRefPtr<nsVolume> vol = FindVolumeByName(aName, &volIndex);
> +    if (!vol) {
> +      return;
> +    }
> +    mVolumeArray.RemoveElementAt(volIndex);

good catch!

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Dave Hylands [:dhylands] from comment #2)
> ::: dom/system/gonk/nsVolumeService.cpp
> @@ +394,5 @@
> > +  {
> > +    MonitorAutoLock autoLock(mArrayMonitor);
> > +    nsVolume::Array::index_type volIndex;
> > +    nsRefPtr<nsVolume> vol = FindVolumeByName(aVolume->Name(), &volIndex);
> > +    if (vol) {
> 
> nit: They seem to like early returns, so I would be inclined to change this
> to be:
> 
> > if (!vol) {
> >   mVolumeArray.AppendElement(aVolume);
> >   return;
> > }
> 
> and then unindent the rest of this function.
>

Won't returning early there skip the observer notifications?

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1166320.patch, v2 [carries r=dhylands]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d636461e155c

While I couldn't return any earlier in the section noted in the review, I did manage to reduce the LOC / indentation with some minor shuffling.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/371c3a42b25c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/371c3a42b25c

Comment 7

[KTucker [:KTucker][Inactive 3/4/2016]]

This is causing a smoke test blocker bug 1171768. We will need this backed out.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This was backed out in bug 1171768.
https://hg.mozilla.org/integration/b2g-inbound/rev/163b199605d9

Comment 9

[Dave Hylands [:dhylands]]

The issue seems to be related to mount locks.

Volumes are managed on the IOThread and are the "master" for all of the information about a volume, except for mount locks.

nsVolumes are managed on the main thread are are the "master" for the mount lock information. This is why the generation number exists, because its possible for updates to be going from main-thread to IOThread for the mount lock information at the same time as volume state changes coming from Volume Manager on the IOThread to change the state.

The generation number increments each time the volume transitions to the MOUNTED state and the mount lock is only valid for a particular generation.

At startup, the VolumeManager marks the volume as "locked" and expects to get an update from the VolumeService to indicate that its really locked or not.

I suspect that this bit of logic got broken in this bug.

Comment 10

[Doug Sherk (:drs) (inactive)]

Andrew, could you hold off on landing this for 3 weeks? We have no confirmation of this, but it's possible that the patches here caused bug 1171827. Due to the difficulty in reproing that bug, it's difficult for us to verify this causality. It's also difficult to verify that this patch won't cause it again. Thanks for your understanding.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1166320.patch, v3

Tested original patch, wouldn't mount my external sdcard on an aries. Tested without and with the new patch, would mount my sdcard.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/db0ec7465b4a

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db0ec7465b4a