Closed Bug 991977 Opened 10 years ago Closed 10 years ago

Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: rainwoo7, Assigned: hansu9866)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.154 Safari/537.36

Steps to reproduce:

1) Connect to test url: http://nactsft.net -> MMS -> audio-> mp3
2) Try to download 




Actual results:

Can not download any content on Internal storage
Actually,there are no action when user try to link click


Expected results:

These content have to downloaded on Internal storage
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Cause of problem which contents can't downloaded in internal storage is checking mount lock of volume.
when Mountlock is true, can't download contents.

That can be found _getDefaultDownloadDirectory() api below code.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm

So, we opened issue why mount lock of internal storage is set by true?
PLz Look at below bug No.989894 and you can see detail information.
-> https://bugzilla.mozilla.org/show_bug.cgi?id=989894

During we discussed that issue,
and then we want know why download toolkit in browser component check mount lock of Volume(storage).

Mount lock of volume is only used to prevent that volume is being shared with PC.
As we see, mount lock is not checked in toolkit for download contents.
Summary: Why mount lock of Volume(storage) is checked during download contents? → Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?
Flags: needinfo?(wchang)
Flags: needinfo?(dhylands)
Sorry - I don't understand what you're asking me for (needinfo).

Also - please needinfo with my mozilla email.
Flags: needinfo?(dhylands)
To Dave Hylands
As I think, this issue need your comment about mount lock of volume.
So I requested you.

To Wain Chang
Could you arrange browser member for this issue?
I heard about you are manager of mozilla.
Flags: needinfo?(wchang)
Flags: needinfo?(edilee)
Flags: needinfo?(edilee)
Flags: needinfo?(edilee)
I made lots of comments about mount locks in bug 989894

What is your specific question? I still don't know what you're trying to ask me.
Woo, if you look at the "blame" of the code you linked, you'll see that it was implemented by bug 936342 with this changeset http://hg.mozilla.org/mozilla-central/rev/9e31c9c04ccf

blame: http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289
Flags: needinfo?(edilee)
I checked below code as you said, and I want to know why mount lock is used in _getDefaultDownloadDirectory()
http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289

As I understand, below code is't necessary in _getDefaultDownloadDirectory().
-> L289 :  !volume.isMountLocked &&

Because Mount lock of volume is only used to prevent that volume is being shared with PC.
Flags: needinfo?(fabrice)
To fabrice,

We had problem which can't download contents in browser.

That is caused by checking mount lock of volume in _getDefaultDownloadDirectory()
http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289

As I understand, below code is't necessary in _getDefaultDownloadDirectory().
-> L289 :  !volume.isMountLocked &&

Could you tell me about reason checking mount lock?
Can we write to the sdcard if it's mount locked?
Flags: needinfo?(fabrice)
Yes, we could do that,
mount lock of volume is only used to prevent that volume is being shared with PC.
So, if we try to write on the sdcard, we don't need to check whether volume is mount locked.

As a result, below code is needed to remove.
As I understand, below code is't necessary in _getDefaultDownloadDirectory().
-> L289 :  !volume.isMountLocked &&
Flags: needinfo?(fabrice)
Woo, please submit a patch and ask dhylands to review. thanks!
Flags: needinfo?(fabrice)
To, dhylands

Could I patch like below?

http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#l289
As a result, below code will be removed in _getDefaultDownloadDirectory().
-> L289 :  !volume.isMountLocked &&
Flags: needinfo?(dhylands)
That isn't sufficient.

For example, the volume could be unmounted, or it could be being formatted.

If you're going to use the volume object directly, then you should check that the state is equal to mounted, like this:
http://dxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#137

mounted is the only state where you can read/write the volume and it implies !sharing and media present.

It would be preferable not to use the volume stuff at all and to use device storage to perform the I/O.

When running under b2g-desktop for example, none of the volume API is available (but the device storage api is available).

Using device storage, you should check storage.available() returns "available".
Flags: needinfo?(dhylands)
As you said, I agree to need to checking !sharing and media preset.
and I can see that below in getDefaultDownloadDirectory() at DownloadIntegration.jsm

