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

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[rafael.marquez]

Attachment: 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

Comment 1

[rafael.marquez]

Attachment: Error expected.png

Comment 2

[rafael.marquez]

I've talked to Cristian and this is an API bug

Comment 3

[Gregor Wagner [:gwagner]]

Can we make the current error msg more generic and fix it in 1.5?

Comment 4

[Aus Lacroix [:aus]]

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. :(

Comment 5

[Francis Djabri [:djabber]]

Attachment: SD-card-not-available-error

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?

Comment 6

[Aus Lacroix [:aus]]

: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.

Comment 7

[Aus Lacroix [:aus]]

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.

Comment 8

[[:fabrice] Fabrice Desré]

Why do we get the string from gecko? we should try to avoid that and move all strings to gaia.

Comment 9

[Aus Lacroix [:aus]]

Attachment: Pull Request - Download Manager should use new ondownloaderror event.

Comment 10

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Update string for accessErrorSD.

Comment 11

[Aus Lacroix [:aus]]

(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.

Comment 12

[[:fabrice] Fabrice Desré]

(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?

Comment 13

[Aus Lacroix [:aus]]

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.

Comment 14

[Aus Lacroix [:aus]]

: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?

Comment 15

[Boris Zbarsky [: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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I guess I would really rather not add even _more_ complexity via forked codepaths into that code, if we can avoid it.

Comment 17

[Aus Lacroix [:aus]]

(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.

Comment 18

[Boris Zbarsky [: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...

Comment 19

[Aus Lacroix [:aus]]

Attachment: Patch - WIP - Event Driven Error Reporting for Gonk

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.

Comment 20

[Aus Lacroix [:aus]]

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).

Comment 21

[Aus Lacroix [:aus]]

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.

Comment 22

[[:fabrice] Fabrice Desré]

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?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 24

[Aus Lacroix [:aus]]

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!

Comment 25

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Report error via nsITransfer instance.

This works beautifully and integrates with the Downloads API very cleanly.

Comment 26

[Aus Lacroix [:aus]]

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.

Comment 27

[Aus Lacroix [:aus]]

Try run -- https://tbpl.mozilla.org/?tree=Try&rev=664e1e52a1d9

Comment 28

[Aus Lacroix [:aus]]

(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

Comment 29

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Report error via nsITransfer instance.

Comment 30

[Aus Lacroix [:aus]]

Tested and working on Keon and Peak. Try run #2 is looking a lot better.

Comment 31

[Dave Hylands [:dhylands]]

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 32

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 34

[Cristian Rodriguez (:crdlc)]

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

Comment 35

[Aus Lacroix [:aus]]

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.

Comment 36

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Addressed review comments.

r+ from bz carries.

Comment 37

[Aus Lacroix [:aus]]

Try run with review comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=bf8ca5e3fc54

Comment 38

[Aus Lacroix [:aus]]

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.

Comment 39

[Aus Lacroix [:aus]]

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.

Comment 40

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1fe3452806

Comment 41

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8393189 [details] [review]
Pull Request - Download Manager should use new ondownloaderror event.

Perfect from my side, thanks for the good job!

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Aus Lacroix [:aus]]

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.

Comment 44

[Aus Lacroix [:aus]]

Gaia commit (master): https://github.com/mozilla-b2g/gaia/commit/288c8763265aa98c16c8b79cddaf37c3dd34d2a2

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 46

[Francesco Lodolo [:flod]]

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.

Comment 47

[Gregor Wagner [:gwagner]]

(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

Comment 48

[Aus Lacroix [:aus]]

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

Comment 49

[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