Closed Bug 898448 Opened 6 years ago Closed 6 years ago

Installation of packaged apps fail due to assuming existence of getDeviceStorage

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: jsmith, Assigned: jsmith)

References

Details

Attachments

(1 file, 4 obsolete files)

See discussion on bug 896059.
Blocks: 896059
OS: All → Android
Hardware: All → ARM
Attachment #781721 - Flags: review?(fabrice)
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)
(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.
Assignee: nobody → jsmith
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.
Attachment #781721 - Flags: review?(fabrice) → review-
Attachment #781721 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed to use getMostRecentWindow.
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.
Attachment #782045 - Flags: review?(fabrice)
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.
Attachment #782045 - Flags: review?(fabrice) → review-
Attachment #782045 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
Comment on attachment 782056 [details] [diff] [review]
Patch v3

Fixed to handle null case.
Attachment #782056 - Flags: review?(fabrice)
Attachment #782056 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=312f81126928

Doesn't look like everything passed, so let me look into this.
Keywords: checkin-needed
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}]
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.
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?
Flags: needinfo?(bzbarsky)
(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.
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.
Flags: needinfo?(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.
(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.
Attachment #782056 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
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 on attachment 782377 [details] [diff] [review]
Patch v4

Green on try.
Attachment #782377 - Flags: review?(fabrice)
Attachment #782377 - Flags: review?(fabrice) → review+
Apparently this patch works on B2G, but fails on Android since bind won't work on a non-existent function.
Attachment #782377 - Attachment is obsolete: true
Attached patch Patch v5Splinter Review
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 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.
Attachment #782719 - Flags: review?(fabrice)
Attachment #782719 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3cf583b9a2a7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.