Closed Bug 830258 Opened 8 years ago Closed 8 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g1819+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla21
blocking-b2g -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 19+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: airpingu, Assigned: airpingu)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

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!
Yes, we can do that.
blocking-b2g: --- → tef?
No longer blocks: app-install
blocking-b2g: tef? → -
Attached patch Easy fix! (obsolete) — Splinter Review
Assignee: nobody → clian
Attachment #702121 - Flags: review?(fabrice)
Comment on attachment 702121 [details] [diff] [review]
Easy fix!

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

Gene, can you add a test?
Attachment #702121 - Flags: review?(fabrice) → review+
OK! No problem!
Attached patch Part 1, fix .uninstall() (obsolete) — Splinter Review
Carry on review+ with r=fabrice.
Attachment #702121 - Attachment is obsolete: true
Attachment #702734 - Flags: review+
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?
Attachment #702736 - Flags: feedback?(fabrice)
Hi Fabrice, ping a bit.
Comment on attachment 702736 [details] [diff] [review]
Part 2, provide test case

Please see comment #6 for the summary. Thanks!
Attachment #702736 - Flags: feedback?(fabrice) → review?(fabrice)
Attachment #702736 - Flags: review?(fabrice) → review+
Rebased.
Attachment #702734 - Attachment is obsolete: true
Attachment #707975 - Flags: review+
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.
Attachment #707975 - Flags: approval-mozilla-b2g18?
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.
Attachment #702736 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/aaa82cd26aab
https://hg.mozilla.org/mozilla-central/rev/2025d4fb452f
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [qa-]
Comment on attachment 702736 [details] [diff] [review]
Part 2, provide test case

Approving for 1.0.1 (mozilla-b2g18 tip) uplift.
Attachment #702736 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #707975 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Batch edit: Bugs fixed on b2g18 after 1/25 merge to v1.0 branch are fixed on v1.0.1 branch.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.