Closed Bug 856084 Opened 11 years ago Closed 10 years ago

Test failure "The cookie has been removed - got 'false'" in /testPreferences/testRemoveCookie.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33 fixed, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr24 wontfix, firefox-esr31 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr24 --- wontfix
firefox-esr31 --- fixed

People

(Reporter: andrei, Assigned: teodruta, Mentored)

References

(
URL
)

Details

(Whiteboard: [mozmill-test-failure][lang=js])

Attachments

(2 files, 1 obsolete file)

b856084v2.patch
1.90 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
b856084v2ESR31ESR24.patch
1.61 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
status-firefox21: --- → affected
Priority: -- → P3
Whiteboard: [mozmill-test-failure]
Did not happen for a couple of months so closing as WFM
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
Happened again today on Linux Ubuntu 12.04 (x86_64) with Firefox 24.0 (24.0, de, 20130902131354)
http://mozmill-release.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1c5d1d9
Status: RESOLVED → REOPENED
status-firefox24: --- → affected
OS: Windows 7 → All
Resolution: WORKSFORME → ---
Versions 21 and 23 don't exist anymore. Same for 24.0. For esr17 I would vote wontfix given that the branch is closed soon.
status-firefox21: wontfix → ---
status-firefox23: wontfix → ---
status-firefox24: affected → ---
status-firefox-esr17: ? → wontfix
Has failed several times (until now 4 times) today, up from about once per week, bumping to P2
Priority: P3 → P2
Firefox 26 Beta is affected.
status-firefox26: ? → affected
Andrei and Cosmin, no further comments here regarding priorities? I would have expected that this gets a P1 given that it causes invalid results for beta builds and causes QA to manually verify this.
Flags: needinfo?(cosmin.malutan)
Flags: needinfo?(andrei.eftimie)
Well let us bump the priority then.

Latest ondemand run this failed twice:
26.0, ru, 20131104182142, Windows 7
http://mozmill-release.blargon7.com/#/functional/report/d17690a112360a2b3155acaa270e7555
26.0, pl, 20131104182142, OSX 10.8.5
http://mozmill-release.blargon7.com/#/functional/report/d17690a112360a2b3155acaa2702b681

Running on both OS'es the test to see if we're able to reproduce the failure.

On daily we had this failure exactly once:
ESR 24.0.1, en-US, 20131011000503, OSX 10.8.5
http://mozmill-daily.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8beae120af

What we know:
- very low reproducibility rate,
- all OS'es, all machines affected.

The only failure we have on daily is on en-US, all others (13 in total) are on various locales.
status-firefox28: --- → ?
Flags: needinfo?(andrei.eftimie)
Priority: P2 → P1
I made testruns and I run the folder separately for 20 times and did't reproduced it.
status-firefox28: ? → ---
Flags: needinfo?(cosmin.malutan)
Priority: P1 → P2
Submit conflict
status-firefox28: --- → ?
Priority: P2 → P1
I managed to reproduce the failure once in 400 runs with the following build:
26.0, ru, 20131104182142

Lets add some dumps and see if we get some relevant informations.
More info.
When it fails, origNumCookies is 2, here:
http://hg.mozilla.org/qa/mozmill-tests/file/8afbeba6b6f4/tests/functional/testPreferences/testRemoveCookie.js#l81

Which might imply not that we don't remove the cookie, but we somehow have 2 cookies, and we remove one.

Reference

Pass:
> == origNumCookies: 1
> == Services.cookies.cookieExists: false
> == cookieRemoved: true

Fail:
> == origNumCookies: 2
> == Services.cookies.cookieExists: true
> == cookieRemoved: false

I'll further test this hypothesis.
Assignee: nobody → andrei.eftimie
Update.

We have no cleanup problems.
The affected cookie is not present in setupModule() before nor after we call Services.cookies.removeAll();

The cookie appears to be added twice when we visit the page (?)
We then remove 1 instance, then fail when checking for its presence (because the "twin" cookie still exists).

In teardownModule() we still see the twin cookie, and we remove it with Services.cookies.removeAll();

===

I will hook up some code to check with API right before and after we visit the page to check the presence of the cookies.

From what I remember we are not able to set 2 cookies with the same name _unless_ they have different paths. I'll try gathering more info on the cookies to see how they differ, and maybe find out how they are stored twice.

