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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mario.garbi, Assigned: andrei)

Tracking

(Depends on: 1 bug)

unspecified

Firefox Tracking Flags

(firefox25 wontfix, firefox26 unaffected, firefox27 affected, firefox28 fixed, firefox29 fixed, firefox-esr17 wontfix, firefox-esr24 unaffected)

Details

(Whiteboard: [mozmill-test-failure][needs Mozmill 2.0.3 for final fix], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Updated

5 years ago
status-firefox25: --- → ?
status-firefox26: --- → ?
status-firefox27: --- → affected
status-firefox28: --- → ?
status-firefox29: --- → ?
status-firefox-esr17: --- → ?
status-firefox-esr24: --- → ?
I think something is wrong with the add-on we are trying to install from that site. Can you please check that?
(Reporter)

Comment 2

5 years ago
Hmm the addon is working ok for me, plus this is not reproducing constantly.
(Assignee)

Comment 3

5 years ago
I'll take this
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
status-firefox28: ? → affected
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
Created attachment 8339952 [details] [diff] [review]
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)
Attachment #8339952 - Flags: feedback?(hskupin)
Attachment #8339952 - Flags: feedback?(dave.hunt)
Attachment #8339952 - Flags: feedback?(andreea.matei)
(Assignee)

Comment 7

5 years ago
Created attachment 8339971 [details] [diff] [review]
esr17_1.patch

Patch for ESR17.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a4f12bd
Attachment #8339971 - Flags: review?(andreea.matei)
(Assignee)

Comment 8

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

Disregard this, was intended for bug 944334
Attachment #8339971 - Attachment is obsolete: true
Attachment #8339971 - Flags: review?(andreea.matei)
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.
Attachment #8339952 - Flags: feedback?(hskupin)
Attachment #8339952 - Flags: feedback?(dave.hunt)
Attachment #8339952 - Flags: feedback?(andreea.matei)
Attachment #8339952 - Flags: feedback-
(Assignee)

Comment 10

5 years ago
I agree, I added a quick fix for us to avoid additional failures since we were running 2.0 in production.
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.
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
*prefs.js file*
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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?
(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. :)
Flags: needinfo?(bmcbride)
(Assignee)

Comment 19

5 years ago
I think I missed to mark the checkbox for the needinfo box.
Thanks for noticing that.
(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.
Flags: needinfo?(bmcbride)
(Assignee)

Comment 21

5 years ago
Created attachment 8344533 [details] [diff] [review]
2.patch
Attachment #8344533 - Flags: review?(hskupin)
Attachment #8344533 - Flags: review?(dave.hunt)
Attachment #8344533 - Flags: review?(andreea.matei)
(Assignee)

Comment 22

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #8339952 - Attachment is obsolete: true
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.
Attachment #8344533 - Flags: review?(hskupin)
Attachment #8344533 - Flags: review?(dave.hunt)
Attachment #8344533 - Flags: review?(andreea.matei)
Attachment #8344533 - Flags: review-
(Assignee)

Comment 24

5 years ago
(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!
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?
Flags: needinfo?(andrei.eftimie)
(Assignee)

Comment 26

5 years ago
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.
Flags: needinfo?(andrei.eftimie)
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.
(Assignee)

Updated

5 years ago
Depends on: 951138
(Assignee)

Comment 28

5 years ago
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
Attachment #8344533 - Flags: review?(hskupin)
Attachment #8344533 - Flags: review?(andreea.matei)
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.
Attachment #8344533 - Flags: review?(hskupin)
Attachment #8344533 - Flags: review?(andreea.matei)
Attachment #8344533 - Flags: review-
Attachment #8344533 - Flags: review+
(Assignee)

Comment 30

5 years ago
Created attachment 8348721 [details] [diff] [review]
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)
Attachment #8344533 - Attachment is obsolete: true
Attachment #8348721 - Flags: checkin+
(Assignee)

Updated

5 years ago
status-firefox29: ? → fixed
(Assignee)

Comment 31

5 years ago
BTW, green testrun on remote with 2.0.2:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed3dc37
(Assignee)

Comment 32

5 years ago
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
status-firefox28: affected → fixed
(Assignee)

Comment 33

5 years ago
Neither Release nor ESR24 are affected:
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed65827
http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01ed6fae2
status-firefox25: ? → wontfix
status-firefox26: ? → unaffected
status-firefox-esr17: ? → wontfix
status-firefox-esr24: ? → unaffected
(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.
Flags: needinfo?(andrei.eftimie)
(Assignee)

Comment 36

5 years ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(andrei.eftimie)
Resolution: --- → FIXED
Please do not close this bug until there are still outstanding questions about fixes on the remaining branches!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 38

5 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][needs Mozmill 2.0.3 for final fix]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The test was removed in bug 962514.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.