872800 - DeviceStorage incorrectly reports volume status changes on composite devices

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

When you call addEventListener on a composite device, you'd get available/unavailable/shared notifications for the underlying non-composite storage areas rather then getting the composite status that you'd get by using the available() method.

Nominating as leo? since this should have been part of bug 858416 (which is leo+)

Comment 1

[Dylan Oliver [:doliver]]

Blocking required 1.1 feature.

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Pt 1 - Removes some extra printf_stderr's that weren't supposed to part of bug 858416

This patch applies cleanly to both trunk and b2g18

Comment 3

[Dave Hylands [:dhylands]]

Attachment: Pt 2 - master - Fix volume availability notifications for composite storage areas.

This patch is rebased to apply on master.

Comment 4

[Dave Hylands [:dhylands]]

Attachment: Pt - b2g18 - Fix volume availability notifications for composite storage areas.

This patch is rebased to apply on mozilla-b2g18.

Comment 5

[Dave Hylands [:dhylands]]

Attachment: Pt 2 - master - Fix volume availability notifications for composite storage areas.

Patch for the master branch (mozilla-b2g18 will need a branch specific patch)

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 750170 [details] [diff] [review]
Pt 2 - master - Fix volume availability notifications for composite storage areas.

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

Hey Marco,

Do you want to review this? And perhaps test it out to see if it fixes the problem in bug 862784.

The way this is coded, if you have a listener on a composite device, then the change message should have e.path == '' and e.reason will be the composite status (i.e. available if any volume is available, etc as per DeviceStorageFile::GetStatus).

For non-composite storage areas, then e.path will contain the volume name, and the e.reason should reflect the availabily of that particular volume.

Comment 7

[Marco Chen [:mchen]]

Hi Dave,

Yes, I can do it on today or tomorrow morning.

Comment 8

[Marco Chen [:mchen]]

Comment on attachment 750170 [details] [diff] [review]
Pt 2 - master - Fix volume availability notifications for composite storage areas.

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

Hi Dave,

It looks great.
I also tested it on Leo device and it works.

About the review comments. There are few for asking the comment for easy to understanding.
And only one about user cache is what I have a question there.
Please help to clarify that then the rest is good to me. Thanks.

::: dom/devicestorage/DeviceStorage.h
@@ +203,3 @@
>  
>    bool IsComposite() { return mStores.Length() > 0; }
> +  bool IsCompositeComponent() { return mCompositeComponent; }

Maybe to add comment for what is difference between IsCompostie & IsCompositeCompoent will be good for other people to understand well. That caused me sometime to get it's function.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3121,5 @@
>    }
>  
>  #ifdef MOZ_WIDGET_GONK
>    else if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    // We only invalidate the used space cache for the volume that actually changed state.

This seems to be not related to this bug. Right?

By the way, this comment seems to indicate that here should do some check about real state change but I don't see related code section here. May I loss something?

@@ +3135,5 @@
>      usedSpaceCache->Invalidate(volName);
>  
> +    // But if we're a composite storage area, we want to report a composite availability,
> +    // so we use mStorageName here instead of volName.
> +    DeviceStorageFile dsf(mStorageType, mStorageName);

Could you add a comment into declare of mStorageType for showing that the empty string means it is a composite device? That will be easy for understanding by others.

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #8)
> Comment on attachment 750170 [details] [diff] [review]
> Pt 2 - master - Fix volume availability notifications for composite storage
> areas.
> 
> Review of attachment 750170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Dave,
> 
> It looks great.
> I also tested it on Leo device and it works.
> 
> About the review comments. There are few for asking the comment for easy to
> understanding.
> And only one about user cache is what I have a question there.
> Please help to clarify that then the rest is good to me. Thanks.
> 
> ::: dom/devicestorage/DeviceStorage.h
> @@ +203,3 @@
> >  
> >    bool IsComposite() { return mStores.Length() > 0; }
> > +  bool IsCompositeComponent() { return mCompositeComponent; }
> 
> Maybe to add comment for what is difference between IsCompostie &
> IsCompositeCompoent will be good for other people to understand well. That
> caused me sometime to get it's function.

Sounds reasonable. A composite storage area is made up of the composite device itself (so IsComposite would return true for this device), and real storage areas (for which IsCompositeComponent would return true).

For a composite device, mStorageName is empty, and there is no root directory.

> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +3121,5 @@
> >    }
> >  
> >  #ifdef MOZ_WIDGET_GONK
> >    else if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> > +    // We only invalidate the used space cache for the volume that actually changed state.
> 
> This seems to be not related to this bug. Right?

Right. We calculate used space for pictures/videos/music in one pass and cache the results. If the volume status changes, or a file is modified then we need to invalidate the cache. This was being done in this function previously, so it still needed to be there. It just got shuffled around a bit.

> By the way, this comment seems to indicate that here should do some check
> about real state change but I don't see related code section here. May I
> loss something?

I don't understand your question.

> @@ +3135,5 @@
> >      usedSpaceCache->Invalidate(volName);
> >  
> > +    // But if we're a composite storage area, we want to report a composite availability,
> > +    // so we use mStorageName here instead of volName.
> > +    DeviceStorageFile dsf(mStorageType, mStorageName);
> 
> Could you add a comment into declare of mStorageType for showing that the
> empty string means it is a composite device? That will be easy for
> understanding by others.

Sure - yeah for composite storage devices, mStorageName will be the empty string.

Comment 10

[Marco Chen [:mchen]]

>>> +    // We only invalidate the used space cache for the volume that actually changed state.

>> By the way, this comment seems to indicate that here should do some check
>> about real state change but I don't see related code section here. May I
>> loss something?

> I don't understand your question.

You comment said that cache should be invalidated when the volume state is actually changed. But I didn't see any code for checking whether the volume state is really changed or not. Ex: cache shouldn't be invalidated when state change is the same ex: sdcard::available -> sdcard::available

Comment 11

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #10)
> >>> +    // We only invalidate the used space cache for the volume that actually changed state.
> 
> >> By the way, this comment seems to indicate that here should do some check
> >> about real state change but I don't see related code section here. May I
> >> loss something?
> 
> > I don't understand your question.
> 
> You comment said that cache should be invalidated when the volume state is
> actually changed. But I didn't see any code for checking whether the volume
> state is really changed or not. Ex: cache shouldn't be invalidated when
> state change is the same ex: sdcard::available -> sdcard::available

Yeah - that's correct. we could maintain that state, but currently don't which means that the cache may get invalidated more frequently than it should. The way the volume manager works though, you get a burst of state changes all at once and then you typically don't get any more until a real state change happens.

The only place that would benefit from this particular optimization (not invalidating the cache too frequently) would be if you trying to view the used space in the settings app while you were unplugging the USB cable.

In any case, this would all be covered under a separate bug and isn't related to this particular bug.

Comment 12

[Marco Chen [:mchen]]

Comment on attachment 750170 [details] [diff] [review]
Pt 2 - master - Fix volume availability notifications for composite storage areas.

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

Ok, since Dave will fire a follow up bug for my question about invalidating user cache then for this bug I am willing to give r+. By the way, please help to add the comment to what I mentioned at previous review comment.

Thanks Dave's quickly support on this issue.

Comment 13

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/projects/birch/rev/f35b9c00f84b - Pt 1
https://hg.mozilla.org/projects/birch/rev/9d8ea345be12 - Pt 2

I filed bug 873107 as a followup for the cache invalidation issue.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f35b9c00f84b
https://hg.mozilla.org/mozilla-central/rev/9d8ea345be12

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 16

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4850c31aa9d4
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9be045b232ea