982182 - Check for Updates in about:apps fails to check for updates

Status: RESOLVED FIXED

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

Description

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

Manually checking for updates by tapping the Check for Updates item in Tools > Apps doesn't work, either because it has no (apparent) effect or because it opens Marketplace in a new tab instead of checking for updates.

In addition to the user-facing impact, this also makes it difficult to test updates (f.e. bug 970300) as well as fixes to the update code (f.e. bug 970200).

Comment 1

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

There are three different problems here.  First, there's an occurrence of "WebApps:GetApkVersions" that should be "Webapps:GetApkVersions", which is a regression from bug 958358.  Second, the sendMessageToJava implementation in WebappManager.jsm throws a JSON exception because it wasn't updated to include a GUID in the message when bug 946344 landed.

And finally, if you "Browse the Firefox Marketplace" to open the Marketplace in a new tab, then return to the about:apps tab and touch "Check for Updates", Fennec opens Marketplace in a new tab again instead of checking for updates.

I'll fix the first two bugs here and have filed bug 982548 on the last one.

Comment 2

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

Attachment: patch v1: fix regressions that hang checking for updates for webapps

Comment 3

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

Note that the patch here was applied on top of the patch for bug 982552, since bug 982552 helped me debug the problem.  And they touch code in the same area.  So if you try to apply this one without applying that one, then application will fail (although the conflict is trivial to resolve).

Comment 4

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

Comment on attachment 8389688 [details] [diff] [review]
patch v1: fix regressions that hang checking for updates for webapps

Does it make sense to lazy load the JSM? It doesn't look like any of the code using sendMessageToJava is called right away, but I might be wrong.

Comment 5

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

Attachment: patch v2: implements suggestion to lazily load module

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Does it make sense to lazy load the JSM? It doesn't look like any of the
> code using sendMessageToJava is called right away, but I might be wrong.

You're right, and it does make sense (plus is cleaner), so I've made this change.  Carrying forward review; this is the version of the patch I'll commit.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6f7b7dbeec

Comment 7

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

Attachment: Fx29 version of fix

The Fx29 version of the fix is even simpler, since only one of the two regressions landed in Fx29.  Here's the trivial, obvious, one-line patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  bug 958358

User impact if declined:
  Webapp installs won't update.

Testing completed (on m-c, etc.):
  This has landed on inbound but hasn't yet merged to Central.  I did, however,
  test this in my own builds, and fix is obvious and trivial.

Risk to taking this patch (and alternatives if risky):
  The risk is low, and there is no alternative.

String or IDL/UUID changes made by this patch:
  None.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a6f7b7dbeec

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df9d8939b21b