Closed Bug 949991 Opened 8 years ago Closed 7 years ago

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

Categories

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

Other
Other
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed)

VERIFIED FIXED
1.4 S3 (14mar)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: rafael.marquez, Assigned: aus)

References

(Blocks 1 open bug)

Details

(Whiteboard: [systemsfe][p=8])

Attachments

(3 files, 7 obsolete files)

*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
blocking-b2g: --- → 1.4?
Whiteboard: [systemsfe]
Hi friends, how could we know it from Gaia? Is there any event ondownloadxxxx like ondownloadstart?
Component: Gaia::System → General
Flags: needinfo?(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.
Flags: needinfo?(aus)
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.
Hi Ghislain, do you take a look to this problem? Thanks a lot mate
any update?
Flags: needinfo?(aus)
Hi guys, I have been able to reproduce this issue. I'll see if I can fix it this week.
Status: NEW → ASSIGNED
Flags: needinfo?(aus)
Whiteboard: [systemsfe] → [systemsfe][p=3]
Likely a regression issue.
blocking-b2g: 1.4? → 1.4+
Component: General → Gaia::System
It's a new feature, it can't be a regression.
blocking-b2g: 1.4+ → 1.4?
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.)
Borja,

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

Thanks
Flags: needinfo?(borja.bugzilla)
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
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.
Target Milestone: --- → 1.4 S1 (14feb)
Blocks: 965159
Target Milestone: 1.4 S1 (14feb) → ---
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?
Flags: needinfo?(borja.bugzilla)
blocking-b2g: 1.4? → 1.4+
Assignee: nobody → 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.
*
WIP.
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.
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.
Flags: needinfo?(bfrancis)
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?
Flags: needinfo?(bfrancis)
Blocks: 977094
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.
Attachment #8383990 - Attachment is obsolete: true
Attachment #8385699 - Flags: review?(bzbarsky)
(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 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.
Attachment #8385699 - Flags: review?(bzbarsky) → feedback?(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?
Attachment #8385699 - Flags: feedback?(bzbarsky) → feedback+
(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.
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
I think this is the right way to go about this based on talking with mrbkap.
Attachment #8385699 - Attachment is obsolete: true
Whiteboard: [systemsfe][p=3] → [systemsfe][p=8]
* 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. :)
Attachment #8386434 - Attachment is obsolete: true
Attachment #8386439 - Flags: review?(jst)
Attachment #8386439 - Flags: review?(bzbarsky)
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 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?
Attachment #8386439 - Flags: review?(bzbarsky)
EL sprint ends next week and I want to have closed this functionality. Do you know when will be landed in master the solution?
Here we go, should make it a lot easier to read. :)
Attachment #8386962 - Flags: review?(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.
(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 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)?
(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.
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 */ }
(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.
Attached patch Patch - v3 - (obsolete) — Splinter Review
Updated patch with first round of changes based on review feedback.
Attachment #8386439 - Attachment is obsolete: true
Attachment #8386962 - Attachment is obsolete: true
Attachment #8386439 - Flags: review?(jst)
Attachment #8386962 - Flags: review?(bzbarsky)
Attachment #8386990 - Flags: review?(jst)
Attachment #8386990 - Flags: review?(bzbarsky)
> Maybe this would be a different bug to cover that work?

That seems fine, yes.
Comment on attachment 8386990 [details] [diff] [review]
Patch - v3 -

r=me
Attachment #8386990 - Flags: review?(bzbarsky) → review+
Attached patch Patch - Final (obsolete) — Splinter Review
r+ from bz carries.
Attachment #8386990 - Attachment is obsolete: true
Attachment #8386990 - Flags: review?(jst)
Attachment #8387026 - Flags: review+
Keywords: checkin-needed
Attached patch Patch - FinalSplinter Review
r+ carries from bz.
Attachment #8387026 - Attachment is obsolete: true
Attachment #8387034 - Flags: review+
Blocks: 980505
Please, please land cross-platform core gecko patches on mozilla-inbound...
https://hg.mozilla.org/mozilla-central/rev/422267c7c845
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The bug 949991 has been fixed but the bug 965159 is still open. Can you check the bug 965159?
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?
Flags: needinfo?(fdjabri)
Attached image Error displayed.png
Attached image Error expected.png
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.
(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".
Flags: needinfo?(fdjabri)
I have opened the bug 982006 to follow the wrong message
Target Milestone: --- → 1.4 S3 (14mar)
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.