776677 - Catch errors thrown from Webapps.js and show to user

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Wesley Johnston (:wesj)]

Errors like:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#147

need to be shown to the user.

Comment 2

[Wesley Johnston (:wesj)]

So I don't think I can get to these errors as easily as I had hoped. They're dispatched on the request event and then bubble to... nowhere? AFAICT, we have no access to the actual events themselves, so we also can't tell if the store has handled them.

I have a quick patch (testing now) to fix that so that nsIDOMRequestService.fireSuccess/Error will return the event and we can see if defaultPrevented == true? That sound ok Jonas?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How about making fireError/Success return a boolean which indicates if defaultPrevented was called? That matches what EventTarget.dispatchEvent does.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Fixed the DOMRequest stuff in bug 780718. This catches the preventDefault and if it wasn't called dispatches a notification. I only hooked that up for some of the errors in WebApps.js (related to install errors).

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Show the errors

This should catch the errors and show them as a toast. I added three different messages. One generic, one for problems with the app manifest, and one for network problems. Errors are listed here:

https://developer.mozilla.org/en-US/docs/DOM/Apps.install

Comment 6

[Aaron Train [:aaronmt]]

Jason, do you have an existing test-case in use for desktop I can try this on once it lands?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 649431 [details] [diff] [review]
Patch 2/2 - Show the errors

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+        switch(aData) {

nit: add a space
          switch (aData)

>+          case("INVALID_MANIFEST"):

nit: I have never seen this case style. Please use:
            case "INVALID_MANIFEST":

>+        try {
>+          msg = Strings.browser.GetStringFromName("webapps.installError." + aData);
>+        } catch(ex) {
>+          // There is no message for this error, just show the default one
>+          
>+        }

Remove this. There are no strings of the form: webapps.installError." + aData
Your switch statement handles the 3 error strings you added.

r+ with fixes

Comment 8

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8236b3579bf3

Need to test the fennec part better (obviously).

Comment 9

[Wesley Johnston (:wesj)]

backed out because of b2g failures with the domrequest patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621cb0844bf5

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0fbd3c0087

Comment 11

[Wesley Johnston (:wesj)]

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

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 649430 [details] [diff] [review]
Patch 1/2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new Webapps stuff
User impact if declined: Markets or pages that install apps but that don't do their own error handling can cause the user to get basically no feedback
Testing completed (on m-c, etc.): landed on inbound 8/8
Risk to taking this patch (and alternatives if risky): Low risk. Webapps.js stuff. the higher risk patches are in bug 780718
String or UUID changes made by this patch: None here. Some in the other patch

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 649431 [details] [diff] [review]
Patch 2/2 - Show the errors

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new Webapps stuff
User impact if declined: Markets or pages that install apps but that don't do their own error handling can cause the user to get basically no feedback
Testing completed (on m-c, etc.): landed on inbound 8/8
Risk to taking this patch (and alternatives if risky): Low risk. Webapps.js stuff. the higher risk patches are in bug 780718
String or UUID changes made by this patch: Error strings to show to the user

Comment 14

[Wesley Johnston (:wesj)]

Test failures. I just pulled the webapps.js part because the rest should be fine.

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

Comment 15

[Jason Smith [:jsmith]]

Can we do some testing on Nightly before we do the uplift here to Aurora? I'd like to see how this functionality works with typical test cases that would cause errors to populate.

Comment 16

[Jason Smith [:jsmith]]

Flagging qawanted to do some testing of this to find out an opinion on the errors messages sent to the user.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b0fbd3c0087

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Nice to have.

Comment 19

[Jason Smith [:jsmith]]

Played around with the implementation of this. Generally, with a few spot checks, this works by only throwing up a alert pop-up with the error contents if an uncaught exception is thrown by the content of the site. A caught exception won't thrown the error to the pop-up which makes sense.

However, I'm inclined to believe that this might be unnecessary to have exposed to a user, as a user isn't going to care about a site problem like this directly in the browser. A developer would, but they could check the ADB console or use a remote debugging tool to find this out. I'm currently in favor of backing this patch out, as I'm not sure I see the value of exposing the pop-up for uncaught JS exceptions.

Comment 20

[Wesley Johnston (:wesj)]

Just to clarify, this should show a toast, but the part that will make this work hasn't landed (due to some test failures). If you're seeing an alert its likely coming from somewhere else.

Without this failures result in the user seeing nothing. i.e. they hit an install button. Nothing happens.

Comment 21

[Marco Castelluccio [:marco]]

I think we shouldn't show this kind of errors to users, they only matter to developers. Instead we should show errors that happen during the installation.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)