898448 - Installation of packaged apps fail due to assuming existence of getDeviceStorage

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

See discussion on bug 896059.

Comment 1

[Jason Smith [:jsmith]]

Attachment: Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

Comment 2

[Marco Castelluccio [:marco]]

Could we wait bug 777402 for the fix in Webapps.jsm? (the patches there already contain that fix, I would prefer not to rebase them again)

Comment 3

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio [:marco] from comment #2)
> Could we wait bug 777402 for the fix in Webapps.jsm? (the patches there
> already contain that fix, I would prefer not to rebase them again)

hmm..I'd rather split out the specific fix actually for the getDeviceStorage piece mainly because the FxAndroid guys are blocked right now with this not working. But let's see what fabrice thinks.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 781721 [details] [diff] [review]
Check for getDeviceStorage Existence in Webapps.jsm before calling getDeviceStorage

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

::: dom/apps/src/FreeSpaceWatcher.jsm
@@ +52,5 @@
>                callback.currentStatus = newStatus;
>              }
>            };
>  
> +          if (navigator.getDeviceStorage) {

There's no navigator object in the global scope in chrome code, so this will fail in interesting ways. You need to get it from the getMostRecentWindow() call.

::: dom/apps/src/Webapps.jsm
@@ +2694,5 @@
>         }
>         download();
>      };
>  
> +    if (navigator.getDeviceStorage) {

Same here.

Comment 5

[Jason Smith [:jsmith]]

Attachment: Patch v2

Fixed to use getMostRecentWindow.

Comment 6

[Jason Smith [:jsmith]]

https://tbpl.mozilla.org/?tree=Try&rev=5e0ffa2e5c95

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 782045 [details] [diff] [review]
Patch v2

Got an okay from fabrice on this on approach in IRC, so I'll ask for review.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 782045 [details] [diff] [review]
Patch v2

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

Sorry I didn't look closely the first time.

::: dom/apps/src/FreeSpaceWatcher.jsm
@@ +57,5 @@
> +                                 .getMostRecentWindow("navigator:browser")
> +                                 .navigator.getDeviceStorage;
> +
> +          if (getDeviceStorage) {
> +            let deviceStorage = getDeviceStorage("apps");

That can return null, so we must take care of that case.

::: dom/apps/src/Webapps.jsm
@@ +2704,5 @@
> +    let getDeviceStorage = Services.wm.getMostRecentWindow("navigator:browser")
> +                                      .navigator.getDeviceStorage;
> +
> +    if (getDeviceStorage) {
> +      let deviceStorage = getDeviceStorage("apps");

Ditto.

Comment 9

[Jason Smith [:jsmith]]

Attachment: Patch v3

Comment 10

[Jason Smith [:jsmith]]

Comment on attachment 782056 [details] [diff] [review]
Patch v3

Fixed to handle null case.

Comment 11

[Jason Smith [:jsmith]]

https://tbpl.mozilla.org/?tree=Try&rev=312f81126928

Doesn't look like everything passed, so let me look into this.

Comment 12

[Jason Smith [:jsmith]]

Only the packaged app install mochitest is relevant in terms of failures. I'm seeing the following:

22:42:58  WARNING -  E/GeckoConsole(  646): [JavaScript Error: "'getDeviceStorage' called on an object that does not implement interface Navigator." {file: "resource://gre/modules/Webapps.jsm" line: 2698}]

Comment 13

[Marco Castelluccio [:marco]]

Same problem with my patch.

Anyway I've noticed that getDeviceStorage is defined in Firefox 22, but not in Firefox Nightly. If you execute this code in the Browser Console (or Error Console):
> Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser").navigator.getDeviceStorage

In Firefox 22 you get a function, in Firefox Nightly you get an undefined value.

If you execute:
> Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser").navigator.getDeviceStorage("apps")

In Firefox 22 you get null, in Firefox Nightly an exception.

Comment 14

[Jason Smith [:jsmith]]

Right, that's cause in Nightly, we landed support for the navigator object being converted to WebIDL (bug 838146).

However, one thing not explained here yet is why in chrome code Services.wm.getMostRecentWindow("navigator:browser").navigator.getDeviceStorage is firing the error cited in comment 12. 

Boris - Shouldn't the navigator object present there be able to call getDeviceStorage?

Comment 15

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #14)
> Right, that's cause in Nightly, we landed support for the navigator object
> being converted to WebIDL (bug 838146).
> 
> However, one thing not explained here yet is why in chrome code
> Services.wm.getMostRecentWindow("navigator:browser").navigator.
> getDeviceStorage is firing the error cited in comment 12. 
> 
> Boris - Shouldn't the navigator object present there be able to call
> getDeviceStorage?

Let me reword that - Shouldn't the navigator object present there be able to allow me to access the function getDeviceStorage to get an undefined value? See the attached patch for clarification.

Comment 16

[Boris Zbarsky [:bzbarsky]]

In the old setup, navigator.getDeviceStorage was always defined.  When called it would return null based on various things like preferences and what you were requesting and whatnot.

In the new setup, it's defined only if the device.storage.enabled preference is set, because if that's not set then it can't possibly ever do anything useful.  This was a purposeful change from the current behavior made in bug 838146.

Now obviously we could change back to the old behavior: define the property unconditionally and return null in various cases.  But note that Jonas actually wants to move in the opposite direction and not define the property at all in contexts that don't have permissions for it, no matter what the pref is set to.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, and the attached patch looks wrong to me, because the method is being invoked with the window as "this", which ought to throw.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Oh, and the attached patch looks wrong to me, because the method is being
> invoked with the window as "this", which ought to throw.

Yup, just realized that. I know what to do now. Thanks for the help.

Comment 19

[Jason Smith [:jsmith]]

Attachment: Patch v4

Comment 20

[Jason Smith [:jsmith]]

I think the updated patch is what we want, but I'm waiting for the try run to confirm.

https://tbpl.mozilla.org/?tree=Try&rev=f3ed5c9b16af

Comment 21

[Jason Smith [:jsmith]]

Comment on attachment 782377 [details] [diff] [review]
Patch v4

Green on try.

Comment 22

[Jason Smith [:jsmith]]

Apparently this patch works on B2G, but fails on Android since bind won't work on a non-existent function.

Comment 23

[Jason Smith [:jsmith]]

Attachment: Patch v5

This time I was actually a bit more careful. Try run is green and manually tested on FxAndroid.

https://tbpl.mozilla.org/?tree=Try&rev=71386fae55ba

On FxAndroid, this code works, but the packaged app installation doesn't appear to completely finish installing during the oninstall event handler:

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

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 782719 [details] [diff] [review]
Patch v5

Talked with marco about this. Code-wise we know this works based on try run and manual testing, but packaged app install still won't fully work with this patch alone. There's a followup issue I'll file for the bug I just found.

Comment 25

[Matthew N. [:MattN]]

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

Comment 26

[Ed Morley [:emorley]]

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