830258 - [Webapps] .uninstall() should return "Webapps:Uninstall:Return:KO" when uninstalling a non-removable app

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Gene Lian (I already quit Mozilla)]

I discovered a potential logic error when I was tracing the .uninstall() codes:

  uninstall: function(aData, aMm) {
    for (let id in this.webapps) {
      let app = this.webapps[id];
      if (app.origin != aData.origin) {
        continue;
      }

      if (!app.removable)
        return;

      (skip...)

      return;
    }

    aMm.sendAsyncMessage("Webapps:Uninstall:Return:KO", aData);
  },

Do we need to return "Webapps:Uninstall:Return:KO" when attempting to uninstall a non-removable app? The caller would probably hang there if it cannot receive any return from its .onsuccess/.onerror callbacks.

Fabrice, how do you feel? Please let me know if we need to provide a patch for this (or not). Thanks!

Comment 1

[[:fabrice] Fabrice Desré]

Yes, we can do that.

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Easy fix!

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 702121 [details] [diff] [review]
Easy fix!

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

Gene, can you add a test?

Comment 4

[Gene Lian (I already quit Mozilla)]

OK! No problem!

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, fix .uninstall()

Carry on review+ with r=fabrice.

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, provide test case

Hi Fabrice,

I provide a test case to test the error return of "NOT_INSTALLED", which hasn't been tested before.

Honestly, I don't quite understand how to simulate a non-removable app for testing. May I have your suggestion please? Or just don't need to do that if it's actually hard to make one?

Comment 7

[Gene Lian (I already quit Mozilla)]

See try first: https://tbpl.mozilla.org/?tree=Try&rev=42dd7ba57967 (looks good!)

Comment 8

[Gene Lian (I already quit Mozilla)]

Hi Fabrice, ping a bit.

Comment 9

[Gene Lian (I already quit Mozilla)]

Comment on attachment 702736 [details] [diff] [review]
Part 2, provide test case

Please see comment #6 for the summary. Thanks!

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, fix .uninstall(), V1.1

Rebased.

Comment 11

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa82cd26aab
https://hg.mozilla.org/integration/mozilla-inbound/rev/2025d4fb452f

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 707975 [details] [diff] [review]
Part 1, fix .uninstall(), V1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: none
Testing completed: yes
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

This is fixing a potential logic bug in Webapps.jsm, which reports a proper error to Gaia when attempting to uninstall a non-removable app. So far, the Gaia codes haven't yet tried to catch any error events when calling .uninstall(), therefore this patch is backward-compatible and should be pretty safe to land. As far as I know, we should keep the Webapps.jsm sync'ed as possible as we can to avoid uplifting conflicts, since it's a core module and lots of people are working on it.

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 702736 [details] [diff] [review]
Part 2, provide test case

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: none
Testing completed: yes
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

This is the corresponding test case to test part-1 patch. Please see comment #12. If part-1 patch got approval to land, it should be fine to land this as well.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aaa82cd26aab
https://hg.mozilla.org/mozilla-central/rev/2025d4fb452f

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702736 [details] [diff] [review]
Part 2, provide test case

Approving for 1.0.1 (mozilla-b2g18 tip) uplift.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/49a989daf5fc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/844a5acc1ec3

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs fixed on b2g18 after 1/25 merge to v1.0 branch are fixed on v1.0.1 branch.