Closed Bug 944337 Opened 7 years ago Closed 7 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Tracking Status
firefox25 --- wontfix
firefox26 --- unaffected
firefox27 --- affected
firefox28 --- fixed
firefox29 --- fixed
firefox-esr17 --- wontfix
firefox-esr24 --- unaffected

People

(Reporter: mario.garbi, Assigned: andrei)

References

(Depends on 1 open bug, )

Details

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

Attachments

(1 file, 3 obsolete files)

I think something is wrong with the add-on we are trying to install from that site. Can you please check that?
Hmm the addon is working ok for me, plus this is not reproducing constantly.
I'll take this
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
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
Attached patch 1.patch (obsolete) — Splinter Review
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)
Attached patch esr17_1.patch (obsolete) — Splinter Review
Patch for ESR17.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a4f12bd
Attachment #8339971 - Flags: review?(andreea.matei)
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-
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.
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.
*prefs.js file*
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.
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.
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.
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)
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)
Attached patch 2.patch (obsolete) — Splinter Review
Attachment #8344533 - Flags: review?(hskupin)
Attachment #8344533 - Flags: review?(dave.hunt)
Attachment #8344533 - Flags: review?(andreea.matei)
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
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-
(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)
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.
Depends on: 951138
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+
Attached patch 3.patchSplinter Review
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+
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
(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)
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
Closed: 7 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 → ---
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
Closed: 7 years ago7 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
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.