987098 - Failure 'Argument 1 of Window.getComputedStyle is not an object' in testPopups\testPopupsBlocked.js

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

We had 1 failure today on Nightly en-US using Windows Vista 32.

http://mozmill-daily.blargon7.com/#/functional/report/97976110c92e838016d17f7fb217f9a2


Seems that the tab haven't been found at this line 
http://hg.mozilla.org/qa/mozmill-tests/file/13f643c45310/firefox/lib/tabs.js#l553

I'll try to reproduce this.

Comment 1

[Daniel Gherasim]

Failed again with Nightly de on Windows 7 32.
http://mozmill-daily.blargon7.com/#/functional/report/97976110c92e838016d17f7fb23051d5

I could reproduce this 2 times in 150 runs, will look into the code now to find a reason.

Comment 2

[Daniel Gherasim]

Failed again with Nightly en-US (20140325030201) on Ubuntu 13.10 64 mm-ub-1310-64-1
http://mozmill-daily.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c9708b6a3

Comment 3

[Daniel Gherasim]

Failed today with Nightly fr (20140325030201) on Windows 7 32 (mm-win-7-32-4)
http://mozmill-daily.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c9714cc14

I can reproduce this locally pretty often (2-4 times out of 10). But with a sleep(500) before getting the button from the popup blocker, the failures are gone.

> // Open the Pop-up test site
> controller.open(TEST_DATA);
> controller.waitForPageLoad();
> controller.sleep(500);
> // Check for the close button in the notification bar
> var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>                                            '/{"value":"popup-blocked"}/anon({"type":"warning"})' +
>                                            utils.australis.getElement("close-button"));

Found out that's actually a regression in firefox, here is the range:
--
1395368550 03/21/2014   03:14:00 AM
No failure in 500 runs
https://hg.mozilla.org/mozilla-central/rev/c148f0b0c8b4

1395411571 03/21/2014   04:07:00 PM
20 failures in 70 runs
https://hg.mozilla.org/mozilla-central/rev/199e65efd08b
--
RR: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c148f0b0c8b4&tochange=199e65efd08b

Mosty likely it's because of bug 933462.
The fix will may be to wait for an event until the popup it's really opened.

Comment 4

[Andrei Eftimie]

Good find Daniel, we should change our test to wait for one of the events mentioned in bug 933462

Comment 5

[Daniel Gherasim]

Failed again on Nightly it on Ubuntu 12.04 64 (mm-ub-1204-64-4).
http://mozmill-daily.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c9773f854

I'll take this.

Comment 6

[Daniel Gherasim]

Attachment: bug_987098_fix.patch

This is a workaround patch to wait for the transitionend event when opening the popup. 

As a follow-up patch, we should refactor waitForTabPanel to use this event with a callback that triggeres it's opening. That also means refactoring testSafeBrowsingNotificationBar.js.

Comment 7

[Daniel Gherasim]

Attachment: bug_987098_fix.patch

This is the right patch, sorry!

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8399394 [details] [diff] [review]
bug_987098_fix.patch

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

::: firefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +51,5 @@
> +    assert.waitFor(() => transitionend, "Notification bar has been opened");
> +  }
> +  finally {
> +    controller.window.removeEventListener("transitionend", onTransitionend);
> +  }

I'd better have this test skipped if it fails that often and move this code directly where it belongs, even if other tests need changes then. It's an enhancement we should have on all of them.

Comment 9

[Daniel Gherasim]

Attachment: bug_987098_fix_2.patch

I have updated waitForTabPanel() to wait for the transionend event and also the tests that uses this.

http://mozmill-crowd.blargon7.com/#/functional/reports?app=All&branch=All&platform=All&from=2014-03-01&to=2014-04-01
http://mozmill-crowd.blargon7.com/#/remote/reports?app=All&braanch=All&platform=All&from=2014-04-01&to=2014-04-01

Comment 10

[Andrei Eftimie]

Comment on attachment 8399933 [details] [diff] [review]
bug_987098_fix_2.patch

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

There are still a few issues to tackle, but this looks so much better with a listener than polling the marginTop property!

