1088551 - Test failure 'Panel is in the correct state - 'closed' should equal 'open'' in testSecurity/testSSLStatusAfterRestart.js

Status: RESOLVED FIXED

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

Description

[Cosmin Malutan, [:cosmin-malutan]]

Module:    firefox/lib/toolbars.js
Test:      testDisplayCertificateStatusAfterRestart    
Failure:   Panel is in the correct state - 'closed' should equal 'open
Branches:  Beta
Platforms: Ubuntu

This fails only on Beta / Ubuntu 
I couldn't reproduce it locally without interfering with the run, but it failed quite a lot in beta runs.
What happens here is that somehow after the notification is opend it get's closed before we can take any action on it.
http://hg.mozilla.org/qa/mozmill-tests/file/cf96c843574c458bfb45f895e50fc4decb9b626e/firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js#l117


http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2014-10-23&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

This would need a skip patch for Beta.

Comment 1

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

Are all Ubuntu machines affected?

Comment 2

[Andrei Eftimie]

Both 35 and 36 are affected: http://mozmill-daily.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2014-10-23&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

Cosmin, please prepare a skip patch.

We blamed a similar failure on the e10s intro window (need to find the bug). But this notification does not exist on Beta.

Comment 3

[Andrei Eftimie]

Attachment: quick_tour_notification.jpg

On OSX, Firefox 34 with mozmill 2.0.8.1 does still has the "Quick Tour" notification. As per my previous comment it's possible this is the cause, and will be fixed with mozmill 2.9

Comment 4

[Andrei Eftimie]

See bug 1082590.

The cause would be that the action to close the panel ("ESC") closes the Quick Tour panel instead of the one we wait for to close.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip patch

Here is the skip patch

Comment 6

[Andrei Eftimie]

Comment on attachment 8510983 [details] [diff] [review]
skip patch

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

Disabled:
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/18333b956bc3 (default)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/3a5440dc4bdb (mozilla-aurora)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/932474404191 (mozilla-beta)

Comment 7

[Andrei Eftimie]

Just a note that we haven't confirmed this to be fixed by mozmill 2.0.9 with bug 1082077
At this stage it's just as likely to be something else.

Comment 8

[Teodor Druta]

I tried to reproduce this bug locally with both mozmill 2.0.8.1 and 2.0.9 using an Ubuntu 13.10 machine and locale builds from the reports, it didn't failed at all.

Comment 9

[Teodor Druta]

There are no more failures since mozmill 2.0.9 landed (24-10-2014) for both release and nightly.
I think this is fixed now.

http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2014-10-25&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

http://mozmill-daily.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2014-10-25&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

Comment 10

[Teodor Druta]

