1197920 - TypeError: DOMApplicationRegistry.getAll is not a function on Fennec

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

I'm seeing the following error consistently in custom built Fennecs:

08-24 11:17:43.291  12238-12254/org.mozilla.fennec_nalexander E/GeckoConsole﹕ [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
    See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
    Date: Mon Aug 24 2015 11:16:37 GMT-0700 (PDT)
    Full Message: TypeError: DOMApplicationRegistry.getAll is not a function
    Full Stack: this.WebappManager._getInstalledApps@resource://gre/modules/WebappManager.jsm:466:5
    this.WebappManager.checkForUpdates/<@resource://gre/modules/WebappManager.jsm:385:33
    TaskImpl_run@resource://gre/modules/Task.jsm:314:40
    TaskImpl@resource://gre/modules/Task.jsm:275:3
    createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
    Task_spawn@resource://gre/modules/Task.jsm:164:12
    this.WebappManager.checkForUpdates@resource://gre/modules/WebappManager.jsm:372:1
    WebappsUpdateTimer.prototype.notify@jar:jar:file:///data/app/org.mozilla.fennec_nalexander-1.apk!/assets/omni.ja!/components/WebappsUpdateTimer.js:77:5
    TM_notify/<@jar:jar:file:///data/app/org.mozilla.fennec

Comment 1

[Nick Alexander :nalexander [he/him]]

I did some digging and I can't tell what's changed recently.  I /do/ see two implementations of DOMApplicationRegistry -- is it possible we're getting the wrong one due to build indeterminancy?

I can't investigate further.  myk, esawin: can you look into this and re-file as appropriate?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #1)
> I did some digging and I can't tell what's changed recently.  I /do/ see two
> implementations of DOMApplicationRegistry -- is it possible we're getting
> the wrong one due to build indeterminancy?

If y'all don't see this, it could be a strange issue with |mach artifact| builds.  That would be interesting to know.

Comment 3

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

This is almost certainly a regression from bug 1191579.  I'll need to investigate more to figure out what the fix should be.

Comment 4

[[:fabrice] Fabrice Desré]

Ha sorry guys. Myk, using AppsServiceChild.jsm should work.

Comment 5

[Eugen Sawin [:esawin]]

Looks like bug 1191579 missed two more use cases ([1], [2]).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#937
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebappManager.jsm#466

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Eugen Sawin [:esawin] from comment #5)
> Looks like bug 1191579 missed two more use cases ([1], [2]).
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#937

This is fine, AppsServiceChild.jsm provides DOMApplicationRegistry.getAll() in https://dxr.mozilla.org/mozilla-central/source/dom/apps/AppsServiceChild.jsm?offset=300#339

> [2]
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> WebappManager.jsm#466

AppsServiceChild.jsm should work for this one too.

Comment 7

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

(In reply to [:fabrice] Fabrice Desré from comment #6)
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> > WebappManager.jsm#466
> 
> AppsServiceChild.jsm should work for this one too.

This is actually the instance in question, per the stack in comment 0.  And I'll look into importing AppsServiceChild for it.  WebappManager is actually loaded into the parent process, but AppsServiceChild should work there too.

Comment 8

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

Attachment: patch v1: import AppsServiceChild.DOMApplicationRegistry for its getAll method

This does what I expect and is the minimal fix for the problem.  Unsure if WebappManager should use AppsServiceChild for anything else, but that would be a separate issue in any case.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8653175 [details] [diff] [review]
patch v1: import AppsServiceChild.DOMApplicationRegistry for its getAll method

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

::: mobile/android/modules/WebappManager.jsm
@@ +28,5 @@
>  
> +// Import AppsServiceChild.DOMApplicationRegistry for its getAll method.
> +var AppsServiceChild = {};
> +XPCOMUtils.defineLazyModuleGetter(AppsServiceChild, "DOMApplicationRegistry",
> +                                  "resource://gre/modules/AppsServiceChild.jsm");

You don't really need AppsServiceChild, but up to you.

Comment 10

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

(In reply to [:fabrice] Fabrice Desré from comment #9)
> You don't really need AppsServiceChild, but up to you.

WebappManager accesses a variety of DOMApplicationRegistry properties that don't exist on the AppsServiceChild implementation of DOMApplicationRegistry, like the doInstall method and registryReady.  If it were to import AppsServiceChild.DOMApplicationRegistry into the global namespace, replacing the implementation of DOMApplicationRegistry that it imported from Webapps.jsm, then it would lose access to those properties.  So it seems like it does need to import AppsServiceChild.DOMApplicationRegistry into AppsServiceChild (or some other namespace), unless there's something I'm missing.

Comment 11

[[:fabrice] Fabrice Desré]

You're right, and damn 8-line contexts :P

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8653175 [details] [diff] [review]
patch v1: import AppsServiceChild.DOMApplicationRegistry for its getAll method

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

More/earlier imports won't help us with startup performance, but we can deal with it later should this become an issue.
Looks good!

Comment 13

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

(In reply to Eugen Sawin [:esawin] from comment #12)

> More/earlier imports won't help us with startup performance, but we can deal
> with it later should this become an issue.

This particular import is lazily loaded when WebappManager.checkForUpdates calls WebappManager._getInstalledApps, and WebappManager.checkForUpdates is called well after startup by WebappsUpdateTimer.  So it shouldn't affect startup.

> Looks good!

I'll land it once fx-team reopens!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4c4e73857103

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c4e73857103