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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P2)

Description

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

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.

Comment 1

[[:fabrice] Fabrice Desré]

Comment on attachment 665698 [details] [diff] [review]
Patch proposal

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

That looks good to me.

Comment 2

[Etienne Segonzac (:etienne)]

Looks like the real cause of this issue was fixed, closing here.

Comment 3

[Etienne Segonzac (:etienne)]

My bad... still happening.

Comment 4

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Clearer commit message.

Comment 5

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4935822abf87

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Should this have a test?

Comment 7

[Phil Ringnalda (:philor)]

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()

Comment 8

[[:fabrice] Fabrice Desré]

Test failures are because of this patch...

Comment 9

[Etienne Segonzac (:etienne)]

Attachment: Patch v3 with test

Fixed the test failure and, since the tests saved my bacon, added a test case for this bug.

Comment 10

[Etienne Segonzac (:etienne)]

(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.

Comment 11

[Etienne Segonzac (:etienne)]

Attachment: Patch v4

Real ref count for the number of times a cpmm is added.

Comment 12

[Etienne Segonzac (:etienne)]

Attachment: Patch v5

Comment 13

[[:fabrice] Fabrice Desré]

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.

Comment 14

[Etienne Segonzac (:etienne)]

Attachment: Patch v6

nit addressed

Comment 15

[Etienne Segonzac (:etienne)]

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

Comment 16

[Etienne Segonzac (:etienne)]

Should maybe inherit the blocking-basecamp from 796823

Comment 17

[[:fabrice] Fabrice Desré]

I pushed the latest patch to try: https://tbpl.mozilla.org/?tree=Try&rev=1aceb2c29811

Comment 18

[Etienne Segonzac (:etienne)]

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?

Comment 19

[[:fabrice] Fabrice Desré]

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

Investing further, but wtf.

Comment 20

[[:fabrice] Fabrice Desré]

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...

Comment 21

[Etienne Segonzac (:etienne)]

Attachment: Patch v7 - test clean up

Is it worth testing again with this small cleanup function added at the end of the test?

Comment 22

[Etienne Segonzac (:etienne)]

So, I suppose the last try run was the same ? :/

Comment 23

[[:fabrice] Fabrice Desré]

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.

Comment 24

[[:fabrice] Fabrice Desré]

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.

Comment 25

[[:fabrice] Fabrice Desré]

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.

Comment 26

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae0ccc25650

Comment 27

[Etienne Segonzac (:etienne)]

Attachment: Patch v8

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ae0ccc25650

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/049788f927b3

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

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