944337 - Test failure "Modal dialog has been found and processed" in /testAddons/testInstallAddonWithEULA.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[mario garbi]

Happened today with Mozmill 2.0 on OSX machines and Firefox 27:
http://mozmill-daily.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a3a7f52
http://mozmill-daily.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a3a8d88

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think something is wrong with the add-on we are trying to install from that site. Can you please check that?

Comment 2

[mario garbi]

Hmm the addon is working ok for me, plus this is not reproducing constantly.

Comment 3

[Andrei Eftimie]

I'll take this

Comment 4

[Andrei Eftimie]

This is a very similar (if not the same) issue to bug 939690

It also fails while running the tests:
> restartTests/testAddons_InstallAddonWithoutEULA
> testAddons/testInstallAddonWithEULA.js

with --profile

Comment 5

[Andrei Eftimie]

Attachment: 1.patch

This is a quick hack.

We fail to either preserve or set addons.mozilla.org as a whitelist entry.
Setting it manually in each test fixes the issue.

We should probably get this in mozmill/mozmill-automation/mozprofile (or where we are setting these settings).

Do we want to get this in to fix the current failures?
(And remove it once we fix the underlying issue)

Comment 6

[Andrei Eftimie]

Remote testrun:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a4a978e

The failure we still see is bug 944334

Comment 7

[Andrei Eftimie]

Attachment: esr17_1.patch

Patch for ESR17.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a4f12bd

Comment 8

[Andrei Eftimie]

Comment on attachment 8339971 [details] [diff] [review]
esr17_1.patch

Disregard this, was intended for bug 944334

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8339952 [details] [diff] [review]
1.patch

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

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +17,4 @@
>  
>  const INSTALL_DIALOG_DELAY = 250;
>  const TIMEOUT_DOWNLOAD = 25000;
> +const XPI_WHITELIST_DOMAIN = "addons.mozilla.org";

this domain should be whitelisted by default. Why is that no longer the case after a couple of restart tests have been run? This clearly looks like a problem in Firefox or a test which removes this entry from the whitelist. We should not workaround but fix the underlying issue.

Comment 10

[Andrei Eftimie]

I agree, I added a quick fix for us to avoid additional failures since we were running 2.0 in production.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We are back at 1.5.24 so there is no pressure today. But I really want to see it investigated and fixed asap. Lets observe this permission between tests so we know where it is getting reset.

Comment 12

[Andrei Eftimie]

I still have to dig a bit here.

The permissions.sqlite seems to be fine, the problem appears to lie in pref.js
I haven't yet been able to single-out the pref that is causing this.

If I clean the pref.js fine (or certain parts of it) the tests are passing nicely.

Comment 13

[Andrei Eftimie]

*prefs.js file*

Comment 14

[Andrei Eftimie]

We have the following 2 preferences:
> xpinstall.whitelist.add
> xpinstall.whitelist.add.180

Which are both being set to ""

Their default values are:
> xpinstall.whitelist.add : addons.mozilla.org
> xpinstall.whitelist.add.180 : marketplace.firefox.com

I'm trying to figure out *who* sets them and *when* they are set.

Haven't had success searching for the pref name on either mxr or mozmill related repositories.

Comment 15

[Andrei Eftimie]

After a whole lot more digging, the mentioned preferences are set when we try to install the addon from amo:
https://addons.mozilla.org/en-US/firefox/addon/restartless-addon-with-eula/eula/227209

I've tested manually with a clean profile.
When I try to install the addon from mozqa.com
http://mozqa.com/data/firefox/addons/extensions/restartless-eula.xpi

The change of preferences are still visible even if I cancel the install.
In this case I have to accept to install the addon (since it is not from a 'whitelisted' domain).

I'm not sure if this action sets the mentioned preferences.
I'll test with other addons and Firefox versions.

Comment 16

[Andrei Eftimie]

I've tested a new profile on all current Firefox builds.

`xpi.whitelist.add` is left at the default state of `addons.mozilla.org` only on Nightly and Aurora
All Beta, Release and ESR24 builds show this preference user set to empty string "".
This is all on a clean profile.

Weird is that even if this preference appears to be set initially (I've added a long sleep at the begging of the test) and checked both about:config and pref.js from the profile folder. The first run always passes. So there should also be something else that affects us here.

But if I remove only this preference from the prefs.js file, a subsequent run always passes.

Comment 17

[Andrei Eftimie]

Blair do you know how the preference `xpinstall.whitelist.add` should behave?

Is it normal to have them set to empty when trying to install an addon?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #17)
> Blair do you know how the preference `xpinstall.whitelist.add` should behave?
> 
> Is it normal to have them set to empty when trying to install an addon?

You should CC or put needinfo on him if you really want his feedback on this question. :)

Comment 19

[Andrei Eftimie]

I think I missed to mark the checkbox for the needinfo box.
Thanks for noticing that.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Andrei Eftimie from comment #17)
> Blair do you know how the preference `xpinstall.whitelist.add` should behave?
> 
> Is it normal to have them set to empty when trying to install an addon?

