1007965 - [B2G][1.4][Flame][Settings] The error message "File not found" appears when trying to open a supported file.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Yeojin Chung [:YeojinC]]

Attachment: FileNotFound.png

Description:
The error message "File not found" appears when trying to open a supported file. 

Repro Steps:
1) Updated Flame to BuildID: 20140508000201
2) Download a file from the browser.
3) Select "Settings" from the home screen.
4) Select "Downloads".
5) Select the downloaded file.
6) Select "Open", "Share", or any other options.
7) Observe the message "File not found": "The file wasn't found on the device". 

Actual: The error message "File not found. The file wasn't found on the device" appears. 

Expected: The user is able to proceed with the selected option.

1.4 Environmental Variables:
Device: Flame 1.4 MOZ
BuildID: 20140508000201
Gaia: 4ce973ef0732b0d52cb043210db598aa176b2ce9
Gecko: 16ab7f6b18f8
Version: 30.0
Firmware Version: v10E

Repro frequency: 100%
See attached: Screenshot, logcat

Comment 1

[Yeojin Chung [:YeojinC]]

Attachment: Logcat

This issue does not occur in the Open C 1.4.

1.4 Environmental Variables:
Device: Open_C 1.4
BuildID: 20140508000201
Gaia: 4ce973ef0732b0d52cb043210db598aa176b2ce9
Gecko: 16ab7f6b18f8
Version: 30.0
Firmware Version: P821A10-ENG_20140410
Base: FFOS_US_EBAY_P821A10V1.0.0B06_LOG_DL

Comment 2

[Yeojin Chung [:YeojinC]]

This issue does not occur in the Buri 1.4.

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140502000201
Gaia: 7b2b82d72cbdd1c7e0f4542cb3390802e65f473e
Gecko: 50be03cea340
Version: 30.0
Firmware Version: v1.2-device.cfg

Comment 3

[Jason Smith [:jsmith]]

So it's a vendor bug, but why?

Dave - Do you know?

Comment 4

[Mike Lien[:mlien]]

Flame is shipped with v1.3. this is agree in the contract. 
for anything happened on v1.4, we need to make sure it's not FxOS related.
please investigate internally first

btw, please help to provide more detailed about download file information, e.g., which file, where to download (because I cannot reproduce it with my Flame on v10F-3 OEM + v1.4 gaia/gecko)

Comment 5

[Jason Smith [:jsmith]]

Leaving qawanted to indicate what was downloaded here.

Comment 6

[Yeojin Chung [:YeojinC]]

I downloaded mp3, jpg, vcf, zip files to the SD card. When I attempted to open zip files, the "Unable to open" message was displayed as usual. The mp3 files worked fine from the Music app, but not from the Downloads.

Comment 7

[Jason Smith [:jsmith]]

We saw this problem during the bug bash as well. With pictures, we saw this error occur if you opened the file from the settings app. However, if you tried to do a pick activity from the gallery app, then you would see the file. Which makes me think that this isn't a vendor problem - instead, it's an issue that occurs due to differences in how storage is setup across different devices.

Comment 8

[Dave Hylands [:dhylands]]

So, this may depend on exactly how the Download app or settings app is opening device storage.

If you use navigator.getDeviceStorage('sdcard') then you can open any file with any extension. If, on the other hand, you use any other type (than 'sdcard') then you can only open files which have an extension supported by that type.

The list of extensions for each type can be found here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/content/devicestorage.properties?from=devicestorage.properties#1

I do hope that the download app is, in fact, using device storage. If it isn't then that may also explain the problem (since newer versions of android have tighter security and you can no longer access files directly).

Comment 9

[Preeti Raghunath(:Preeti)]

Gregor,

Can you please review?

Comment 10

[howie [:howie]]

download related, change component to gaia system. please re-assign if not the case, thanks.

Comment 11

[Gregor Wagner [:gwagner]]

Aus, can you take a look here?

Comment 12

[Aus Lacroix [:aus]]

I know exactly what's going on with this bug. I'll take care of it this week.

Comment 13

[Aus Lacroix [:aus]]

So, what's happening here is that the DownloadHelper object we're using to open the file relies on the fact that the path to the "downloads" folder should have "sdcard" as it's parent. This is *not* the case for the flame. On the flame the parent folder is "sdcard0".

