navigator.mozApps.mgmt.onuninstall gets triggered a boatload of times

RESOLVED FIXED in Firefox 18

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: etienne, Assigned: etienne)

Tracking

unspecified
mozilla19
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 6 obsolete attachments)

Assignee

Description

7 years ago
Posted patch Patch proposal (obsolete) — Splinter Review
To the untrained eye, it looks like WebappsApplicationMgmt gets added as a listener for Webapps:Uninstall:Return:OK every time a WebappsApplication is created.

Not sure if it's linked to this: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#342

Preventing DOMApplicationRegistry from adding the same listener for the same message multiple times fixes the issue (cf patch proposal).
But this is obviously just curing a symptom.
Assignee

Updated

7 years ago
Attachment #665698 - Flags: feedback?(fabrice)
Comment on attachment 665698 [details] [diff] [review]
Patch proposal

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

That looks good to me.
Attachment #665698 - Flags: feedback?(fabrice) → feedback+
Assignee

Comment 2

7 years ago
Looks like the real cause of this issue was fixed, closing here.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Assignee

Comment 3

7 years ago
My bad... still happening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee

Comment 4

7 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Clearer commit message.
Attachment #665698 - Attachment is obsolete: true
Attachment #665890 - Flags: review?(fabrice)
Attachment #665890 - Flags: review?(fabrice) → review+
Assignee

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [qa-]
Should this have a test?
Assignee: nobody → etienne
Flags: in-testsuite?
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25041824911b - one of the things in that push was causing

9351 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_cross_origin.xul | Test timed out.
9354 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_getNotInstalled.xul | Test timed out.
9358 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_app.xul | Test timed out.
9364 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | Test timed out.
9365 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
9366 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 404 remaining tests.
9367 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
Test failures are because of this patch...
Assignee

Comment 9

7 years ago
Posted patch Patch v3 with test (obsolete) — Splinter Review
Fixed the test failure and, since the tests saved my bacon, added a test case for this bug.
Attachment #665890 - Attachment is obsolete: true
Attachment #666493 - Flags: review?(fabrice)
Assignee

Comment 10

7 years ago
(In reply to Phil Ringnalda (:philor) from comment #7)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/25041824911b - one of
> the things in that push was causing
> 
> 9351 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_cross_origin.xul | Test timed out.
> 9354 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_getNotInstalled.xul | Test timed out.
> 9358 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_app.xul | Test timed out.
> 9364 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_errors.xul | Test timed out.
> 9365 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test
> timeouts, giving up.
> 9366 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 404
> remaining tests.
> 9367 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_errors.xul finished in a non-clean fashion, probably because it
> didn't call SimpleTest.finish()

Sorry again for this, newbie mistake.
Assignee

Updated

7 years ago
Flags: in-testsuite? → in-testsuite+
Assignee

Updated

7 years ago
Depends on: 777508
Assignee

Comment 11

7 years ago
Posted patch Patch v4 (obsolete) — Splinter Review
Real ref count for the number of times a cpmm is added.
Attachment #666493 - Attachment is obsolete: true
Attachment #666493 - Flags: review?(fabrice)
Attachment #667452 - Flags: review?(fabrice)
Assignee

Comment 12

7 years ago
Posted patch Patch v5 (obsolete) — Splinter Review
Attachment #667452 - Attachment is obsolete: true
Attachment #667452 - Flags: review?(fabrice)
Attachment #667578 - Flags: review?(fabrice)
Comment on attachment 667578 [details] [diff] [review]
Patch v5

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

r=me with nit addressed and I'd like to see a try run.

::: dom/apps/src/Webapps.jsm
@@ +435,5 @@
>      }, this);
>    },
>  
>  
> +  // Passing -1 as aMsgNames will remove all references to aMm

Please use a special message name instead of -1. We don't get random messages here, so something like "Webapps:Internal:ClearMM" is ok to me. Feel free to bikeshed the message name.
Assignee

Comment 14

7 years ago
Posted patch Patch v6 (obsolete) — Splinter Review
nit addressed
Attachment #667578 - Attachment is obsolete: true
Attachment #667578 - Flags: review?(fabrice)
Assignee

Comment 15

7 years ago
Waiting for my commit access to do the try run, but happy to learn how to do it.
Assignee

Updated

7 years ago
Blocks: 796823
Assignee

Updated

7 years ago
blocking-basecamp: --- → ?
Assignee

Comment 16

7 years ago
Should maybe inherit the blocking-basecamp from 796823
blocking-basecamp: ? → +
Assignee

Comment 18

7 years ago
Mmhhh... I guess it's part of the fun but everything is green for me in |extensions/cookie| (chrome) and |toolkit/mozapps| (browser).

Fabrice, any pointers?
https://tbpl.mozilla.org/?tree=Try&rev=9d5a23dc1e9a is also orange but I can't repro the failures locally either.

Investing further, but wtf.
Hrm, so I did two other try runs:

- one with the new test disabled:
https://tbpl.mozilla.org/?tree=Try&rev=612acfa56e8e

- one with the test enabled:
https://tbpl.mozilla.org/?tree=Try&rev=b3d89b15d03e

So it looks like the failure is a side effect of the new test...
Assignee

Comment 21

7 years ago
Is it worth testing again with this small cleanup function added at the end of the test?
Attachment #667924 - Attachment is obsolete: true
Priority: -- → P2
Assignee

Comment 22

7 years ago
So, I suppose the last try run was the same ? :/
Yes :(

The only thing that makes sense to me is that we don't clear the permissions properly when uninstalling, so the next test starts with a profile in an unexpected state. That would actually be a platform bug.
hm... we install/uninstall http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/apps/basic.webapp in this test, and this app has no permissions.
Blocks: 802027
We need this patch for bug 802027, and without that the app installation experience is just awful. So I will land it without the test, and open a new bug to investigate why the test is failing.
Depends on: 803628
Assignee

Comment 27

7 years ago
Posted patch Patch v8Splinter Review
New patch with test working!
I was able to reproduce the failure by copying the failing test in the webapps mochitests dir.

Turns out we're uninstalling 2 apps, but not the one we initially installed!
This fixes it.
Attachment #673332 - Flags: review?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/7ae0ccc25650
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
The patch landed on m-c is different from the one that landed on Aurora. I suspect a merge failure? Pushed a correction:

https://hg.mozilla.org/releases/mozilla-aurora/rev/733109bd4ed1
Attachment #673332 - Flags: review?(fabrice)

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.