This is going slower then anticipated, I only managed to fail the test once today, from 900 runs.
Can anyone please check the ratio between failures of beta/release vs. nightly builds? I can see nearly no failures for our nightlies.
Note that this appears to be more common on exotic locales (I'm not sure of this, but that's what we've seen from the _few_ failures we have recorded). And we have no exotic locales on Nightly, and we only really run them ATM on Beta and Release.
Do you have overall numbers for us sorted by such locales on beta and release?
I don't think it's related to exotic locales, the link in the URL field, sorted by locales show en-US, de, it mostly. 
On Aurora we fail as well, with the 4 locales that we test.
Looking more over the failures, versions, locales, I can't draw any real conclusion.
Locale probably has no influence for this failure.


Nr. of failures / locale
========================

Daily
-----
2 : en-US

Release
-------
3 : it
2 : es-ES
2 : en-US
2 : ru
2 : de
1 : sr
1 : pt-BR
1 : pl
1 : ml
1 : fr
1 : en-GB
1 : cs

Total
=====
20 failures recorded
Last time it failed on 27/12, decreasing the priority a little.
Priority: P1 → P2
It's been about a month. What's the status of this failure?
Flags: needinfo?(andrei.eftimie)
This is still failing, but has a low occurrence rate,
I am able to reproduce this about 1 time in 900 runs.

I detailed what is happening in comment 15.
My next step (which I started, but didn't yet got around to finish it due to the low occurrence rate) was to try holding the browser open when this occurs to manually check _why_ we have 2 cookies saved instead of one.

With this I should be able to verify if this is a bug in Firefox or a problem in our tests.
Flags: needinfo?(andrei.eftimie)
Lets put it back into the general bucket, so that someone else could pick it up until we have time for. Andrei, would you like to mentor this bug?
Assignee: andrei.eftimie → nobody
Status: REOPENED → NEW
Priority: P2 → P3
I would still like to finish what I started. But if anyone wants to help out that would be great.

Basically I have the specific test patched to sleep for a long time when this fails.
I only need to leave my VM run this for a day (last time I tried it didn't fail at all for a whole day).

But I need to check this at regular intervals to catch the failure.

When I do we should see if the 2 cookies are _identical_ if that is the case we might have a bug in Firefox. This is probably unlikely, since you can not have 2 cookies with the same name on the same domain + path.

If they are not identical, it might be that their path differs. In this case we might be able to understand what is going wrong.

==
For both cases this is likely either a asynchronicity issue or a race condition.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mentor=andrei_eftimie]
Would you mind to upload your patch for the test so others could directly use it?

To get started please have a look at https://developer.mozilla.org/en-US/docs/Mozmill_Tests
Whiteboard: [mozmill-test-failure][mentor=andrei_eftimie] → [mozmill-test-failure][mentor=andrei_eftimie][lang=js]
Failed again:
http://mozmill-release.blargon7.com/#/functional/report/4e9e123ff3e2bbcb949b2192928ad60b

I should run the test I had mentioned in comment 28 to see if we get somewhere.
Mentor: andrei.eftimie
Whiteboard: [mozmill-test-failure][mentor=andrei_eftimie][lang=js] → [mozmill-test-failure][lang=js]
This failed today on Firefox 31.0 te on mm-osx-107-2 (2014-07-17_12-39-21)  : http://mozmill-release.blargon7.com/#/functional/report/db75145408bff5ccf8a072085b4db7ba
(In reply to Andrei Eftimie from comment #33)
> This is still failing, though rarely, failed 4 times in the last month:
> http://mozmill-release.blargon7.com/#/functional/
> failure?branch=All&platform=All&from=2013-07-
> 30&test=%2FtestPreferences%2FtestRemoveCookie.js&func=testRemoveCookie.
> js%3A%3AtestRemoveCookie

Wrong link? I cannot find a single failure for that since December last year.
status-firefox25: wontfix → ---
status-firefox26: wontfix → ---
status-firefox27: wontfix → ---
status-firefox28: wontfix → ---
status-firefox33: --- → ?
status-firefox34: --- → ?
status-firefox-esr17: wontfix → ---
status-firefox-esr31: --- → affected
Assignee: nobody → teodor.druta
Attached patch b856084.patch (obsolete) — — Splinter Review 
The issue is related to the filter field functionality not displaying query results fast enough resulting in two cookies in the cookies list, so the test actually removes the incorrect cookie and fails when checking if the cookie has been removed.
This patch should fix this issue.
Status: NEW → ASSIGNED
Attachment #8506895 - Flags: review?(andrei.eftimie)
Attachment #8506895 - Flags: review?(andreea.matei)
Comment on attachment 8506895 [details] [diff] [review]
b856084.patch

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

Interesting train of thought, but I can't follow it.

I've tested numerous times, and I don't see how we could have 2 cookies shown. There can only be one.
The opposite also holds. If we were to try removing the cookie _before_ it was shown, we would immediately fail when checking the removed cookie and the rowCount:
>  expect.ok(cookieRemoved, "The cookie has been removed");
>  expect.equal(cookiesList.view.rowCount, (origNumCookies - 1),
>               "There is one less cookie than before");

Please add more data here.

Are you able to reproduce the issue?
Having 2 different cookies and the test removing the wrong one might explain the failure, but I don't see from where the second cookie might come from...

::: firefox/tests/functional/testPreferences/testRemoveCookie.js
@@ +92,5 @@
> +
> +  // wait for the slow search cookies functionality in firefox
> +  assert.waitFor(() => (cookiesList.view.rowCount === 1), 
> +                 "There is only one row in the cookies list !");
> +  

nit: make sure you don't have trailing whitespaces
Attachment #8506895 - Flags: review?(andrei.eftimie)
Attachment #8506895 - Flags: review?(andreea.matei)
Attachment #8506895 - Flags: review-
If you see more than 1 cookie please dump all the cookie information to the console. This might help to figure out what's wrong. If that is really the case it should be a bug in Firefox and no workaround is warranted.
Actually your patch might indeed fix the issue.(In reply to Andrei Eftimie from comment #38)
> The opposite also holds. If we were to try removing the cookie _before_ it
> was shown, we would immediately fail when checking the removed cookie and
> the rowCount:
> >  expect.ok(cookieRemoved, "The cookie has been removed");
> >  expect.equal(cookiesList.view.rowCount, (origNumCookies - 1),
> >               "There is one less cookie than before");

Actually this is exactly how the test is failing. So indeed we might click on "Remove Cookie" before the list is populated, thus failing to remove it...

In this case your patch should solve the issue.
Comment on attachment 8506895 [details] [diff] [review]
b856084.patch

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

Fix the mentioned nits, and add a commit message please.

::: firefox/tests/functional/testPreferences/testRemoveCookie.js
@@ +89,5 @@
>  
>    // Get the number of cookies in the file manager before removing a single cookie
>    var cookiesList = aController.window.document.getElementById("cookiesList");
> +
> +  // wait for the slow search cookies functionality in firefox

Rephrase into: "Wait for the cookie list to load"

@@ +91,5 @@
>    var cookiesList = aController.window.document.getElementById("cookiesList");
> +
> +  // wait for the slow search cookies functionality in firefox
> +  assert.waitFor(() => (cookiesList.view.rowCount === 1), 
> +                 "There is only one row in the cookies list !");

"There is one item in the cookie list."
Attached patch b856084v2.patch — — Splinter Review 
Updated the patch following the review notes.
Attachment #8506895 - Attachment is obsolete: true
Attachment #8508573 - Flags: review?(andrei.eftimie)
Attachment #8508573 - Flags: review?(andreea.matei)
Comment on attachment 8508573 [details] [diff] [review]
b856084v2.patch

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

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

Please check backporting, if the patch applies cleanly and tests well. Thanks!
Attachment #8508573 - Flags: review?(andrei.eftimie)
Attachment #8508573 - Flags: review?(andreea.matei)
Attachment #8508573 - Flags: review+
The patch applies cleanly on all branches except mozilla-esr31 and mozilla-esr24.
I have attached a patch for this branches.
Attachment #8514154 - Flags: review?(andrei.eftimie)
Attachment #8514154 - Flags: review?(andreea.matei)
Lets get this transplanted, hopefully no more failures!
This is a point fix in the test itself, I will land across branches:
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/2e55e5da4350 (mozilla-aurora)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/d2aff51c188b (mozilla-beta)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/9ff4d8332d7e (mozilla-release)
status-firefox33: affected → fixed
status-firefox34: affected → fixed
status-firefox35: affected → fixed
Comment on attachment 8514154 [details] [diff] [review]
b856084v2ESR31ESR24.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/333c8d5f61eb (mozilla-esr31)
Attachment #8514154 - Flags: review?(andrei.eftimie)
Attachment #8514154 - Flags: review?(andreea.matei)
Attachment #8514154 - Flags: review+
Attachment #8514154 - Flags: checkin+
Thanks Teodor for figuring this out and providing a fix.
Lets hope this is the last we'll see of this failure.

Please reopen if this doesn't fully solve the issue.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 10 years ago
status-firefox-esr31: affected → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: