Closed Bug 982006 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

Other
Other
defect
Not set

Tracking

()

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=13])

Attachments

(5 files, 4 obsolete files)

Attached image Error displayed.png
*Procedure
1. Connect the device to a computer as USB storage
2. Try Download an file


*Expected Result
A error message is displayed. error expected.png (attached image)


*Actual Result
A wrong message is displayed. Error displayed.png (attached image)



This bug has been opened after resolving the bug 949991
Attached image Error expected.png
blocking-b2g: --- → 1.4?
Whiteboard: [systemsfe]
I've talked to Cristian and this is an API bug
Flags: needinfo?(aus)
Component: Gaia::System → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → Trunk
Can we make the current error msg more generic and fix it in 1.5?
Flags: needinfo?(fdjabri)
blocking-b2g: 1.4? → 1.4+
Making the error message more generic for 1.4 would be the easiest, otherwise it will require a gecko patch to add a different error message to differentiate between SD card busy when mounted to host and SD card physically not present. Regardless, this is a gecko patch, though. :(
Flags: needinfo?(aus)
Assignee: nobody → aus
Target Milestone: --- → 1.4 S4 (28mar)
Here's a spec for a generic error message. This is a little bit awkward though - is a bug going to be raised to split this into two messages for 1.5?
Flags: needinfo?(fdjabri)
:djabber, we'll be filing a follow up bug to differentiate between these two errors.

Note that the spec changes the title of the error so this will mean one string change and one additional string. I'd rather keep the current title which is "Download %s" where %s is the URL. 

There's a separate issue apparently where the title is *not* showing up. This is what I'm seeing on my phone as well (just like the screenshot).

For the scope of this bug, I'll fix the title issue and make sure it's displayed and update the string to be more generic.

It should be noted that all the errors in the string bundle that is relevant to the underlying component is entirely focused on errors during downloads and it therefor only has one title it uses for all errors.

Expanding that will require many new translations. Because of this, the follow-up bug will be targeted for 1.5.
Whiteboard: [systemsfe] → [systemsfe][p=3]
Well, the title not showing is likely a simple gaia bug. Yay!

https://github.com/mozilla-b2g/gaia/blob/374a5833edf45b40142e16e52eb3cc0e7396513e/apps/system/js/app_modal_dialog.js#L235

Note that we're not setting the title... anywhere.

This bug will consist of one gecko patch (for the string change in the properties file) and one gaia patch to actually display the title.
Status: NEW → ASSIGNED
Why do we get the string from gecko? we should try to avoid that and move all strings to gaia.
(In reply to Fabrice Desré [:fabrice] from comment #8)
> Why do we get the string from gecko? we should try to avoid that and move
> all strings to gaia.

We end up getting these strings in the fashion because this error occurs deep within the external helper application handler when it attempts to open a local file to save a download that is about to start. This can happen because the SD card isn't present or the SD card is mounted via USB to the host PC. This all happens before we are ever notified that there is a new download.

Do we have something to send these errors in to the window context as a domevent to be handled? Is that something we already do in places? We could potentially do this instead and forego these string changes.
Flags: needinfo?(fabrice)
(In reply to Ghislain Aus Lacroix [:aus] from comment #11)

> Do we have something to send these errors in to the window context as a
> domevent to be handled? Is that something we already do in places? We could
> potentially do this instead and forego these string changes.

I don't think we do that for now, but there's no reason to not do it if we need. Is the current code just using the prompt service? Where does that happen?
Flags: needinfo?(fabrice)
OK, after chatting with Fabrice about this I think we have a real plan we're all happy with.

1. For e10s, prompting will remain as is (as seen here: http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1853)

2. For Gonk (e10s), prompting will be removed and we will use a custom component to send the status changes to the DOM window via DOM events. These events will then be handled in the gaia application window code.

This will enable us to fully handle the errors as we see fit in the future and move all strings related to these errors into gaia and out of gecko. I think for the time being, the best UX is still the use of a modal alert to the user, so the current gaia pull request changes will remain (to enable us to have titles!) but it will be expanded significantly.

The scope of the changes will be limited to the External Helper Application Service errors which we currently report via SendStatusChange and no more.
Whiteboard: [systemsfe][p=3] → [systemsfe][p=8]
:bz, Before I get too far into it, do you think that https://bugzilla.mozilla.org/show_bug.cgi?id=982006#c13 is viable, long term, in your opinion?
Flags: needinfo?(bzbarsky)
I'm not sure I follow the proposal, exactly, but that's also because I don't know why that existing prompting code is there, exactly....

What are the situations in which we have neither mDialogProgressListener nor mTransfer?  And should we have one or the other in those situations?
Flags: needinfo?(bzbarsky)
I guess I would really rather not add even _more_ complexity via forked codepaths into that code, if we can avoid it.
(In reply to Boris Zbarsky [:bz] from comment #15)
> I'm not sure I follow the proposal, exactly, but that's also because I don't
> know why that existing prompting code is there, exactly....
> 
> What are the situations in which we have neither mDialogProgressListener nor
> mTransfer?  And should we have one or the other in those situations?

I believe this happens because we're running with e10s turned on and we don't have IPC for either of those pieces so we default to using a prompter instead of communicating the failure back to the child.

(In reply to Boris Zbarsky [:bz] from comment #16)
> I guess I would really rather not add even _more_ complexity via forked
> codepaths into that code, if we can avoid it.

I think realistically these errors could be surfaced to the browser element implementation and handled from there for e10s and gonk. However, we'd want to move the prompter code that currently exists out of the external helper handler and into BrowserElementChildPreLoad and only alert on error when the error event is not handled by the content loaded into the browser frame.
Flags: needinfo?(bzbarsky)
I see.

I guess we can just add another "else" to the cascade to communicate the failure to someone who has registered to be told about it...
Flags: needinfo?(bzbarsky)
There's a few things I'm not totally thrilled with...

* Namespacing for the event
* There should be one event type that then contains the errorId and other relevant info that we can stick in there (original rv?, URI?)
* Right now on the Gaia side of things, the AppWindow would need to handle these error events OR we could make the BrowserElementChildPreload handle it. I'm really not sure which is best here. It would make possibly more sense for BrowserElementChildPreload to reformat this error into a generic mozbrowsererror(iirc) event that the AppWindow can *then* use. Or we leave it up to the application.

Sorry, that's a lot of questions, but, this is a bigger change than I was expecting since this is a 1.4 blocker.
Attachment #8395150 - Flags: feedback?(fabrice)
Attachment #8395150 - Flags: feedback?(bzbarsky)
I should mention that the patch totally works! I have an event listener on the browser apps window and I can get the errors as expected and calling preventDefault() prevents the default prompter from being used (for e10s sans gonk, we would always use this).
Blocks: 981539
Smaug,

I'm trying to create a custom dom event from c++/gecko side of things and i would like event.detail to be a nice JS object, is there an easy way to do that?

I've tried setting the variant to hold a property bag but that doesn't get converted into a nice js object. It remains a property bag and that interface sucks in JavaScript.
Flags: needinfo?(bugs)
Comment on attachment 8395150 [details] [diff] [review]
Patch - WIP - Event Driven Error Reporting for Gonk

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

I think I'd like that to be hooked up to the mozbrowser api - but it's not a very strong preference. Could you loop in Ehsan and/or Jonas?
Attachment #8395150 - Flags: feedback?(fabrice)
(In reply to Ghislain Aus Lacroix [:aus] from comment #21)
> Smaug,
> 
> I'm trying to create a custom dom event from c++/gecko side of things and i
> would like event.detail to be a nice JS object, is there an easy way to do
> that?
The way to do that is to create a new WebIDL dictionary and
then something like http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp?rev=8693eace6c33&mark=137-141,143-144,146-153,160-163#126
which is absolutely horrible and error prone.

So, if we're about to expose some new API, why not create a new event type with proper semantics.
No need to go through event.detail.foo, just event.foo.
Event codegen can create such implementations easily.
It is a bit annoying to expose new interfaces, but if download handling has APIs anyway...

But yeah, perhaps ehsan or sicking have option here about the API.
Flags: needinfo?(bugs)
OK, Chatted with :sicking about this and here's where we are now.

* Firefox Desktop will want to use e10s as soon as they can, they will need a solution for this as well
* Firefox OS has it's own Downloads API that can already handle some types of errors, it would be nice for it to also provide failures to even start a download.
* jsdownloads is used by everyone as their base for providing download functionality so it would be great if it could be getting these errors.

The solution will provide the following:

* jsdownloads will be made aware of failures to start downloads (by the external helper application service, in a way similar that it gets notified of downloads starting).
* Firefox OS Downloads API will have the addition of an ondownloaderror event handler that can be used by any application that has download permissions.
* The current Gaia Download Manager UI will handle the errors in the same fashion as it reports successful downloads currently (this is how UX would prefer it as well).

The current patch is a great starting point. I'll try and post an updated WIP as soon as I have something that works.

Note that this bug will need to land in two pieces, one landing for the Gecko bits, one landing for the Gaia bits.

Thanks everyone for your help!
Attachment #8395150 - Attachment is obsolete: true
Attachment #8395150 - Flags: feedback?(bzbarsky)
Attachment #8393195 - Attachment is obsolete: true
Attachment #8393189 - Attachment description: Pull Request - Use title in calls for alert, prompt and confirm app modal dialogs. → Pull Request - Download Manager should use new ondownloaderror event.
This works beautifully and integrates with the Downloads API very cleanly.
Attachment #8398770 - Flags: review?(bzbarsky)
Comment on attachment 8393189 [details] [review]
Pull Request - Download Manager should use new ondownloaderror event.

Cristian, feel free to redirect. This can land without the gecko patch, but we'll probably want to know that the gecko patch *will* land before merging this.
Attachment #8393189 - Flags: review?(crdlc)
Attachment #8393189 - Flags: review?(alive)
Attachment #8398770 - Flags: review?(bzbarsky)
(In reply to Ghislain Aus Lacroix [:aus] from comment #27)
> Try run -- https://tbpl.mozilla.org/?tree=Try&rev=664e1e52a1d9

First run burned miserably. Second try? https://tbpl.mozilla.org/?tree=Try&rev=0496a036a715
Attachment #8398770 - Attachment is obsolete: true
Attachment #8398813 - Flags: review?(bzbarsky)
Tested and working on Keon and Peak. Try run #2 is looking a lot better.
Comment on attachment 8398813 [details] [diff] [review]
Patch - v2 - Report error via nsITransfer instance.

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

Some drvie-by comments (I noticed the device storage content)

::: dom/downloads/src/DownloadsAPI.js
@@ +318,5 @@
>        }
>      });
>  
>      if (aDownload.error) {
> +      this.error = 

nit: trailing space

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +348,3 @@
>    nsDOMDeviceStorage::GetDefaultStorageName(NS_LITERAL_STRING("sdcard"),
>                                              storageName);
>    NS_ENSURE_TRUE(!storageName.IsEmpty(), NS_ERROR_FAILURE);

On a real device, the storageName should always come back non-empty.

But on b2g-desktop, the returned storageName will always be empty.

So this NS_ENSURE_TRUE should be removed (and GetDefaultStorageName should probably explicitly set storageName to the empty string - I just filed bug 989538)

@@ +363,5 @@
> +  // Check device storage status before continuing.
> +  nsString storageStatus;
> +  dsf.GetStatus(storageStatus);
> +
> +  // If we get an empty status, it means the sd card is not present.

That shouldn't be the case. The returned status should always be one of "available", "shared" or "unavailable". Any other value is a bug.

I see one code path in GetStatus that doesn't initialize the returned status, and that path should only happen with some type internal error (I filed bug 989536 to get that fixed).

@@ +367,5 @@
> +  // If we get an empty status, it means the sd card is not present.
> +  if (storageStatus.IsEmpty()) {
> +    return NS_ERROR_FILE_NOT_FOUND;
> +  }
> +  else if (storageStatus.EqualsLiteral("shared") ||

nit: else not required

You should probably compare not equal to "available"

@@ +1656,5 @@
> +
> +    rv = CreateFailedTransfer(aChannel && NS_UsePrivateBrowsing(aChannel));
> +#ifdef PR_LOGGING
> +    if (NS_FAILED(rv)) {
> +      LOG(("Failed to create transfer to report failure." \

nit: backslash not required. You probably want a space after the .

In C/C++: "This is "  "a test" (there can be arbitrary white-space (including a newline) between the 2 strings, is the same as "This is a test"
Comment on attachment 8398813 [details] [diff] [review]
Patch - v2 - Report error via nsITransfer instance.

Do we want to piggyback on the existing error codes or just add new ones for this?  Just trying to somewhat minimize ifdefs... ;)

Not sure whether it's worth trying to share more of the code with CreateTransfer (by creating a common method that both will call).

r=me either way, I think, though I have to admit that I'm not as current on all the interactions here as I probably should be.  :(
Attachment #8398813 - Flags: review?(bzbarsky) → review+
Comment on attachment 8393189 [details] [review]
Pull Request - Download Manager should use new ondownloaderror event.

r=me if https://bugzilla.mozilla.org/show_bug.cgi?id=914317 is not regressed.
BTW, unit test for app_modalo_dialog wanted.
Attachment #8393189 - Flags: review?(alive) → review+
Comment on attachment 8393189 [details] [review]
Pull Request - Download Manager should use new ondownloaderror event.

Hi Aus, 

    I've added some minor comments but the most important is to add some unit tests to cover these new errors in Gaia.

    Request to me a new review when you address/answer comments.

Thanks a lot Aus
Attachment #8393189 - Flags: review?(crdlc)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Thanks for all the feedback guys! I'll be adding additional tests for the app modal dialog changes and ensure 914317 doesn't regress because of the changes.

The second try run looks good as well. I'll do one more push to try with all the changes before requesting landing.
r+ from bz carries.
Attachment #8398813 - Attachment is obsolete: true
Attachment #8399580 - Flags: review+
Try run with review comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=bf8ca5e3fc54
Pull request updated with review comments addressed and tests added. Waiting for Travis to go green before landing. Note that it's not necessary for the gecko bits to land just yet for the gaia changes to land, however, the current behavior will persist until it is landed.
Unfortunately, my changes to the AppModalDialog does regress 914317. I'm going to attempt to fix it while keeping my changes. It's the only thing that is preventing the gaia piece from landing.

The gecko patch can land.
Keywords: checkin-needed
Whiteboard: [systemsfe][p=8] → [systemsfe][p=13]
Keywords: leave-open
Comment on attachment 8393189 [details] [review]
Pull Request - Download Manager should use new ondownloaderror event.

Perfect from my side, thanks for the good job!
Attachment #8393189 - Flags: review+
Alright! I fixed the potential regression (bug 914317) which would've broken unless Alive had told me about it. Thanks Alive! :D

The AppModalDialog now ignores titles coming from gecko which simply contain the application url or origin. Under these conditions it will try and use the name of the application if it's available, if not, we simply use an empty string like we used to.

The underlying gecko errors that still use a prompter will have relevant titles for the most part and they will get used automatically.

I also added tests for this in the app modal dialog tests.

Gaia check-in imminent! Waiting on Travis to say I'm good to go.
Gaia commit (master): https://github.com/mozilla-b2g/gaia/commit/288c8763265aa98c16c8b79cddaf37c3dd34d2a2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/15312c8b1528

The Gaia patch has some conflicts on v1.4, so you'll need to rebase that.
Flags: needinfo?(aus)
Target Milestone: 1.4 S5 (11apr) → mozilla31
Hi, a clarification request: is it normal for these patches to land on mozilla-aurora without any kind of approval request? Because it just broke string-freeze for all products.
(In reply to Francesco Lodolo [:flod] from comment #46)
> Hi, a clarification request: is it normal for these patches to land on
> mozilla-aurora without any kind of approval request? Because it just broke
> string-freeze for all products.

There is no approval needed for 1.4 blockers: https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.4.0
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
Target Milestone: mozilla31 → 1.4 S5 (11apr)
Depends on: 999511
You need to log in before you can comment on or make changes to this bug.