Closed Bug 865347 Opened 7 years ago Closed 7 years ago

Add a test to device storage for checking sdcard availability.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dhylands, Assigned: jwwang)

References

Details

Attachments

(2 files, 6 obsolete files)

Currently, this requires a real device.

After seeing the gaia in nightly, I'd like to add support to the VolumeService to add/manipulate volumes.

If we had that capability then we could simulate sdcard insertion/removal even if there wasn't a real device. This would make testing this in device storage fairly straight forward.
Assign to JW first. He is the newbie in TPE office.
Assignee: nobody → jwwang
Hi Dave,

The normal flow of adding a volume is as follows:

1. nsVolumeService::Observe is called when there are changes in the volume state.
2. nsVolumeService::CreateOrFindVolumeByName is called to append a volume to the list maintaied by nsVolumeService.
3. the new created volume is populated with the passed-in nsIVolume in the 1st parameter of nsVolumeService::Observe


Adding an add() function to nsIVolumeService will not mean much unless a valid nsIVolume is also provided for data population. Does it make sense for the test case to go with an invalid volume?
I guess it all depends on how you define "Invalid volume".

I don't see any problems with creating volumes which may not have any corresponding volume in the volume manager.

nsIVolumeService was created to be a main thread mirror of the volumes maintained by the volume manager, but I don't see any reason why it can't just have some extra volumes.

As long as they point to valid mount locations, it should be fine.
It looks like we can add 3 functions to nsIVolumeService to support your test cases:
1. void addVolume(in nsIVolume vol);
2. void removeVolumeByName(in DOMString volName);
3. void removeVolumeByPath(in DOMString path);

Can you describe how the test cases will use above APIs to test sdcard availability in order to make sense of these functions?
We'd also need a function to set the state of a volume.

Ideally, the volumes should know if they were added using addVolume, versus added by coming from the VolumeManager.

The function for setting the state should fail on volumes coming from the volume manager, and only suceed on volumes that were added through addVolume.

When you add a volume or change its state, then this should trigger an event.

Currently, when the VolumeManager causes a Volume to change state, it sends out an NS_VOLUME_STATE_CHANGED message to registered observers.

I think that we need something similar to occur when an nsIVolume changes state (adding a new volume would trigger a state change). Then device storage can notice when new volumes are added and otherwise change state. Currently device storage listens for NS_VOLUME_STATE_CHANGED, but it should really listen for NSI_VOLUME_STATE_CHANGED messages instead. Then it can update internal caches and other state when the volume state changes.

For example, if a volume becomes unavailable, then trying to add a new file to device storage area for that volume should fail. If the volume then becomes available, then adding a file to to the device storage area should then succeed.

This whole thing will make more sense onceug 858416 lands.
Blocks: 875376
1. Fake volumes are added or updated through nsVolumeService::UpdateFakeVolume().
2. nsVolume::IsFake() to test a fake volume.
3. Changes to fake volumes will notify registered observers as normal volumes do.
4. nsVolumeService::UpdateFakeVolume() should be called in the parent process by the test code as an alternative volume source of volume manager to simulate volumes that don't physically exist.
Attachment #754691 - Flags: review?(dhylands)
Comment on attachment 754691 [details] [diff] [review]
add a function to manipulate fake volumes to test SD card insert/remove

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

So the way that this is currently coded, the "fake" volumes don't get propagated to the content child processes, so they won't find out about them.

Currently, when ContentParent::Observe sees a volume changed notification (https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1516) it sends a FileSystemUpdate to the child process.

ContentChild::RecvFileSystemUpdate (https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1181) then calls VolumeService::UpdateVolume, which then causes the volume the volume cache to be updated in the child, and the changes propogated through a volume changed notification in the child.

We need to add the "fake" attribute, which I'd rename from mFake to mIsFake to the FileSystemUpdate in PContent.ipdl

I'd like to see UpdateFakeVolume set the mIsFake attribute and then just call UpdateVolume. We also need UpdateFakeVolume to work when called from a child process. We could do this by moving FileSystemUpdate from the child section to both:  The Child->Parent direction should only succeed for fake volumes.

::: dom/system/gonk/nsVolumeService.cpp
@@ +374,5 @@
> +    nsString volName;
> +    aVolume->GetName(volName);
> +    vol = FindVolumeByName(volName);
> +
> +    // don't allow updating volumes coming from volume manager

nit: I'd reword this to be something like:

// Don't allow a non-fake volume to become a fake volume.

@@ +379,5 @@
> +    if (vol && !vol->IsFake()) {
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    // volume not existed, create a fake one

nit: I'd reword this to:

// Volume doesn't exist, create a new fake volume.

and the comment should go inside the if.

@@ +386,5 @@
> +      mVolumeArray.AppendElement(vol);
> +    }
> +  }
> +
> +  // by here, we have a fake volume (newly created or existed)

