878310 - Improve DeviceStorage volume status reporting

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

Currently, device storage does a simple mapping of volume statuses onto one of: available, unavailable, shared.

The current mapping is:
mounted -> available
shared or sharedmnt -> shared
anything else -> unavailable

So currently, when a volume transitions from mounted to shared and back to mounted (so the cycle of sharing a volume with the PC, and then giving it back), device storage winds up reporting the following:

    available, unavailable, shared, unavailable, available

As part of adding support for multiple storage areas, and integrating all of that into the mediadb, this is insufficient.

So we'd like to propose the following changes:

1 - Add an isMediaPresent attribute to nsIVolume. This would directly reflect the mMediaPresent attribute currently maintained for the Volume object (from Volume.h)

2 - Add an isSharing attribute. This would be set to true by the AutoMounter when the automounter tries to share a volume, and entering mounted, nomedia, or formatting states would set the attribute to false.

This latter change would then cover all of the transition states that the volume might go through when going from Mounted -> unmounting -> idle -> pending -> shared -> idle -> checking -> mounted.

Device Storage would then be modified to report statuses as follows:

if !vol.isMediaPresent report unavailable
if vol.isSharing report shared
if volume state is mounted, report available
otherwise report unavailable

Under this new scheme, device storage would wind up reporting a sequence of:

    available, shared, available

when transitioning from mounted through to shared and back to mounted.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Don't send unavailable when transitioning from mounted to shared and back again

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Don't send unavailable when transitioning from mounted to shared and back again. v2

Comment 3

[Dave Hylands [:dhylands]]

Comment on attachment 793756 [details] [diff] [review]
Don't send unavailable when transitioning from mounted to shared and back again. v2

Kyle has reviewed a bunch of the volume stuff, and this changeset contains mostly volume stuff with a splash of device storage thrown in.

Comment 4

[Dave Hylands [:dhylands]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Request to nominate it as leo+, because it blocks a leo+ bug, bug 890482.

Comment 7

[Peter Dolanjski [:pdol]]

Marking non-blocking based on triage.

Please re-nom if the user impact is severe (hard to tell from the bug).  If re-noming, please comment on the risk.

Comment 8

[Dave Hylands [:dhylands]]

Attachment: 1.1 specific version of the patch.

This is a 1.1 specific version of the patch. Carrying r+ from master version of the patch.

Comment 9

[Dave Hylands [:dhylands]]

Renoming now that I've confirmed that this patch actually fixes bug 901919 which is marked as leo+

This is a fairly straight forward patch (adds a couple of attributes to the nsVolume object) and modifies the volume state reporting to eliminate unwanted state transitions.

So I think that the risk is low.

This patch has already been confirmed to fix several leo related problems on master.

Comment 10

[Dave Hylands [:dhylands]]

I confirmed that a 1.1 specific version of the patch for bug 878310 fixes this bug, so I renom'd buf 878310 as leo?

Comment 11

[Dave Hylands [:dhylands]]

(In reply to Dave Hylands [:dhylands] from comment #10)
> I confirmed that a 1.1 specific version of the patch for bug 878310 fixes
> this bug, so I renom'd buf 878310 as leo?

Oops - comment was supposed to go on bug 901919

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Dave Hylands [:dhylands] from comment #11)
> (In reply to Dave Hylands [:dhylands] from comment #10)
> > I confirmed that a 1.1 specific version of the patch for bug 878310 fixes
> > this bug, so I renom'd buf 878310 as leo?
> 
> Oops - comment was supposed to go on bug 901919

The patch has substantial amount of change and I don't think uplifting so much is a good idea at this point in the cycle.Can you confirm and give more information on the risk here ? How much testing has been done or is needed.Risk of new regressions etc ? 

Is a backout(may be low risk ?) not possible to fix 901919 ?

Comment 13

[Dave Hylands [:dhylands]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> (In reply to Dave Hylands [:dhylands] from comment #11)
> > (In reply to Dave Hylands [:dhylands] from comment #10)
> > > I confirmed that a 1.1 specific version of the patch for bug 878310 fixes
> > > this bug, so I renom'd buf 878310 as leo?
> > 
> > Oops - comment was supposed to go on bug 901919
> 
> The patch has substantial amount of change and I don't think uplifting so
> much is a good idea at this point in the cycle.Can you confirm and give more
> information on the risk here ? How much testing has been done or is
> needed.Risk of new regressions etc ? 

I guess it depends on your definition of substantial :)

Most of the patch adds 2 new attributes to the nsVolume class, which shouldn't affect anybody else, since they're not using the attributes.

The part of the patch which is significant, is the changes to nsDeviceStorage.cpp, which modifies the events being delivered. mediadb.js is currently the only code which is using those events. If you dropped the changes from nsDeviceStorage.cpp, then this patch is essentially a no-op, adding some stuff that nobody else would use.

I tested by adding instrumentation to mediadb.js to observe the delivered events. Without the patch, device storage delivers mounted -> unavailable -> shared -> unavailable -> mounted.

With the patch, I observed mounted -> shared -> mounted. The intermediate unavailable events are indistinguishable from the unavailable that occurs when the media is physically removed.

The same patch on master was also tested by John Hu and fixes bug 884688, which is a dup of 901919. 901919 is marked as leo+, so I suppose I could just attach the patch to that bug and land it, but that doesn't feel right to me. I'll mark this bug as blocking 901919.

> Is a backout(may be low risk ?) not possible to fix 901919 ?

This isn't a regression. Its a bug that's always been there. There isn't anything to backout.

Comment 14

[Preeti Raghunath(:Preeti)]

plus based on comment 30 in 901919.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/52e44e1d88da

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/52e44e1d88da