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

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

We have 1 failure on Aurora, Windows 7, fr:
http://mozmill-ci.blargon7.com/#/functional/report/25ad365ca7bcf4905e9b700b4f4b6135

Comment 1

[Daniela Petrovici]

Did not happen for a couple of months so closing as WFM

Comment 2

[mario garbi]

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

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

It failed again on Mac x64 with Beta es-ES
http://mozmill-release.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8beacd541a

Comment 4

[Andreea Matei [:AndreeaMatei]]

Happened on Ubuntu 13.04, with Beta ru:
http://mozmill-release.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d392d72

Comment 5

[Andrei Eftimie]

One new failure with 25 Release Candidate:
http://mozmill-release.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dfda62a

Comment 6

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

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.

Comment 7

[Andrei Eftimie]

Has failed several times (until now 4 times) today, up from about once per week, bumping to P2

Comment 8

[u279076]

Firefox 26 Beta is affected.

Comment 9

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

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.

Comment 10

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

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

I made testruns and I run the folder separately for 20 times and did't reproduced it.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Submit conflict

Comment 13

[Andrei Eftimie]

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.

Comment 14

[Andrei Eftimie]

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.

Comment 15

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

Comment 16

[Andreea Matei [:AndreeaMatei]]

Has failed today with Beta it locale, on OS X 10.7.5:
http://mozmill-release.blargon7.com/#/functional/report/21341f02f219acb032f628de853ff14e

Comment 17

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

Can anyone please check the ratio between failures of beta/release vs. nightly builds? I can see nearly no failures for our nightlies.

Comment 18

[Andreea Matei [:AndreeaMatei]]

No failures with nightly, I see only 2 with 24esrpre:
http://mozmill-daily.blargon7.com/#/functional/failure?branch=All&platform=All&from=2013-07-30&to=&test=%2FtestPreferences%2FtestRemoveCookie.js&func=testRemoveCookie.js%3A%3AtestRemoveCookie

Comment 19

[Andrei Eftimie]

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.

Comment 20

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

Do you have overall numbers for us sorted by such locales on beta and release?

Comment 21

[Andreea Matei [:AndreeaMatei]]

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.

Comment 22

[Andrei Eftimie]

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

Comment 23

[Andreea Matei [:AndreeaMatei]]

Failed today with Esr24, on OS X 10.7:
http://mozmill-daily.blargon7.com/#/functional/report/8ff37f5518a39f6af2983cfe442c847b

Comment 24

[Andreea Matei [:AndreeaMatei]]

Last time it failed on 27/12, decreasing the priority a little.

Comment 25

[u279076]

It's been about a month. What's the status of this failure?

Comment 26

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

Comment 27

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

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?

Comment 28

[Andrei Eftimie]

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.

Comment 29

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

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

Comment 30

[Andreea Matei [:AndreeaMatei]]

Failed today with OS X 10.8.5, beta:
http://mozmill-release.blargon7.com/#/functional/report/a438ea29b921b2e8124749eda90eeb8b

Comment 31

[Andrei Eftimie]

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.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 33

[Andrei Eftimie]

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

Comment 34

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

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

Comment 35

[Andrei Eftimie]

Indeed, that was the wrong link.
Corrected: http://mozmill-release.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-07-01&test=%2FtestPreferences%2FtestRemoveCookie.js&func=testRemoveCookie

Comment 36

[Andreea Matei [:AndreeaMatei]]

Failed with release bg, on Win Vista:
http://mozmill-release.blargon7.com/#/functional/report/eefb0f8a26a2a6196c62c274481ed97d

Comment 37

[Teodor Druta]

Attachment: b856084.patch

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.

Comment 38

[Andrei Eftimie]

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

Comment 39

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

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.

Comment 40

[Andrei Eftimie]

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 41

[Andrei Eftimie]

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

Comment 42

[Teodor Druta]

Attachment: b856084v2.patch

Updated the patch following the review notes.

Comment 43

[Andreea Matei [:AndreeaMatei]]

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!

Comment 44

[Teodor Druta]

Attachment: b856084v2ESR31ESR24.patch

The patch applies cleanly on all branches except mozilla-esr31 and mozilla-esr24.
I have attached a patch for this branches.

Comment 45

[Andrei Eftimie]

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)

Comment 46

[Andrei Eftimie]

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)

Comment 47

[Andrei Eftimie]

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.