287        if (volume &&
288            volume.isMediaPresent &&
289            !volume.isMountLocked &&
290            !volume.isSharing) {

That is implemented as you mentioned, but !volume.isMountLocked is not necessary.
As a result, there is no problem about removing below code which check mount lock.
-> ##  !volume.isMountLocked &&
Flags: needinfo?(dhylands)
Please see comment 12

You shouldn't check isMediaPresent and you shouldn't check isSharing.

You SHOULD check that the status is MOUNTED. This means that the volume is accessible to FxOS (and it implies isMediaPresent and it also implies !isSharing)

The test from comment 13 would incorrectly pass if the volume state is 1, 2, 3, 5, or 6.
See http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIVolume.idl#13 for the various states.
Flags: needinfo?(dhylands)
Okay, as you mentioned,
I understand, We should check whether state of volume is MOUNTED.

if (preferredStorageName) {
   let volume = volumeService.getVolumeByName(preferredStorageName);
   if (volume && volume.state !== Ci.nsIVolume.STATE_MOUNTED) {
	  directoryPath = OS.Path.join(volume.mountPoint, "downloads");
	  yield OS.File.makeDir(directoryPath, { ignoreExisting: true });
	}
}

Is it right?
Flags: needinfo?(dhylands)
Almost.

You seem to be testing !== mounted, which it should be === mounted
Flags: needinfo?(dhylands)
Thanks Dave.
That's correct, we test do that and that problem is solved.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
hansu

What did you reopen this? please give details
Flags: needinfo?(hansu9866)
The patch proposed by Woo fixed the problem, and I understand that the comment 16 was a r+. 
So we should uplift this patch before changing to resolved.
Clear the needinfo
Flags: needinfo?(hansu9866)
(In reply to hansu9866 from comment #19)
> The patch proposed by Woo fixed the problem, and I understand that the
> comment 16 was a r+. 
> So we should uplift this patch before changing to resolved.

In which case, I think you'll need to upload a proper patch and request for review :)
(In reply to Wayne Chang [:wchang] from comment #21)
> (In reply to hansu9866 from comment #19)
> > The patch proposed by Woo fixed the problem, and I understand that the
> > comment 16 was a r+. 
> > So we should uplift this patch before changing to resolved.
> 
> In which case, I think you'll need to upload a proper patch and request for
> review :)

OK, thanks your guide.
Comment on attachment 8411457 [details] [diff] [review]
Modify DownloadIntegration.jsm file for volume check condition

To. Dave
I upload the patch.
Could you check it for review?
It is based on your comment.
Thanks.
Attachment #8411457 - Flags: review?(dhylands)
Attachment #8411457 - Flags: review?(dhylands) → review+
Keywords: checkin-needed
Thanks for the patch! Can you please add commit information to this patch so I can land it? Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee: nobody → hansu9866
Flags: needinfo?(hansu9866)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> Thanks for the patch! Can you please add commit information to this patch so
> I can land it? Thanks!
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Thanks for your guide.
Is it possible?

Author name:
Hansu Kim <hansu9866@gmail.com>
Commit message:
"Bug 991977 - Why mount lock of Volume(storage) is checked during download contents in Browser Toolkit?; r=dhylands".
Flags: needinfo?(hansu9866)
(In reply to hansu9866 from comment #26)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> > Thanks for the patch! Can you please add commit information to this patch so
> > I can land it? Thanks!
> > https://developer.mozilla.org/en-US/docs/
> > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> > in_for_me.3F
> 
> Thanks for your guide.
> Is it possible?
> 
> Author name:
> Hansu Kim <hansu9866@gmail.com>
> Commit message:
> "Bug 991977 - Why mount lock of Volume(storage) is checked during download
> contents in Browser Toolkit?; r=dhylands".

You checkin comment should describe the fix (which may not bear any resemblence to the original reported problem). A better comment would something along the lines of:

Bug 991977 - Ensure volume is mounted before starting download. r=dhylands
(In reply to Dave Hylands [:dhylands] from comment #27)
> (In reply to hansu9866 from comment #26)
> (In reply to Ryan VanderMeulen
> [:RyanVM UTC-4] from comment #25)
> > Thanks for the patch! Can you please
> add commit information to this patch so
> > I can land it? Thanks!
> >
> https://developer.mozilla.org/en-US/docs/
> >
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> >
> in_for_me.3F
> 
> Thanks for your guide.
> Is it possible?
> 
> Author name:
> > Hansu Kim <hansu9866@gmail.com>
> Commit message:
> "Bug 991977 - Why
> mount lock of Volume(storage) is checked during download
> contents in
> Browser Toolkit?; r=dhylands".

You checkin comment should describe the fix
> (which may not bear any resemblence to the original reported problem). A
> better comment would something along the lines of:

Bug 991977 - Ensure
> volume is mounted before starting download. r=dhylands

Thanks, I'm agree, it is the better.
Dear RyanVM UTC-4,
Is it possible?

Author name:
Hansu Kim <hansu9866@gmail.com>
Commit message:
Bug 991977 - Ensure volume is mounted before starting download. r=dhylands
Flags: needinfo?(ryanvm)
Sounds great, thanks :-)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5d205aa662c
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.