(In reply to Teodor Druta from comment #9)
> There are no more failures since mozmill 2.0.9 landed (24-10-2014) for both
> release and nightly.
> I think this is fixed now.
> 
> http://mozmill-release.blargon7.com/#/remote/
> failure?app=Firefox&branch=All&platform=All&from=2014-10-
> 25&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.
> js&func=testDisplayCertificateStatusAfterRestart
> 
> http://mozmill-daily.blargon7.com/#/remote/
> failure?app=Firefox&branch=All&platform=All&from=2014-10-
> 25&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.
> js&func=testDisplayCertificateStatusAfterRestart

Please disregard this comment, Mozmill 2.0.9 isn't in production yet and also this test is skipped

Will continue investigating this bug locally.

Comment 11

[Daniela Domnici]

 I tried to reproduce it locally with mozmill 2.0.9 using an Ubuntu 13.10 machine, it didn't fail at all.Here are the results:
http://mozmill-crowd.blargon7.com/#/remote/report/6ceccc4004d363e02f4bec5e0e5ea5b9 .

Comment 12

[Andreea Matei [:AndreeaMatei]]

Before enabling this again, I'd like it checked on an Ubuntu machine from staging, with latest beta.

Comment 13

[Daniela Domnici]

I tried to reproduce it using Ubuntu 13.10 production machine. It didn`t fail at all. Here are the results: http://mozmill-crowd.blargon7.com/#/remote/report/8469cd03fbfbd17b47a7e8cdb312b46f

Comment 14

[Andreea Matei [:AndreeaMatei]]

Ok, let's enable this and see how it behaves.
http://hg.mozilla.org/qa/mozmill-tests/rev/b897645d673e (default)

Comment 15

[Daniela Domnici]

Here are the results for different branches:
Nightly(local machine):http://mozmill-daily.blargon7.com/#/remote/report/8469cd03fbfbd17b47a7e8cdb321d224
Aurora(local machine): http://mozmill-crowd.blargon7.com/#/remote/report/8469cd03fbfbd17b47a7e8cdb3839e2e
Beta(production machine): http://mozmill-crowd.blargon7.com/#/remote/report/8469cd03fbfbd17b47a7e8cdb312b46f

Andreea, I think we can backport it.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/77972bff784c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/38c1b700b1a1 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/18f0bfedeb9a (release)

Thanks for checking this Daniela. Marking as fixed by Mozmill 2.0.9 release.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

This fails again on 35.0beta/Ubuntu .

Comment 18

[Andreea Matei [:AndreeaMatei]]

Skip patch does not apply anymore. It's strange it reproduced again, Daniela did a testrun on a production machine for beta with ko locale. Must be intermittent. Lets update the skip patch.

Comment 19

[Daniela Domnici]

Attachment: skipSSL.patch

Updated skip patch for beta branch.

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8537723 [details] [diff] [review]
skipSSL.patch

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

Disabled: 
http://hg.mozilla.org/qa/mozmill-tests/rev/7a4d80a2418c (beta)

Comment 21

[Andreea Matei [:AndreeaMatei]]

Mihaela, I just checked the reports, what failed with latest beta was another failure with waitForPageLoad not the one in the summary:
http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=35&platform=All&from=2014-12-15&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart
It should have been a separate bug, the remote page might have been down.
Today is not reproducing anymore, Daniela tested on staging. I'll backout the skip patch:
http://hg.mozilla.org/qa/mozmill-tests/rev/fa6edeb06f56 (beta)

Comment 22

[Andreea Matei [:AndreeaMatei]]

Disabled on beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/7f7bade7f0b1 (beta)

It appears only on production we can reproduce after all. Daniela is investigating after todays runs.

Comment 23

[Daniela Domnici]

Attachment: fix_patch

Bug fix:'Panel is in the correct state - 'closed' should equal 'open' in toolbars.js for testSSLStatusAfterRestart.js.

Here are the results(multiple testruns):
Nightly : http://mozmill-crowd.blargon7.com/#/remote/reports?app=All&branch=37&platform=All&from=2014-12-23&to=2014-12-23 ;

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8540671 [details] [diff] [review]
fix_patch

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

::: firefox/lib/toolbars.js
@@ +1429,5 @@
>  
>    var eventType = "popupshown";
>    if (open) {
>      if (spec.panel.getNode()) {
> +      assert.waitFor(() => (spec.panel.getNode().state == "closed"),

Lets use triple =.

@@ +1437,5 @@
>        spec.parent = spec.parent || spec.panel._defaultView;
>      }
>    }
>    else {
> +    assert.waitFor(() => (spec.panel.getNode().state == "open"),

Same here.

Comment 25

[Daniela Domnici]

Attachment: fix_patch_V1

Changes requested were added.
Full testrun results:
Nightly: http://mozmill-crowd.blargon7.com/#/remote/report/f4189e259b668417231c4d9ed0a8bab3

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8540686 [details] [diff] [review]
fix_patch_V1

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

Just to mention, Daniela tested this fix with nightly, aurora and beta, on a affected production machine where she could reproduce before. Did about 40 testruns for each branch.

Comment 27

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

Comment on attachment 8540686 [details] [diff] [review]
fix_patch_V1

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

r=me with the nits fixed. Thanks for analyzing and testing the fix Daniela!

::: firefox/lib/toolbars.js
@@ +1430,5 @@
>    var eventType = "popupshown";
>    if (open) {
>      if (spec.panel.getNode()) {
> +      assert.waitFor(() => (spec.panel.getNode().state === "closed"),
> +                     "Panel is in the correct state");

I think we should really say "Panel is in closed state".

@@ +1438,5 @@
>      }
>    }
>    else {
> +    assert.waitFor(() => (spec.panel.getNode().state === "open"),
> +                   "Panel is in the correct state");

Same here for open.

Comment 28

[Daniela Domnici]

Attachment: fix_patch_V2

Changes requested were added.

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8542139 [details] [diff] [review]
fix_patch_V2

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

Great, lets see how it works.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Seems I forgot to add the link here:
http://hg.mozilla.org/qa/mozmill-tests/rev/ab3e7cf39ec3 (default)
I'll watch todays runs and if all is fine we can backport.

Comment 31

[Mihaela Velimiroviciu (:mihaelav)]

This failed again with latest beta (36.0 beta 1):
http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2015-01-06&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

Comment 32

[Mihaela Velimiroviciu (:mihaelav)]

This should be skipped on Beta as we got a lot of failures again.

Comment 33

[Daniela Domnici]

Attachment: skip SSl on beta

Created a new skip patch for beta branch.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8549482 [details] [diff] [review]
skip SSl on beta

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

This fails only on linux, lets update the manifest for that.

Comment 35

[Daniela Domnici]

Attachment: skip SSL for linux machine

Created a new skip pach for linux machine.

Comment 36

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8549496 [details] [diff] [review]
skip SSL for linux machine

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

http://hg.mozilla.org/qa/mozmill-tests/rev/f74f426c606d (beta)

Comment 37

[Cosmin Malutan, [:cosmin-malutan]]

I could reproduce this about once in 10 tries, but I would't skip it on default and aurora.
This test has an fallout failure after the restart:
> controller.waitForPageLoad(URI=https://ssl-ev.mozqa.com/, readyState=complete)
We don't have to worry about that once we fix the notification failure. 

We open the notification by clicking on the identity box, this should have the security state of the page opened in the respective tab. But if the page in cause has no valid proxystate, the handler will return early:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=63680efe6d55#7287

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

As I said in the comment above, in order to be able to open the notification, the proxy state of the page has to be valid(loaded), here is the fix-patch. 

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

Comment 39

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8551665 [details] [diff] [review]
patch v3.0

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

Looks good, but I have one suggestion.

::: firefox/lib/toolbars.js
@@ +1007,5 @@
>      waitForNotificationPanel(aCallback, {open: spec.open, panel: panel});
> +  },
> +
> +  /**
> +   * Waits for the proxy state of the current page

Nit: "Wait for the proxy state of the current page to be valid" or "Wait for a certain proxy state of the current page" (depending on what you do for the next proposal)

@@ +1009,5 @@
> +
> +  /**
> +   * Waits for the proxy state of the current page
> +   */
> +  waitForProxyState : function locationBar_waitForProxyState() {

To be clearer for what state we are waiting for, I suggest to either rename the function to waitForProxyStateValid() or to add an argument to the function that holds the state value we are waiting for (the second option will allow us to wait for other states as well if we need to)

Comment 40

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Done, thanks.

Comment 41

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8551771 [details] [diff] [review]
patch v4.0

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

One of you should add me so we can move further with the fix :) Looks good to me.

Comment 42

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

Comment on attachment 8551771 [details] [diff] [review]
patch v4.0

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

Some nits, but with those fixed r=me.

::: firefox/lib/toolbars.js
@@ +1007,5 @@
>      waitForNotificationPanel(aCallback, {open: spec.open, panel: panel});
> +  },
> +
> +  /**
> +   * Wait for a certain proxy state of the current page

Please add a bit more what the proxy state actually is. This might be very confusing for users of this method.

@@ +1016,5 @@
> +  waitForProxyState : function locationBar_waitForProxyState(aState="valid") {
> +    var urlbar = this.getElement({type: "urlbar"});
> +
> +    assert.waitFor(() => urlbar.getNode().getAttribute("pageproxystate") === aState,
> +                   "Current page is in the expected proxy state!");

So far as I got it is about the URL and not the page. So "Current URL has the proxy state: aState" would be better, which also includes the expected value.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +78,2 @@
>  
>      cert = security.getCertificate(tabBrowser.securityUI);

I would put the blank line before the call to waitForProxyState() given that it is closer related to the verify function as for loading the page.

@@ +93,2 @@
>  
>      cert = security.getCertificate(tabBrowser.securityUI);

Same here.

Comment 43

[Cosmin Malutan, [:cosmin-malutan]]

This failed for 15 times on the today's Aurora ran. 
http://mozmill-daily.blargon7.com/#/remote/failure?app=Firefox&branch=37&platform=All&from=2015-01-20&to=2015-01-21&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatus

Comment 44

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.1

I ran the test alone for checking.
If we don't wan't to skip the test on aurora until tomorrow we should backport it straight to aurora.

Comment 45

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8552428 [details] [diff] [review]
patch v4.1

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

We're ready to land with this.

::: firefox/lib/toolbars.js
@@ +1007,5 @@
>      waitForNotificationPanel(aCallback, {open: spec.open, panel: panel});
> +  },
> +
> +  /**
> +   * Wait current URL to have the expected pageproxystate state

Wait for the ..

Comment 46

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.2

Done

Comment 47

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/92bd95dfd28f (default)

Comment 48

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/15092cffb500 (aurora)

Comment 49

[Cosmin Malutan, [:cosmin-malutan]]

Andreea, could you backport this to beta and close the bug please? 
http://mozmill-crowd.blargon7.com/#/remote/report/7485d49d3e5bf7c4685a1f78860457c1

Comment 50

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/15d0244bf78c (beta fix)
https://hg.mozilla.org/qa/mozmill-tests/rev/4defb6e26e02 (beta unskip)

Comment 51

[Daniela Domnici]

Failed again with the latest beta (36.0b5).

Reports: http://mozmill-release.blargon7.com/#/remote/report/7485d49d3e5bf7c4685a1f78866d164b

Comment 52

[Daniela Domnici]

Reports:
http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2015-01-29&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart

Comment 53

[Cosmin Malutan, [:cosmin-malutan]]

This failed because the initial fix didn't get back-ported to beta and release:
http://hg.mozilla.org/qa/mozmill-tests/rev/ab3e7cf39ec3

Comment 54

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/7a59961dfd27 (beta)

Comment 55

[Cosmin Malutan, [:cosmin-malutan]]

Andreea, could you backport this patch to release, we need it to re-enable  tests like the one in bug 1096178.

Comment 56

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/ede05e1c365e (release)