962790 - "reference to undefined property installed.apps" on uninstall

Status: RESOLVED FIXED

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

Description

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

I recently saw this message when uninstalling an app with synthetic APKs enabled (and the latest changes on inbound):

[JavaScript Warning: "ReferenceError: reference to undefined property installed.apps" {file: "resource://gre/modules/WebappManager.jsm" line: 178}]

I wonder if this is another regression from bug 959420, since WebappManager.jsm can be lazily loaded; and if it's loaded lazily, imports Webapps.jsm, and then immediately calls DOMApplicationRegistry.doGetAll in autoUninstall, then bug 959420 means the registry probably isn't ready to provide a list of installed apps.

Although it should still provide an empty list in that case, so this is puzzling.

Comment 1

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

Attachment: 0001-Bug-962790-ensure-registryReady-before-calling-DOMAp.patch

Even if bug 959420 didn't cause this particular problem, I think it means that we can't ever count on the registry being ready unless its new registryReady promise is resolved, so any DOMApplicationRegistry calls that might happen soon after importing Webapps.jsm need to wait until registryReady.

Here's a patch that fixes those calls in WebappManager.  Requesting feedback from Marco to confirm my suspicion that we'll need to do this in general when accessing DOMApplicationRegistry from now on.

WesJ: this is another case where the whitespace-excluding diff shows that these changes simply wrap a simple promise chain around a block of indentation-only changes in three different places:

+    DOMApplicationRegistry.registryReady.then(() => {
… (indentation-only changes)
+    });

Comment 2

[Marco Castelluccio [:marco]]

Comment on attachment 8363999 [details] [diff] [review]
0001-Bug-962790-ensure-registryReady-before-calling-DOMAp.patch

We need to wait whenever we need to use the registry soon after startup (even before bug 959420 landed).

Comment 3

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

(In reply to Marco Castelluccio [:marco] from comment #2)
> We need to wait whenever we need to use the registry soon after startup
> (even before bug 959420 landed).

Hmm, I think we'll actually have to check registryReady now whenever we access the registry, even if it's long after startup, unless we know for sure that the registry must already have been initialized, since it doesn't necessarily get initialized on startup.

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
> (In reply to Marco Castelluccio [:marco] from comment #2)
> > We need to wait whenever we need to use the registry soon after startup
> > (even before bug 959420 landed).
> 
> Hmm, I think we'll actually have to check registryReady now whenever we
> access the registry, even if it's long after startup, unless we know for
> sure that the registry must already have been initialized, since it doesn't
> necessarily get initialized on startup.

That's correct, we need to wait unless we're sure the registry has been loaded (in the desktop case we load it at startup, in Firefox for Android too if I recall correctly).

Comment 5

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

(In reply to Marco Castelluccio [:marco] from comment #4)
> That's correct, we need to wait unless we're sure the registry has been
> loaded (in the desktop case we load it at startup, in Firefox for Android
> too if I recall correctly).

Synthetic APKs does it more lazily now in Fennec.  But even if it didn't, it sometimes launches Fennec and then immediately accesses the registry, so loading the registry aggressively on startup still wouldn't solve this problem.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8363999 [details] [diff] [review]
0001-Bug-962790-ensure-registryReady-before-calling-DOMAp.patch

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

This seems likely to be forgotten often. All of my solutions also feel like they might too though...

Comment 7

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

(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 8363999 [details] [diff] [review]
> 0001-Bug-962790-ensure-registryReady-before-calling-DOMAp.patch
> 
> Review of attachment 8363999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems likely to be forgotten often. All of my solutions also feel like
> they might too though...

Yup.  Marco and I are both thinking about this, although we haven't come up with a solution yet.

Comment 8

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

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

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a85a91d17061

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> Yup.  Marco and I are both thinking about this, although we haven't come up
> with a solution yet.

A possible but not beautiful solution would be to make all DOMApplicationRegistry public methods wait on the registryLoaded promise before doing anything.