Yes, this is expected.

We use the permissions manager to determine whether a site is allowed to install an add-on. These preferences are just a simple and convenient way to get hosts into the permissions manager. If one of these preferences has a non-empty string value, it's imported into the permissions manager the first time a site requests an install*. We set it to an empty string so it isn't imported more than once (for performance reasons).

* Originally this was done at startup. Bug 697314 changed that recently (for performance reasons) so it only happens when we find we need to check the permissions manager, but still only happens once after startup.

Comment 21

[Andrei Eftimie]

Attachment: 2.patch

Comment 22

[Andrei Eftimie]

Pressed Enter to quickly.

Given that `xpinstall.whitelist.add` works as expected, lets clean up after we install some addons.

This fixes our remote testrun under mozmill 2.0:
http://mozmill-crowd.blargon7.com/#/remote/report/8ff37f5518a39f6af2983cfe4402aa2e

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8344533 [details] [diff] [review]
2.patch

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

::: firefox/tests/remote/testAddons/testInstallAddonWithEULA.js
@@ +39,5 @@
>  
>  function teardownModule(aModule) {
>    prefs.preferences.clearUserPref(PREF_INSTALL_DIALOG);
>    prefs.preferences.clearUserPref(PREF_LAST_CATEGORY);
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST);

Well, I'm still not sure why we have to reset this pref, and why addons.mozilla.org is not whitelisted anymore after we install the first add-on. Once added to the permission manager the domain should always be whitelisted.

Comment 24

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Well, I'm still not sure why we have to reset this pref, and why
> addons.mozilla.org is not whitelisted anymore after we install the first
> add-on. Once added to the permission manager the domain should always be
> whitelisted.

I dug around more, and I see what you mean here.
We do seem to have a problem, after a test has finished running via mozmill the permissions.sqlite is emptied!

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I cannot wait longer for a solution on this bug. Given that it has been stalled for nearly a week I'm glancing to take over the work which has to happen here. Andrei, is a final solution visible for today?

Comment 26

[Andrei Eftimie]

Lets recap what I said on IRC.

I don't have another working solution ATM besides clearing the xpinstall pref.
For having a green 2.0 green run, we could go with that and file a followup bug to fix the underlying issue.

I am not sure, but from what I saw we end up cleaning the profile multiple times.
Roughly I think this was it: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L113

When this is done the permissions.sqlite is nuked, but we remain with the user set prefs for xpinstall.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, that sounds like the problem. Can you please file a bug in mozbase for that problem? If we clean-up the profile we should also clean-up non-default preferences.

Comment 28

[Andrei Eftimie]

Comment on attachment 8344533 [details] [diff] [review]
2.patch

Filed bug 944337 for fixing the underlying issue with permissions vs prefs in mozbase.

I've reissued review flags for this patch.
This will allow us to successfully run these tests with mozmill 2.0

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8344533 [details] [diff] [review]
2.patch

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

Looks fine. Two nits to fix before we can get this landed. Please do so ASAP.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +38,5 @@
>    tabs.closeAllTabs(aModule.controller);
>  }
>  
>  function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST);

Please add a comment why this is necessary to do.

Comment 30

[Andrei Eftimie]

Attachment: 3.patch

Added a comment.

I wrongly referenced this bug in comment 28.
It should have been bug 951138

Landed it:
http://hg.mozilla.org/qa/mozmill-tests/rev/a4009c80179d (default)

Comment 31

[Andrei Eftimie]

BTW, green testrun on remote with 2.0.2:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed3dc37

Comment 32

[Andrei Eftimie]

Fixes the problem on Aurora:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed4558b

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/c3edf041487c (mozilla-aurora)

Unfortunately this patch doesn't resolve the issue on Beta :(
I'll check all remaining branches

Comment 33

[Andrei Eftimie]

Neither Release nor ESR24 are affected:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed65827
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed6fae2

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This works now:
http://mozmill-staging.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01edc7239

So lets get it backported.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #32)
> Unfortunately this patch doesn't resolve the issue on Beta :(

Uh, why not? Did you run it correctly?


(In reply to Andrei Eftimie from comment #33)
> Neither Release nor ESR24 are affected:

So it is a regression in Firefox? If only specific branches are affected the underlying issue cannot be a mozprofile one.

Comment 36

[Andrei Eftimie]

Not sure what I saw earlier, but this also fixes the problem on Beta:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01edd548f

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/643480b4394b (mozilla-beta)

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please do not close this bug until there are still outstanding questions about fixes on the remaining branches!

Comment 38

[Andrei Eftimie]

We do not have the affected tests on mozilla-release nor on mozilla-esr24.

I see the same failures on both branches if I replicate the conditions.
So we are good.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So it only happens when more than a test installing addons from addons.mozilla.org. I would suggest we leave this bug open until we got the final fix. As of now we only landed a workaround.

Comment 40

[Andreea Matei [:AndreeaMatei]]

The test was removed in bug 962514.