This causes this bit of code here (https://github.com/mozilla-b2g/gaia/blob/master/shared/js/download/download_helper.js#L105) to extract the incorrect relative path to fetch the blob from Device Storage to take action on the content (view, listen, watch, etc). This occurs because it gets passed in 'sdcard' which it finds but then, fails to cut out the rest of the folder name.

It is my understanding that the Device Storage API name "sdcard" is an opaque identifier that doesn't necessarily map to one exact physical location. Because of this, I think we should actually change the Downloads API Download Object to also return the storage name and relative path from storage root where the download resides *as well as* the full path in case an application requires it. This will allow the helper to fetch the blob easily for all content (and for other applications, too!)

Comment 14

[Aus Lacroix [:aus]]

:dhylands, are my assumptions correct in comment 13? (https://bugzilla.mozilla.org/show_bug.cgi?id=1007965#c13)

Comment 15

[Dave Hylands [:dhylands]]

In DeviceStorage, fully qualified names look like:

/storageName/relativePath/filename

The storageName is "virtual". Its assumed to be a volume name and the volume mount point is substituted to come up with the real filename.

You can see the volume mount points by doing:

> adb shell vdc volume list

In C++ code, you can do:
http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp?from=nsDeviceStorage.cpp#832

and volMountPoint will have the volume mount point.

Note: That reading/writing to the volumes requires root access.

Comment 16

[Aus Lacroix [:aus]]

I'm also considering having the Download object simply have a blob that is set when the download completes. This may be the best way to go but I need to check first what implications this may have on low resource devices.

Comment 17

[Aus Lacroix [:aus]]

After reading through our Blob / File implementation I've decided it's probably best to only supply the Storage Name and Path on said Storage instead of providing a Blob. The Blob has buffers it allocates via an input stream to read from the file itself. We might as well avoid this until we know we're going to do something with it.

Comment 18

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Provide DeviceStorage information as part of Download properties.

Comment 19

[Aus Lacroix [:aus]]

Attachment: Pull Request - DownloadHelper should use new storageName and storagePath properties. DownloadStore should save storageName and storagePath for later use.

Comment 20

[Aus Lacroix [:aus]]

Try Run: https://tbpl.mozilla.org/?tree=Try&rev=f5c1ea80ca2b

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 8425892 [details] [diff] [review]
Patch - v1 - Provide DeviceStorage information as part of Download properties.

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

r=me with nits addressed.

Boris, do you mind r+'ing the webidl changes? thanks!

::: dom/downloads/src/DownloadsAPI.js
@@ +326,5 @@
>      });
>  
> +    // When the path changes, we should update the storage name and
> +    // storage path used for our downloaded file in case our download
> +    // was re-targetted to a different storage and/or filename.

can that really happen except on first call to _update() ?

@@ +346,5 @@
> +          // Finally, create the relative path of the file that can be used
> +          // later on to retrieve the file via DeviceStorage. Our path
> +          // needs to omit the starting '/'.
> +          this.storageName = preferredStorageName;
> +          this.storagePath = 

nit: trailing whitespace.

@@ +414,5 @@
>    },
>  
> +  _updateWithStorageInformation: function(aDownload) {
> +    
> +  },

That's unused, right?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8425892 [details] [diff] [review]
Patch - v1 - Provide DeviceStorage information as part of Download properties.

r=me on the webidl bits

Comment 23

[Aus Lacroix [:aus]]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> Comment on attachment 8425892 [details] [diff] [review]
> Patch - v1 - Provide DeviceStorage information as part of Download
> properties.
> 
> Review of attachment 8425892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed.
> 
> Boris, do you mind r+'ing the webidl changes? thanks!
> 
> ::: dom/downloads/src/DownloadsAPI.js
> @@ +326,5 @@
> >      });
> >  
> > +    // When the path changes, we should update the storage name and
> > +    // storage path used for our downloaded file in case our download
> > +    // was re-targetted to a different storage and/or filename.
> 
> can that really happen except on first call to _update() ?

In theory, as things flow currently, it shouldn't happen but there's no harm in supporting that case since we would have to check if we need to initialize storageName and storagePath anyway.

> 
> @@ +414,5 @@
> >    },
> >  
> > +  _updateWithStorageInformation: function(aDownload) {
> > +    
> > +  },
> 
> That's unused, right?

Whoops! Yes, I'll remove that.

Comment 24

[Aus Lacroix [:aus]]

Try run looks good as well, just need to make sure it builds on JB Debug and KK Debug (looks like infra failure). I'll post the final patch real soon! Thanks guys!

Comment 25

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Provide DeviceStorage information as part of Download properties.

Review comments addressed. r+ from fabrice and bz carries.

Comment 26

[Aus Lacroix [:aus]]

Try run looks good! JB and KK debug builds are fine. Final patch is ready to land.

Comment 27

[Aus Lacroix [:aus]]

Push to b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/f8c15df3a068

Leaving open until Gaia changes land.

Comment 28

[Aus Lacroix [:aus]]

Apologies, my initial push to b2g-inbound had a bad commit message (it didn't fit on one line). I backed out that commit and have committed a new revision with the correct commit message.

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

Comment 29

[Aus Lacroix [:aus]]

Hooray! I can check-in my own stuff!

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8425898 [details] [review]
Pull Request - DownloadHelper should use new storageName and storagePath properties. DownloadStore should save storageName and storagePath for later use.

LGTM, good job! Thanks a lot

Comment 32

[Aus Lacroix [:aus]]

Gaia commit (master): https://github.com/mozilla-b2g/gaia/commit/69790f4d8f787b8f4dbda4a9eb2c03bff254b3eb

Comment 33

[Aus Lacroix [:aus]]

Gaia commit (v1.4): https://github.com/mozilla-b2g/gaia/commit/7709936aeb21859d1607dbd038489493803bb085

Gecko patch will land on b2g30 this evening.

Comment 34

[Aus Lacroix [:aus]]

And now you can open downloads on the flame, woohoo!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/0c8e7c2ea9ec