Closed Bug 992766 Opened 6 years ago Closed 6 years ago

[Download Manager] A wrong message is displayed if you try to retry a failed download, while the device is connected as USB

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: rafael.marquez, Assigned: aus)

References

Details

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

Attachments

(2 files, 3 obsolete files)

Attached image Reintento USB.png
*Procedure
1. Connect the device to a computer as USB storage
2. Try Download an file
The download fails and A message indicating that the memory is in use is displayed
3. Open settings / Downloads
The file is displayed as failed
4.Click on Retry button and press try again

Expected Result
A message indicating that the memory is in use is displayed

Actual Result
A message indicating that the memory card is full is displayed. (See attached file)


Bug reproduced in v1.4 and v1.5 (master)
blocking-b2g: --- → 1.4?
QA Contact: rafael.marquez
Whiteboard: [systemsfe]
OS: Other → Gonk (Firefox OS)
Hardware: Other → ARM
Checking it out...
Assignee: nobody → aus
Status: NEW → ASSIGNED
blocking-b2g: 1.4? → 1.4+
Working on a fix now that would be in the downloads api and would infer the error based on the state of the device storage and a certain error code from the js downloads api. 

Specifically, in cases where we get a generic failure (NS_ERROR_FAILURE) from the js downloads api we will go through a series of extra sanity checks to try and pin point the real cause of failure (one that is actionable, preferably).

Should be ready shortly. No gaia changes are required.
Note that there is a small cosmetic code style fix in there too, I'm happy to take that out if it doesn't feel like it should land as part of this.
Whiteboard: [systemsfe] → [systemsfe][p=3]
Target Milestone: --- → 1.4 S5 (11apr)
Attachment #8403578 - Flags: review?(fabrice)
Tested on Peak and Keon, works as expected.
Comment on attachment 8403578 [details] [diff] [review]
Patch - v1 - Infer error when DeviceStorage API is available and generic error code is recieved from jsdownloads

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

::: dom/downloads/src/DownloadsAPI.js
@@ +301,5 @@
>  
>    /**
>      * Updates the state of the object and fires the statechange event.
>      */
> +  _update: function(aDownload, aInferredError) {

I don't like that you call the same function with two different contexts based on aInferredError. Can you instead split it in two functions?
keep _update(aDownload) from there call a _updateWithError(aDownload, aError)

@@ +324,5 @@
> +      // When we get a generic error failure back from the js downloads api
> +      // we will verify the status of device storage to see if we can't provide
> +      // a better error result value.
> +      //
> +      // XXX If these checks expand further, consider moving them into their 

nit: trailing whitespace.
Attachment #8403578 - Flags: review?(fabrice) → review-
Patch v2 tested on GP Peak.
Attachment #8403697 - Attachment is obsolete: true
Attachment #8403697 - Flags: review?(fabrice)
Attachment #8403699 - Flags: review?(fabrice)
This check is superfluous -- if we're in agreement, I would rather remove it (but originally put it in because we do this in _update).

This is from _upddateWithError

+    if (this.id != aDownload.id) {
+      return;
+    }
Comment on attachment 8403699 [details] [diff] [review]
Patch - v2 - Separate updateWithError from update, add utility function to sendStatusChange event.

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

r=me with nits fixed, but can we add tests?

::: dom/downloads/src/DownloadsAPI.js
@@ +328,5 @@
> +      // XXX If these checks expand further, consider moving them into their
> +      // own function.
> +      //
> +      var result = aDownload.error.result;
> +      var storage = this._window.navigator.getDeviceStorage("sdcard");

s/var/let

@@ +338,5 @@
> +        // error is really happening.
> +        changed = false;
> +        debug("Attempting to infer error via device storage sanity checks.");
> +        // Get device storage and request availability status.
> +        var available = storage.available();

s/var/let

@@ +370,5 @@
>      }
>  
> +    this._sendStateChange();
> +  },
> +  

trailing whitespace

@@ +381,5 @@
> +      new this._window.DOMError("DownloadError", aError);
> +
> +    this._sendStateChange();
> +  },
> +  

trailing whitespace
Attachment #8403699 - Flags: review?(fabrice) → review+
r+ carries, review nits addressed. Tested on Peak.
Attachment #8403699 - Attachment is obsolete: true
Attachment #8404800 - Flags: review+
Keywords: checkin-needed
Whiteboard: [systemsfe][p=3] → [systemsfe][p=5]
Blocks: 994873
I filed a follow-up bug to add a mochi test for this so that it doesn't regress. It's dependent on this bug.
https://hg.mozilla.org/mozilla-central/rev/15029cea96fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Verified in master(v1.5) branch
Status: RESOLVED → VERIFIED
In v1.4 the bug continues. I'll review again the bug when this patch is added in this branch
You need to log in before you can comment on or make changes to this bug.