nit: I don't think that this comment really adds any value. I'd remove it.

@@ +388,5 @@
> +  }
> +
> +  // by here, we have a fake volume (newly created or existed)
> +
> +  // Nothing has really changed. Don't bother telling anybody.

nit: Comment belongs inside the if
Attachment #754691 - Flags: review?(dhylands) → review-
(In reply to Dave Hylands [:dhylands] from comment #7)
> So the way that this is currently coded, the "fake" volumes don't get
> propagated to the content child processes, so they won't find out about them.

obs->NotifyObservers is called in UpdateFakeVolume(). Won't that reach ContentParent::Observe() to propagate changes to the child processes?

> ContentChild::RecvFileSystemUpdate
> (https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.
> cpp#1181) then calls VolumeService::UpdateVolume, which then causes the
> volume the volume cache to be updated in the child, and the changes
> propogated through a volume changed notification in the child.

When nsVolumeService::UpdateVolume() is called in the child, obs->NotifyObservers is called to notify observers. Won't that trigger ContentParent::Observe() recursively?
So I guess I was concerned that in the child process, it doesn't know which volumes are fake and which ones aren't. Maybe it doesn't make any difference, but it somehow doesn't feel right.

I'm perfectly happy having UpdateFakeVolume only be valid to be called from the main process for the time being.

What I'm really interested in is being able to write tests which can say manipulate volume status and then observe how devices storage reacts. I'm not sure if the test code runs from the child or the parent, and that will really determine whether we need to remote UpdateFakeVolume or not.
I think the client of nsVolumeService should be ignorant of whether it is a fake volume as long as they behave the same to the test case.

For test code to work, we should add the ability to add fake volumes on both parent and child processes. I would like to add UpdateFakeVolume to the parent section of PContent. Since the code is for test case only, I would like to separate them to keep the logic clean.
OK - Adding UpdateFakeVolume to the parent section of PContent.ipdl seems fine to me.

I'd like to see UpdateFakeVolume just set the mIsFake flag on the volume it was passed and have it call UpdateVolume.

UpdateVolume just needs a check that an incoming fake volume doesn't override an existing real volume.

That will make UpdateFakeVolume not be almost a duplicate of UpdateVolume.
1. Fake volumes are added or updated through nsVolumeService::UpdateFakeVolume().
2. nsVolume::IsFake() to test a fake volume.
3. Changes to fake volumes will notify registered observers as normal volumes do.
4. nsVolumeService::UpdateFakeVolume() should be called by the test code as an alternative volume source of volume manager to simulate volumes that don't physically exist.
   When called in the parent, it calls nsVolumeService::UpdateVolume with a 2nd parameter, aIsFake which indicate whethere the volume is fake or not.
   When called in the child, it sends the volume information to the parent. ContentParent::RecvUpdateFakeVolume then calls nsVolumeService::UpdateFakeVolume() in the parent process.
Attachment #754691 - Attachment is obsolete: true
Attachment #756409 - Flags: review?(dhylands)
Comment on attachment 756409 [details] [diff] [review]
add a function to manipulate fake volumes to test SD card insert/remove

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

I'd really like to see isFake added as a readonly attribute in nsIVolume.idl (don't forget to update the uuid).

I don't like passing around the extra fake argument to a bunch of functions when it really belongs as an attribute on the volume.

We can add a private SetIsFake method to nsVolume (and nsVolumeService is already a friend so it can call private methods on nsVolume).

Then we don't need the special constructor to create a fake volume, just create a regular volume and call SetIsFake.

I would argue that we want isFake exposed since the callers really need to know which volumes that they're allowed to manipulate.

So overall, I really like the way this is turning out. Once we get rid of the aIsFake arguments, I think I'll be happy. So I think we just need another small round of change and it should be good to go.

::: dom/system/gonk/nsVolume.h
@@ +77,5 @@
> +    mMountGeneration(-1),
> +    mMountLocked(true),
> +    mIsFake(aFake)
> +  {
> +  }

As I mentioned in the overview, I think that we can remove this constructor.

@@ +91,4 @@
>    int32_t  mState;
>    int32_t  mMountGeneration;
>    bool     mMountLocked;
> +

nit: I'd get rid of the blank line. I think that mIsFake is just a regular attribute like the rest.

::: dom/system/gonk/nsVolumeService.cpp
@@ +326,4 @@
>  
>  //static
>  already_AddRefed<nsVolume>
> +nsVolumeService::CreateOrFindVolumeByName(const nsAString& aName, bool aIsFake)

I'd prefer not to add the aIsFake here. Just return a regular volume, and have the caller call SetIsFake if they want a fake volume.

@@ +347,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsString volName;
>    aVolume->GetName(volName);
> +  nsRefPtr<nsVolume> vol = CreateOrFindVolumeByName(volName, aIsFake);

I wouldn't add the aIsFake argument here

@@ +362,1 @@
>    vol->Set(aVolume);

The Set method should copy the fake attribute.

@@ +371,5 @@
> +NS_IMETHODIMP
> +nsVolumeService::UpdateFakeVolume(nsIVolume* aVolume)
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    UpdateVolume(aVolume, true);

I really don't like this fake argument.

Let's do

nsReftr<nsVolume> vol = new nsVolume(EmptyString());
vol->Set(aVolume);
vol->SetIsFake(true);
UpdateVolume(vol.get());

and then we don't need to add an aIsFake method to UpdateVolume.
Attachment #756409 - Flags: review?(dhylands) → review-
My original thought was to separate the codes for test only and those for normal workflow. Therefore, I didn't add the isFake attribue to nsIVolume.idl since the caller woundn't have to know whether it is a real volume or not. Instead, it was passed around as an additional parameter to the functions. However, if the caller needs to know a fake volume, let's go the way as you suggested.

Btw, can you explain about updating uuid? I checked the log and found out it was just revolving between uuid(3c9cae8d-9da2-4aa1-b8bf-4db8c8620808) and uuid(1134f267-7b81-42f2-b64a-6edb91286576). Should I update the uuid of nsIVolumeService.idl as well?
(In reply to jwwang from comment #14)
> My original thought was to separate the codes for test only and those for
> normal workflow. Therefore, I didn't add the isFake attribue to
> nsIVolume.idl since the caller woundn't have to know whether it is a real
> volume or not. Instead, it was passed around as an additional parameter to
> the functions. However, if the caller needs to know a fake volume, let's go
> the way as you suggested.

Yeah - I think that eventually, the callers will need to know. Most of the code won't need to know, but some will.

> Btw, can you explain about updating uuid? I checked the log and found out it
> was just revolving between uuid(3c9cae8d-9da2-4aa1-b8bf-4db8c8620808) and
> uuid(1134f267-7b81-42f2-b64a-6edb91286576). Should I update the uuid of
> nsIVolumeService.idl as well?

Everytime you change an .idl (other than just comments) you need to assign it a new uuid.

I normaly do:

/msg firebot uuid

in IRC.

I also realized that we don't have any way of creating a fake volume from JS, only from C++.
We'll also need to be able to amnipulate the attributes. So I think that this means we also need to remove readonly from the attributes in the JS, but put in checks in C++ that returns an eror if you try to manipulate one of the fields on a non-fake volume.
We should leave make the isFake and name attribute read-only and add add a new method to nsIVolumeService called

nsIVolume CreateFakeVolume(in DOMString volName);

which creates a fake volume. Then make mountPoint and state by non-readonly attributes.

I think that the mountGeneration should remain read-only and only increment when the state changes to mounted (as with the regular volume).

And we should mention in the comments that the mountPoint and state are only modifiable for fake volumes.
Actually, let's also add the mount point as a parameter to CreateFakeVolume, and the state is the only parameter which need to be changed.

Then we can remove updateFakeVolume from nsIVolumeService.idl (but leave it as a method which would be called by nsVolume::SetState
I am worried changing read-only attributes to writable ones in nsIVolume.idl would be an overkill in order for our test case.

We can add code to prevent modifying attributes on a non-fake volume, however this kinda mix logic for normal workflow with that for test case only. I would always like to separate them whenever possible to make code clear.

Here is my approach to support fake volumes in JS:
1. add a method to nsIVolumeService:
   nsIVolume createFakeVolume(in DOMString name, in DOMString path, in long state, in long mountGeneration, in boolean isMountLocked);
   This is a factory method to create a fake volume with specified attributes.

2. add a method to nsIVolumeService:
   void updateFakeVolume(in nsIVolume aVolume);
   By passing a volume created from step 1, we can change a fake volume managed by the volume service.

pros:
All logic about fake volume is kept in one place as much as possible.

cons:
Might have to copy logic from Volume::SetState in order to mutate mount generation correctly.
(In reply to jwwang from comment #18)
> I am worried changing read-only attributes to writable ones in nsIVolume.idl
> would be an overkill in order for our test case.
> 
> We can add code to prevent modifying attributes on a non-fake volume,
> however this kinda mix logic for normal workflow with that for test case
> only. I would always like to separate them whenever possible to make code
> clear.
> 
> Here is my approach to support fake volumes in JS:
> 1. add a method to nsIVolumeService:
>    nsIVolume createFakeVolume(in DOMString name, in DOMString path, in long
> state, in long mountGeneration, in boolean isMountLocked);
>    This is a factory method to create a fake volume with specified
> attributes.

I'd just pass in volumeName and mountPoint here. mountGeneration and isMountLocked is really internal state information that should be generated automatically.

> 2. add a method to nsIVolumeService:
>    void updateFakeVolume(in nsIVolume aVolume);
>    By passing a volume created from step 1, we can change a fake volume
> managed by the volume service.

I'd change this to be

void updateFakeVolume(in DOMString name, int long state);

Then we don't need to change any of the attributes, and we just pass in the information that can be changed.

We might want to call this SetFakeVolumeState instead of updateFakeVolume.
1. nsVolumeService::CreateFakeVolume
   When this is called in the child, it just forwards the request to the parent.
2. nsVolumeService::SetFakeVolumeState
   When this is called in the child, it just forwards the request to the parent.
3. nsVolumeService::CreateOrFindVolumeByName(const nsAString& aName, bool aIsFake = false);
   There is no way to tell if the return value is an existed or newly created volume. The 2nd paramenter 'aIsFake' is required to create a fake volume and the name is not found.
4. nsVolume::SetState(int32_t aState)
   Since sMountGeneration in Volume.cpp is not accessible, I have another generation number for fake volumes. Does it matter if fake volumes have a different series of generation numbers?
Attachment #756409 - Attachment is obsolete: true
Attachment #757829 - Flags: review?(dhylands)
(In reply to jwwang from comment #20)
> 4. nsVolume::SetState(int32_t aState)
>    Since sMountGeneration in Volume.cpp is not accessible, I have another
> generation number for fake volumes. Does it matter if fake volumes have a
> different series of generation numbers?

That should be fine. The mount generation only needs to be unique per volume.

It may take me a couple of days to do the review.
Comment on attachment 757829 [details] [diff] [review]
add functions to manipulate fake volumes to test SD card insert/remove

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

r=me with the comments addressed.

Sorry to take so long on the review.

::: dom/ipc/PContent.ipdl
@@ +471,5 @@
>      async SystemMessageHandled();
>  
> +    // called by the child (test code only) to propagate volume changes to the parent
> +    async CreateFakeVolume(nsString fsName, nsString mountPoint);
> +    async SetFakeVolumeState(nsString fsName, int32_t fsState); 

nit: Trailing space

::: dom/system/gonk/nsVolume.cpp
@@ +242,5 @@
> +{
> +  static int32_t sMountGeneration = 0;
> +
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

This function should only be called on fake volumes, so we should add

MOZ_ASSERT(IsFake());

::: dom/system/gonk/nsVolumeService.cpp
@@ +396,5 @@
> +    }
> +    if (!vol) {
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +    vol->SetState(state);

We should check the volume which was found, is in fact a fake volume, and fail to modify the state of a real volume.
Attachment #757829 - Flags: review?(dhylands) → review+
1. nsVolumeService::CreateFakeVolume
   When this is called in the child, it just forwards the request to the parent.
2. nsVolumeService::SetFakeVolumeState
   When this is called in the child, it just forwards the request to the parent.
3. nsVolumeService::CreateOrFindVolumeByName(const nsAString& aName, bool aIsFake = false);
   There is no way to tell if the return value is an existed or newly created volume. The 2nd paramenter 'aIsFake' is required to create a fake volume and the name is not found.
4. nsVolume::SetState(int32_t aState)
   Since sMountGeneration in Volume.cpp is not accessible, I have another generation number for fake volumes.
Attachment #757829 - Attachment is obsolete: true
Attachment #763409 - Flags: review+
Do we continue this bug with new test cases on devices storage or open a new bug?
You probably want to add at least one test case to this bug, just to verify that it has enough functionality to be used from a test case.
Attachment #764667 - Flags: review?(dhylands)
Comment on attachment 764667 [details] [diff] [review]
Part 2 - Add a test case to test new functions

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

Nice!

::: dom/system/gonk/tests/marionette/test_fakevolume.js
@@ +26,5 @@
> +is(Ci.nsIVolume.STATE_MOUNTED, vol.state, "state");
> +ok(vol.mountGeneration > oldMountGen, "mount generation should be incremented");
> +
> +finish();
> + 

nit: trailing space
Attachment #764667 - Flags: review?(dhylands) → review+
Remove trailing space.
Attachment #764667 - Attachment is obsolete: true
Attachment #771185 - Flags: review+
Remove trailing space.
Attachment #771185 - Attachment is obsolete: true
Attachment #771187 - Flags: review+
Add hg mq comments.
Attachment #763409 - Attachment is obsolete: true
Attachment #772521 - Flags: review+
Keywords: checkin-needed
Depends on: 898634
You need to log in before you can comment on or make changes to this bug.