949991 - [Download Manager] Nothing happens if you try to download a file while the device is connected as USB

Status: VERIFIED FIXED

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

Description

[rafael.marquez]

*Procedure
1. Connect the device to a computer as USB storage
2. Try Download an file

*Expected result
A message indicating that the memory is in use is displayed. You can view an explample trying make a screenshot with the device connected as USB storage

*Actual Result
 Nothing happens. The file is not downloades and any error message is displayed

Comment 1

[Cristian Rodriguez (:crdlc)]

Hi friends, how could we know it from Gaia? Is there any event ondownloadxxxx like ondownloadstart?

Comment 2

[Aus Lacroix [:aus]]

You can set an onstatechange handler on the download object itself. You receive the download object in the ondownloadstart event data.

On error, the state should be 'stopped' and the 'error' DOMError typed attribute should be set. 

Let me know if this doesn't work as it definitely should. :) It's possible though that we don't catch the lack of storage properly.

Comment 3

[Cristian Rodriguez (:crdlc)]

I think that we aren't receiving the ondownloadstart for those cases. When it will happen in that way we should support these king of messages in notifications. Thanks for your feedback

(In reply to Ghislain 'Aus' Lacroix from comment #2)
> You can set an onstatechange handler on the download object itself. You
> receive the download object in the ondownloadstart event data.
> 
> On error, the state should be 'stopped' and the 'error' DOMError typed
> attribute should be set. 
> 
> Let me know if this doesn't work as it definitely should. :) It's possible
> though that we don't catch the lack of storage properly.

Comment 4

[Cristian Rodriguez (:crdlc)]

Hi Ghislain, do you take a look to this problem? Thanks a lot mate

Comment 5

[rafael.marquez]

any update?

Comment 6

[Aus Lacroix [:aus]]

Hi guys, I have been able to reproduce this issue. I'll see if I can fix it this week.

Comment 7

[Preeti Raghunath(:Preeti)]

Likely a regression issue.

Comment 8

[Aus Lacroix [:aus]]

It's a new feature, it can't be a regression.

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I ran into a similar issue, was going to raise a bug, but I can't reproduce.

I saw the behavior described in comment 1. I then went to the downloads directly and deleted all the files in there, and then cleared everything existing already in the download list in the settings app.

Now I get downloads in the notification center, but never showing up in the settings app. If I figure out how to reproduce reliably I'll raise a separate bug or comment further here, but just wanted to note this somewhere. (At the moment I am not sure how to fix this, other than re-flashing, doing a full reset didnt work.)

Comment 10

[Cristian Rodriguez (:crdlc)]

Borja,

   Could you see why the settings app is not updated after removing all downloads?

Thanks

Comment 11

[Marcelino Veiga Tuimil [:sonmarce]]

Well, this is not a regression because download manager is a new feature for 1.4, but it is a really bad behavior for the user, download manager is not working because of phone being plugged via USB, and the user does not see any visual indication of the error, so he will think everything it is going well but download do not progress and no way to know the reason

Comment 12

[Aus Lacroix [:aus]]

It's quite possible this will end up being a blocker. But, from my point of view, the flag was set with the wrong reason. 

The API should either error out, or wait for storage to become available again to download the file. I believe it's easiest at this point to error out and that seems to be what's expected at this time based on how other applications that require sdcard access behave when it's not mounted.

Comment 13

[Borja Salguero [:borjasalguero]]

Regarding the download action, if the device is connected as USB Storage we can not delete the file properly, so probably we are not retrieving any error... Any idea about Gecko behaviour?

Comment 14

[Aus Lacroix [:aus]]

This is unfortunately a lot more serious of a problem than I first thought.

Here's what's going on (in chronological order).

* User presses a link in the browser
* The docshell calls the uri loader
* The uri loader determines we'll externally handle this url
* The external app service gets a request for handler for this content
* The external app handler eventually receives it's OnStartRequest callback
* In external app handler onstartrequest we will eventually attempt to get a temp file (SetUpTempFile) to output the bytes to (http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1614)
* SetUpTempFile calls GetDownloadDirectory which fails here: http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#351 because the media is not available.
* This error crawls back up the call stack to OnStartRequest where we simply cancel the request and say there was a write failure.

This failure does *not* get reported as an error to the outer <iframe> that originally triggered this so there's no way to tell the user that anything bad happened.
*

Comment 15

[Aus Lacroix [:aus]]

Attachment: WIP - Add nsIWebProgress flags for REQUEST.

WIP.

Comment 16

[Aus Lacroix [:aus]]

OK. Progress update.

With a few changes to BrowserElementChildPreload.js I've been able to receive an error back in the browser in gaia. This is what it looks like:

E/GeckoConsole(  525): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:351 in browser_handleBrowserEvent/<: browser.js > handleBrowserEvent > evt =  [object XrayWrapper [object CustomEvent]]
E/GeckoConsole(  525): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:352 in browser_handleBrowserEvent/<: browser.js > handleBrowserEvent > evt.detail =  [object Object]
E/GeckoConsole(  525): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:353 in browser_handleBrowserEvent/<: browser.js > handleBrowserEvent > evt.type =  mozbrowsererror
E/GeckoConsole(  525): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:504 in browser_handleBrowserEvent/<: browser > mozbrowsererror > 
E/GeckoConsole(  525): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:505 in browser_handleBrowserEvent/<:   detail =  {"type":"other","msg_name":"error"}

Not great but, it's a start. I'm not sure why, but, even with the debugging code, I'm not seeing any type of error status in BrowserElementChildPreload.js's _progressListener. So I don't know if my changes are really helping, or not.

Although, I think I'm basically getting this error:

        // TODO See nsDocShell::DisplayLoadError for a list of all the error
        // codes (the status param) we should eventually handle here.
        sendAsyncMsg('error', { type: 'other' });

Line 1101, in BrowserElementChildPreload.js. So, I think the next steps are to at least support the SDCardError that we're hopefully getting back.

Comment 17

[Aus Lacroix [:aus]]

Ben, does this make sense to you? If you have a chance, could you read through the bug and see if you agree with handling this error in the browser.

Comment 18

[Ben Francis [:benfrancis]]

Hi Aus,

I hope I've understood the problem correctly.

A download can be initiated from any app, right? So if the download manager (which is part of the system app) can not start the download because there is no storage space currently available, then surely this error should be reported to the user by the system app, via a notification, error dialog or similar? There doesn't seem to be anything specific to the browser app here?

The browser app currently manages the download of images, audio and video when long pressing on an HTML element and handles similar types of errors from the Device Storage API and displays an error to the user at the bottom of the screen which disappears after three seconds https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/js/browser.js#L963 As the system app is now responsible for handling downloads, I think this code should be moved there.

To be honest I'm surprised to see that this code still exists in the browser, because I would have thought that long pressing an img/audio/video element to save it would be handled by the download manager by now. Do we have a bug tracking this?

Comment 19

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Return NS_ERROR_FILE_ACCESS_DENIED instead of NS_ERROR_FAILURE

This ensures that the prompter gets called and that the error is known to the user within the context they attmepted to trigger the download.

Comment 20

[Aus Lacroix [:aus]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #19)
> Created attachment 8385699 [details] [diff] [review]
> Patch - v1 - Return NS_ERROR_FILE_ACCESS_DENIED instead of NS_ERROR_FAILURE
> 
> This ensures that the prompter gets called and that the error is known to
> the user within the context they attmepted to trigger the download.

The patch will cause nsExternalAppHandler::SendStatusChange (http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1756) to set the msgId of the error to the correct value and show the prompt to the user via the nsIPrompt obtained from the InterfaceRequester of the current window context we're in.

Comment 21

[Aus Lacroix [:aus]]

Comment on attachment 8385699 [details] [diff] [review]
Patch - v1 - Return NS_ERROR_FILE_ACCESS_DENIED instead of NS_ERROR_FAILURE

I'm still running into some problems with this patch. Often, the prompter is null.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8385699 [details] [diff] [review]
Patch - v1 - Return NS_ERROR_FILE_ACCESS_DENIED instead of NS_ERROR_FAILURE

I'm all in favor of more informative error codes than NS_ERROR_FAILURE.

NS_ERROR_FILE_ACCESS_DENIED sounds like a permissions error, though.  Do we not have an error code to indicate lack of space?  Or can we not differentiate that from a permissions failure here?

Comment 23

[Aus Lacroix [:aus]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #22)
> Comment on attachment 8385699 [details] [diff] [review]
> Patch - v1 - Return NS_ERROR_FILE_ACCESS_DENIED instead of NS_ERROR_FAILURE
> 
> I'm all in favor of more informative error codes than NS_ERROR_FAILURE.
> 
> NS_ERROR_FILE_ACCESS_DENIED sounds like a permissions error, though.  Do we
> not have an error code to indicate lack of space?  Or can we not
> differentiate that from a permissions failure here?

In this case, it's somewhat the same as a permission error as the SD card is not mounted (the volume itself, is inaccessible because it's not mounted on the local filesystem but instead mounted to the computer via USB).

This change also mirrors what we do on Firefox for Android when there is lack of SD card (not mounted locally).

The main issue I'm running into is that the interface requester set on the external app handler does not return a prompter, most of the time. But, I feel like it should since it is in the context of the currently active docshell, however, the external helper app service does create a new load group and retargets the binding of the requests before OnStartRequest is called on the handler. Maybe this causes a disconnect somewhere? Do we lose access to the window and therefor are unable to create prompter?

Any pointers here would be very helpful. I'm learning how all this code works as I'm going.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> The main issue I'm running into is that the interface requester set on the external app
> handler does not return a prompter, most of the time.

What concrete object is that interface requestor?

> Maybe this causes a disconnect somewhere?

It's possible, yes.  Certainly nsExternalAppHandler::RetargetLoadNotifications nukes all the notification callbacks bits on the channel...

Comment 25

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Return more descriptive error code, use window docshell to get prompter if interface requestor fails to return a prompt.

I think this is the right way to go about this based on talking with mrbkap.

Comment 26

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Return more descriptive error code, use window docshell to get prompter if interface requestor fails to return a prompt.

* Return more descriptive (NS_ERROR_FILE_ACCESS_DENIED) when calling
  GetDownloadDirectory while the sd card (or on device storage) is not
  available because it is either not present or it is mounted to a host
  computer.

* If no prompt is returned by the interface request (mWindowContext) in
  the nsExternalAppHelper object we will attempt to get the doc shell from
  the current window (by asking mWindowContext) and then ask the doc shell
  for a prompt instead.

* editor stripped out all extra whitespace present. sorry, it's annoying
  but good that it did it. :)

Comment 27

[Aus Lacroix [:aus]]

I'm uncertain about the legitimacy of this fix for environments other than FxOS. That's why the larger change is #ifdef'd for MOZ_WIDGET_GONK only.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386439 [details] [diff] [review]
Patch - v2 - Return more descriptive error code, use window docshell to get prompter if interface requestor fails to return a prompt.

Would you mind posting a patch without the random whitespace changes?

Comment 29

[rafael.marquez]

EL sprint ends next week and I want to have closed this functionality. Do you know when will be landed in master the solution?

Comment 30

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Same patch, minus the whitespace changes.

Here we go, should make it a lot easier to read. :)

