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

RESOLVED FIXED in 2.0 S1 (9may)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rainwoo7, Assigned: hansu9866)

Tracking

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

5 years ago
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
Reporter

Updated

5 years ago
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Reporter

Comment 1

5 years ago
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.
Reporter

Updated

5 years ago
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?
Reporter

Updated

5 years ago
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)
Reporter

Comment 3

5 years ago
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)
Reporter

Updated

5 years ago
Flags: needinfo?(edilee)
Reporter

Updated

5 years ago
Flags: needinfo?(edilee)
Reporter

Updated

5 years ago
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.

Comment 5

5 years ago
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)
Reporter

Comment 6

5 years ago
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.
Reporter

Updated

5 years ago
Flags: needinfo?(fabrice)
Reporter

Comment 7

5 years ago
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)
Reporter

Comment 9

5 years ago
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 &&
Reporter

Updated

5 years ago
Flags: needinfo?(fabrice)
Woo, please submit a patch and ask dhylands to review. thanks!
Flags: needinfo?(fabrice)
Reporter

Comment 11

5 years ago
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)
Reporter

Comment 13

5 years ago
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 &&
Reporter

Updated

5 years ago
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)
Reporter

Comment 15

5 years ago
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?
Reporter

Updated

5 years ago
Flags: needinfo?(dhylands)
Almost.

You seem to be testing !== mounted, which it should be === mounted
Flags: needinfo?(dhylands)
Reporter

Comment 17

5 years ago
Thanks Dave.
That's correct, we test do that and that problem is solved.
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Assignee

Updated

5 years ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
hansu

What did you reopen this? please give details
Flags: needinfo?(hansu9866)
Assignee

Comment 19

5 years ago
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.
Assignee

Comment 20

5 years ago
Clear the needinfo
Assignee

Updated

5 years ago
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 :)
Assignee

Comment 23

5 years ago
(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.
Assignee

Comment 24

5 years ago
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+
Assignee

Updated

5 years ago
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
Assignee

Comment 26

5 years ago
(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
Assignee

Comment 28

5 years ago
(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.
Assignee

Comment 29

5 years ago
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: 5 years ago5 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.