896059 - Installation of packaged apps is broken

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Build: Firefox 25 Nightly
Device: Samsung Galaxy Nexus
OS: Android 4.2

STR

1. Go to http://mozqa.com/webapi-permissions-tests/
2. Select Install Packaged App under Packaged App Test Case 2

Expected

The packaged app should be able to be installed.

Actual

Installation fails and leaves behind a broken icon on the about:apps page.

Comment 1

[Aaron Train [:aaronmt]]

E/GeckoConsole(23746): [JavaScript Error: "Services.wm.getMostRecentWindow(...).navigator.getDeviceStorage is not a function" {file: "resource://gre/modules/Webapps.jsm" line: 2543}]

E/GeckoConsole(23746): [JavaScript Error: "TypeError: app.manifest is undefined" {file: "http://mozqa.com/webapi-permissions-tests/apps.js" line: 31}]

E/GeckoConsole(23746): [JavaScript Error: "this._manifest is undefined" {file: "resource://gre/modules/AppsUtils.jsm" line: 512}]

E/GeckoConsole(23746): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "this._manifest is undefined" {file: "resource://gre/modules/AppsUtils.jsm" line: 512}]' when calling method: [nsIDOMEventListener::handleEvent]" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2.apk!/assets/omni.ja!/components/Webapps.js" line: 760}]

Comment 2

[u421692]

The regression window is:

1.mozilla-central
good build: 19.07.2013 
bad build: 20.07.2013 
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=af4e3ce8c487&tochange=bf73e10f5e54

2.inbound
good build:1374130488
bad build: 1374133787
-pushlog: 
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d07610709b5d&tochange=30c1dda8e849

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> E/GeckoConsole(23746): [JavaScript Error:
> "Services.wm.getMostRecentWindow(...).navigator.getDeviceStorage is not a
> function" {file: "resource://gre/modules/Webapps.jsm" line: 2543}]

This error will be fixed by the patches in bug 777402.

> E/GeckoConsole(23746): [JavaScript Error: "TypeError: app.manifest is
> undefined" {file: "http://mozqa.com/webapi-permissions-tests/apps.js" line:
> 31}]

This one is expected, the code in that page is expecting app.manifest to exist in request.onsuccess, but there isn't such a property for packaged apps.

About the regression window, which one is it? Is the second a more detailed one?

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

I'm not sure about that regression window. I've managed to find two problems trying things our from: http://mozqa.com/webapi-permissions-tests/

1.) I forgot to add button toasts to the webapp layout. Since we initialize button toast in webapps, I guess the initialization it throwing...

2.) There have been some recent dom changes that caused navigator.getDeviceStorage to be undefined (as opposed to just returning null before). bug 838146.

Comment 6

[Wesley Johnston (:wesj)]

Even with that, some of the apps in the test page are not installing. Some with INVALID_SECURITY_LEVEL. Others, more worrying, somehow manage to force themselves to launch on install? Which then crashes quickly...

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 781247 [details] [diff] [review]
Patch

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

::: mobile/android/app/mobile.js
@@ +778,5 @@
>  pref("gfx.canvas.azure.backends", "skia");
>  pref("gfx.canvas.azure.accelerated", true);
> +
> +// Enable device storage so that packaged apps can install
> +pref("device.storage.enabled", true);

Why are we solving this bug by turning on device storage? Shouldn't we make the fix in the DOM Apps API code here?

Comment 8

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Even with that, some of the apps in the test page are not installing. Some
> with INVALID_SECURITY_LEVEL. Others, more worrying, somehow manage to force
> themselves to launch on install? Which then crashes quickly...

Note - some of the test apps I have intentionally will fail installation - the comments included with the test case should indicate that information.

The launch issue is bug 873701.

The crash issue sounds new though. Can you include a crash report URL here? What's the stack showing?

Comment 9

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

Comment on attachment 781247 [details] [diff] [review]
Patch

># HG changeset patch
># Parent a4c1961bf723dc7285f57f2cccbdf8144de187ab
>
>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js
>--- a/mobile/android/app/mobile.js
>+++ b/mobile/android/app/mobile.js
>@@ -772,8 +772,11 @@
> pref("media.useAudioChannelService", false);
> 
> // Turn on the CSP 1.0 parser for Content Security Policy headers
> pref("security.csp.speccompliant", true);
> 
> // Enable hardware-accelerated Skia canvas
> pref("gfx.canvas.azure.backends", "skia");
> pref("gfx.canvas.azure.accelerated", true);
>+
>+// Enable device storage so that packaged apps can install
>+pref("device.storage.enabled", true);

We should not need this part. The DOM code should correctly handle the null returned by getDeviceStorage. Seems like it is not returning null, which is a separate bug. Let's not flip this pref unless that is the Real Fix (tm) for this issue.


>diff --git a/mobile/android/base/resources/layout/web_app.xml b/mobile/android/base/resources/layout/web_app.xml

>+    <LinearLayout android:id="@+id/toast"
>+                  style="@style/Toast">
>+
>+        <TextView android:id="@+id/toast_message"
>+                  style="@style/ToastMessage" />
>+
>+        <ImageView android:id="@+id/toast_divider"
>+                   style="@style/ToastDivider" />
>+
>+        <Button android:id="@+id/toast_button"
>+                style="@style/ToastButton" />
>+
>+    </LinearLayout>

Maybe this means we should move super toast to BrowserApp? I'm ok with doing that in a followup.

Holding r+ until we decide on the "device.storage.enabled" pref

Comment 10

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

Just as an aside: It seems like a lot of stuff is changing under our feet and breaking packaged app installation. What the hell is happening?

Comment 11

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

Hindsight being what it is, if we had install tests enabled, bug 838146 would not have landed without fixing the WebApps.jsm code that uses getDeviceStorage.

Comment 12

[Jason Smith [:jsmith]]

Attachment: DOM Apps Part - Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

Comment 13

[Jason Smith [:jsmith]]

Comment on attachment 781708 [details] [diff] [review]
DOM Apps Part - Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

I haven't tested this yet, but I think this is the patch you want to fix the navigator.getDeviceStorage is not a function issue.

Note - this patch only fixes the bug in the DOM Apps code.

I'm spinning a try build in a second to test this manually.

Comment 14

[Aaron Train [:aaronmt]]

The above should be in a new bug.

Comment 15

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

Comment on attachment 781708 [details] [diff] [review]
DOM Apps Part - Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

You also need to fixup FreeSpaceWatcher
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/FreeSpaceWatcher.jsm#56

Comment 16

[Jason Smith [:jsmith]]

Comment on attachment 781708 [details] [diff] [review]
DOM Apps Part - Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

Ack. And that's what I get for hacking something together quickly.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch

Removed the device storage stuff.

Comment 18

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

Comment on attachment 784840 [details] [diff] [review]
Patch

Good for now

Comment 19

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1fa2987be8

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d1fa2987be8

Comment 21

[Ioana Chiorean]

Build: Firefox 29 Nightly 2014-01-10
Device: Samsung Galaxy Nexus
OS: Android 4.3

(In reply to Jason Smith [:jsmith] from comment #0)
> STR
> 1. Go to http://mozqa.com/webapi-permissions-tests/
> 2. Select Install Packaged App under Packaged App Test Case 2

Using the mentioned steps I am able to install the app and I do not get any errors. 
Marking this as Verified Fixed