Comment 31

[Boris Zbarsky [:bzbarsky]]

> it's annoying but good that it did it. :)

Random cleanup should not be happening in the same changeset as substantive changes.  Please feel free to land the cleanup as a separate changeset, though.

Comment 32

[Aus Lacroix [:aus]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #31)
> > it's annoying but good that it did it. :)
> 
> Random cleanup should not be happening in the same changeset as substantive
> changes.  Please feel free to land the cleanup as a separate changeset,
> though.

Not a problem. I'll make sure what lands is only what is in the -w version of the patch.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386962 [details] [diff] [review]
Patch - v2 - Same patch, minus the whitespace changes.

I don't think there should be anything Gonk-specific here.

That said, what exactly is mWindowContext when you can't get an nsIPrompt from it?  Is it basically sometimes a docshell (when you can get an nsIPrompt) and sometimes a window (when you can't)?

Comment 34

[Aus Lacroix [:aus]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #33)
> Comment on attachment 8386962 [details] [diff] [review]
> Patch - v2 - Same patch, minus the whitespace changes.
> 
> I don't think there should be anything Gonk-specific here.
> 
> That said, what exactly is mWindowContext when you can't get an nsIPrompt
> from it?  Is it basically sometimes a docshell (when you can get an
> nsIPrompt) and sometimes a window (when you can't)?

Yes, that's what I observed during testing. I made the change gonk specific because the issue doesn't seem to arise elsewhere, but, if it feels like an overall safe-change, I'm happy to remove that #ifdef.

Comment 35

[Boris Zbarsky [:bzbarsky]]

It should be able to arise elsewhere any time nsExternalAppHandler::MaybeCloseWindow is called, no?

I think we should really fix that code to use the docshell of the opener window, not the opener window, as the context.

Also, maybe window should just getInterface to prompt instead of working around it here?  If we do the workaround, please do avoid having two separate alert() calls.  Just if (!prompt) { /* get it the other way */ }

Comment 36

[Aus Lacroix [:aus]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #35)
> It should be able to arise elsewhere any time
> nsExternalAppHandler::MaybeCloseWindow is called, no?

Ah, yes. That's true. It would be better then to make this a general solution. I've gone ahead and removed the #ifdef.

> 
> I think we should really fix that code to use the docshell of the opener
> window, not the opener window, as the context.

That'd be awesome, but I'm not sure how I would go about ensuring that we're always using the docshell of the opener window as the context. Maybe this would be a different bug to cover that work?

> Also, maybe window should just getInterface to prompt instead of working
> around it here?  If we do the workaround, please do avoid having two
> separate alert() calls.  Just if (!prompt) { /* get it the other way */ }

I restructured the code so that there would only be one call to Alert(). New patch shortly.

Comment 37

[Aus Lacroix [:aus]]

Attachment: Patch - v3 -

Updated patch with first round of changes based on review feedback.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Maybe this would be a different bug to cover that work?

That seems fine, yes.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386990 [details] [diff] [review]
Patch - v3 -

r=me

Comment 40

[Aus Lacroix [:aus]]

Attachment: Patch - Final

r+ from bz carries.

Comment 41

[Aus Lacroix [:aus]]

Attachment: Patch - Final

r+ carries from bz.

Comment 42

[Gregor Wagner [:gwagner]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

Please, please land cross-platform core gecko patches on mozilla-inbound...

Comment 44

[Gregor Wagner [:gwagner]]

backed out on b2g-inbound
https://hg.mozilla.org/integration/b2g-inbound/rev/f6da4c229476

landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/422267c7c845

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/422267c7c845

Comment 46

[rafael.marquez]

The bug 949991 has been fixed but the bug 965159 is still open. Can you check the bug 965159?

Comment 47

[rafael.marquez]

If I try to download a file with the device connected in USB mode, the device displays an error message which I think is not correct. 

Message displayed: Error displayed.png (attached image)

Expected message: error expected.png (attached image)

Francis, can you verify which is the correct message?

Comment 48

[rafael.marquez]

Attachment: Error displayed.png

Comment 49

[rafael.marquez]

Attachment: Error expected.png

Comment 50

[Aus Lacroix [:aus]]

Unfortunately it's not trivial to change that error message and if it becomes necessary it should be done as a separate bug. Could we just go ahead and file a different bug instead of discussing this here? 

The current error message is what gecko uses for all platforms.

Comment 51

[Francis Djabri [:djabber]]

(In reply to rafael.marquez from comment #49)
> Created attachment 8388400 [details]
> Error expected.png

This dialog is definitely preferable, as it follows the styling correctly and also gives the user instructions about a potential fix. Better still would be "Unplug the phone and try downloading again".

Comment 52

[rafael.marquez]

I have opened the bug 982006 to follow the wrong message

Comment 53

[rafael.marquez]

Bug fixed in v1.4 and v1.5(master), but I have found a new bug while the device is connected in USB mode. bug 992766