::: firefox/lib/tabs.js
@@ +933,5 @@
>    /**
>     * Waits for a particular tab panel element to display and stop animating
>     *
>     * @param {number} tabIndex
>     *        Index of the tab to check

Missing aCallback entry.

@@ +937,5 @@
>     *        Index of the tab to check
>     * @param {string} elemString
>     *        Lookup string of the tab panel element
>     */
> +  waitForTabPanel: function tabBrowser_waitForTabPanel(tabIndex, aCallback, elemString) {

We should assert that aCallback exists and is a function.

@@ +939,5 @@
>     *        Lookup string of the tab panel element
>     */
> +  waitForTabPanel: function tabBrowser_waitForTabPanel(tabIndex, aCallback, elemString) {
> +    var transitionend = false;
> +    function onTransitionend() { transitionend = true; }

Proper camelCase please `onTransitionEnd`

::: firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
@@ +82,5 @@
>    // Verify the element is loaded onto the page and go to the phishing site
>    var ignoreWarningButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
>    var mainFeatureElem = new elementslib.ID(controller.tabs.activeTab, "main-feature");
> +
> +  tabBrowser.waitForTabPanel(tabBrowser.selectedIndex,() => {

nit: please leave a space after the comma

@@ +83,5 @@
>    var ignoreWarningButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
>    var mainFeatureElem = new elementslib.ID(controller.tabs.activeTab, "main-feature");
> +
> +  tabBrowser.waitForTabPanel(tabBrowser.selectedIndex,() => {
> +    controller.waitThenClick(ignoreWarningButton);

This can be ignoreWarningButton.waitThenClick()

@@ -113,5 @@
>   var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>                                              '/{"value":"blocked-badware-page"}' +
>                                              '/{"accesskey":"' + buttonAccessKey + '"}');
> -
> -  tabBrowser.waitForTabPanel(tabBrowser.selectedIndex, '/{"value":"blocked-badware-page"}');

We don't need these anymore?
I see the tabPanel opens for every individual test.

Comment 11

[Daniel Gherasim]

Attachment: bug_987098_fix_3.patch

Thanks for review, I made the changes except one:

(In reply to Andrei Eftimie from comment #10)
> @@ -113,5 @@
> >   var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
> >                                              '/{"value":"blocked-badware-page"}' +
> >                                              '/{"accesskey":"' + buttonAccessKey + '"}');
> > -
> > -  tabBrowser.waitForTabPanel(tabBrowser.selectedIndex, '/{"value":"blocked-badware-page"}');
> 
> We don't need these anymore?
> I see the tabPanel opens for every individual test.

We are waiting for the tabPanel in checkIgnoreWarningButton that's called before every test.

Comment 12

[Andrei Eftimie]

Comment on attachment 8399995 [details] [diff] [review]
bug_987098_fix_3.patch

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

For some reason this patch doesn't apply, please check that.

Comment 13

[Daniel Gherasim]

Attachment: patch v3.1

Patch that applies cleanly.

Comment 14

[Andrei Eftimie]

Comment on attachment 8400603 [details] [diff] [review]
patch v3.1

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

In general this looks good.
Just a few changes needed.

Since we're changin a lib please also run some full testruns to make sure we didn't regress anything.

::: firefox/lib/tabs.js
@@ +940,4 @@
>     * @param {string} elemString
>     *        Lookup string of the tab panel element
>     */
> +  waitForTabPanel: function tabBrowser_waitForTabPanel(tabIndex, aCallback, elemString) {

Since we're changing this, please also update the other arguments to have the "a" prefix.

@@ +940,5 @@
>     * @param {string} elemString
>     *        Lookup string of the tab panel element
>     */
> +  waitForTabPanel: function tabBrowser_waitForTabPanel(tabIndex, aCallback, elemString) {
> +    assert.ok(() => typeof aCallback === "function", "Callback function is defined");

Use assert.equal here.

@@ +951,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        var tabPanel = this.getTabPanelElement(tabIndex, elemString);
> +        return transitionEnd && tabPanel.exists();

If we use the retrieve the element directly in the return statement, we might get a small perf improvement, as we'll only retrieve it once the transition has completed.

::: firefox/tests/functional/testPopups/testPopupsBlocked.js
@@ +38,5 @@
>   *
>   */
>  var testPopUpBlocked = function() {
>    var windowCount = mozmill.utils.getWindows().length;
> +  

nit: extra whitespace

Comment 15

[Daniel Gherasim]

Attachment: bug_987098_fix_3.2.patch

Thanks,
Updated the patch and here are the testrun reports:
Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/b13d116bf1db6c174a7539f94b242639

Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/b13d116bf1db6c174a7539f94b2603f2

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8402616 [details] [diff] [review]
bug_987098_fix_3.2.patch

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

With that small update, please request review from Henrik. Thanks!

::: firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
@@ +112,5 @@
>  var checkNoPhishingButton = function(aData) {
>    // Click on the web forgery report button
>   var buttonAccessKey = utils.getProperty("chrome://browser/locale/browser.properties",
>                                           aData.buttonAccessKey);
>   var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,

I know you haven't touch these lines, but I see they are only one space intended, could you please fix that?

Comment 17

[Daniel Gherasim]

Attachment: bug_987098_fix_3.3.patch

Testrun reports for windows 8.1:
http://mozmill-crowd.blargon7.com/#/remote/report/b13d116bf1db6c174a7539f94b2998cd
http://mozmill-crowd.blargon7.com/#/functional/report/b13d116bf1db6c174a7539f94b2a55c5

Max OSX 10.6:
http://mozmill-crowd.blargon7.com/#/remote/report/b13d116bf1db6c174a7539f94b325ca4
http://mozmill-crowd.blargon7.com/#/functional/report/b13d116bf1db6c174a7539f94b3254a6

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8402677 [details] [diff] [review]
bug_987098_fix_3.3.patch

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

::: firefox/lib/tabs.js
@@ +951,5 @@
> +      aCallback();
> +
> +      assert.waitFor(() => transitionEnd, "Notification transition finished");
> +      var tabPanel = this.getTabPanelElement(aTabIndex, aElemString);
> +      assert.waitFor(() => tabPanel.exists(), "Notification bar has been opened")

Why do we need this extra waitFor? Seeing that the transition has been finished is not enough? The last two lines look like a no-op for me.

Comment 19

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 8402677 [details] [diff] [review]
> bug_987098_fix_3.3.patch
> 
> Review of attachment 8402677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tabs.js
> @@ +951,5 @@
> > +      aCallback();
> > +
> > +      assert.waitFor(() => transitionEnd, "Notification transition finished");
> > +      var tabPanel = this.getTabPanelElement(aTabIndex, aElemString);
> > +      assert.waitFor(() => tabPanel.exists(), "Notification bar has been opened")
> 
> Why do we need this extra waitFor? Seeing that the transition has been
> finished is not enough? The last two lines look like a no-op for me.

With this we could check if the correct needed notification has been opened. 
Do we preffer removing the check or changing the message to "Correct notification bar has been opened" ?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Our code is synchronous, so no other action should have taken place which could also trigger a notification panel. Also there can only be one instance open. Means they are getting replaced. But you can replace that with assert.ok(), and move it outside of the try/finally block, right at the end of the method.

Comment 21

[Daniel Gherasim]

Attachment: bug_987098_fix_3.4.patch

Ok, thanks!
Indeed it's looking better now.

Comment 22

[Andrei Eftimie]

Comment on attachment 8403151 [details] [diff] [review]
bug_987098_fix_3.4.patch

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

Looks good to me.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8403151 [details] [diff] [review]
bug_987098_fix_3.4.patch

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

Except one small nit all looks fine. Fix it and we can get it landed. Thanks.

::: firefox/lib/tabs.js
@@ +949,5 @@
>  
> +    try {
> +      aCallback();
> +
> +      assert.waitFor(() => transitionEnd, "Notification transition finished");

nit: Lets move the message to a new line.

Comment 24

[Daniel Gherasim]

Attachment: bug_987098_fix_3.5.patch

Comment 25

[Andrei Eftimie]

Comment on attachment 8404478 [details] [diff] [review]
bug_987098_fix_3.5.patch

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

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