981517 - Test failure "Element.waitForElement(): Element 'ID: ignoreWarningButton' has been found" in testSecurity/testSafeBrowsingNotificationBar.js

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

Here is a sample report:
http://mozmill-daily.blargon7.com/#/remote/report/3ed2024184b13bf096824c080877b24c

The remote page "http://www.itisatrap.org/firefox/its-a-trap.html" has changed.
We'll need to update our code accordingly.

Daniel, can you please prepare a skip patch.

Comment 1

[Daniel Gherasim]

Attachment: disable_test_bug_981517

Patch applies cleanly on default and aurora.
On beta & esr24 the test is already disabled due to bug 946723.

I'll come up with a fix for this as soon as possible.

Comment 2

[Andrei Eftimie]

Comment on attachment 8388383 [details] [diff] [review]
disable_test_bug_981517

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

Could you please update the skip messages.
There's no need to mention the file. We _know_ which file we are referring to as we are either in the file or in the adjacent manifest.

::: firefox/tests/remote/testSecurity/manifest.ini
@@ +3,5 @@
>  [testMD5HashSignature.js]
>  [testMixedContentPage.js]
>  [testSSLDisabledErrorPage.js]
>  [testSafeBrowsingNotificationBar.js]
> +disabled = Bug 981517 - Test failure "Element.waitForElement(): Element 'ID: ignoreWarningButton' has been found" in testSecurity/testSafeBrowsingNotificationBar.js

There's no need to specify the file. This message is just below the reference to the test.

::: firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
@@ +161,5 @@
>  }
> +
> +setupModule.__force_skip__ = "Bug 981517 - Test failure \"Element.waitForElement(): " +
> +                             "Element 'ID: ignoreWarningButton' has been found\" " +
> +                             "in testSecurity/testSafeBrowsingNotificationBar.js'";

Again, we know the file.

Comment 3

[Daniel Gherasim]

Attachment: disable_test_bug_981517_v2

I'll keep that in mind next time. 
Thanks,

Comment 4

[Andrei Eftimie]

Comment on attachment 8388397 [details] [diff] [review]
disable_test_bug_981517_v2

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/de6717b0139b (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/33926fc9ffcd (mozilla-aurora)

::: firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
@@ +162,5 @@
> +
> +setupModule.__force_skip__ = "Bug 981517 - Test failure \"Element.waitForElement(): " +
> +                             "Element 'ID: ignoreWarningButton' has been found\" ";
> +teardownModule.__force_skip__ = "Bug 981517 - Test failure \"Element.waitForElement(): " +
> +                             "Element 'ID: ignoreWarningButton' has been found\" ";

nit: I didn't notice this before. I've updated it in the final patch.

Comment 5

[Daniel Gherasim]

This is a regression in firefox and I found that it started failing once bug 971240 landed.

Regression:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a2b62ced704d&tochange=b0406a8daedd

Comment 6

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

Why has this been disabled on Aurora? I don't see that the patch on bug 971240 has been backported to aurora.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Hmm, those test pages should only have changed minimally, but in fact they're now totally empty.

Comment 8

[Andrei Eftimie]

Backed out the skip for aurora.
http://hg.mozilla.org/qa/mozmill-tests/rev/e1347f0f5e3d (mozilla-aurora)

Comment 9

[Daniel Gherasim]

Attachment: bug_987517_fix.patch

Unskip & fix patch.

Reports:
http://mozmill-crowd.blargon7.com/#/remote/reports?app=All&branch=All&platform=All&from=2014-03-10&to=2014-03-10

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8388468 [details] [diff] [review]
bug_987517_fix.patch

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

::: firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
@@ +14,5 @@
>    // Phishing URL object
>    {
>      buttonAccessKey : "safebrowsing.notAForgeryButton.accessKey",
>      reportPage : "www.google.com/safebrowsing/report_error",
> +    unsafePage : "http://www.itisatrap.org/firefox/its-a-trap.html"

Would using https here work as well? Then it'd be preferable.

Comment 11

[Daniel Gherasim]

Attachment: bug_987517_fix_2.patch

It works as well. Thanks.
Updated the patch.

Comment 12

[Andrei Eftimie]

Comment on attachment 8388481 [details] [diff] [review]
bug_987517_fix_2.patch

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

Looks good, but we'll also need to update this test:
/testSecurity/testSafeBrowsingWarningPages.js

This hasn't failed since it's skipped with another problem.
Please make the same updates. You'll be able to test it since the failure there is intermittent. We won't unskip that test, but all the domain changes should be part of the same fix.

Comment 13

[Daniel Gherasim]

Attachment: bug_987517_fix_3.patch

Also updated testSafeBrowsingWarningPages.js and tested it. Works fine.

Comment 14

[Andrei Eftimie]

Comment on attachment 8389133 [details] [diff] [review]
bug_987517_fix_3.patch

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

Looks good.

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

Comment 16

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

As seen on the duplicate we still have this problem on Aurora. Not sure if that got backported somewhere?

Comment 17

[Andrei Eftimie]

I'm not sold yet that bug 983115 is a dupe. As stated there they are probably related.
I wasn't able to reproduce that failure.

Comment 18

[Andrei Eftimie]

This started failing on Aurora as well. So the changes have been backported.
http://mozmill-daily.blargon7.com/#/remote/failure?app=All&branch=29.0&platform=All&from=2014-03-14&to=2014-03-14&test=%2FtestSecurity%2FtestSafeBrowsingNotificationBar.js&func=testNotificationBar

Weird that I can't reproduce it locally using today's build.
I'll transplant it once I figure out what the problem here is.

Comment 19

[Andrei Eftimie]

Ugh, these failures are actually on the Holly branch.

Comment 20

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

If that is really holly only we can mark it as fixed again. I wouldn't worry about those also given that today it was the last build of that project branch.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Today we still had a new build on holly.
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-holly/
And we had a couples of test failures.
http://mozmill-daily.blargon7.com/#/remote/failure?app=All&branch=All&platform=All&from=2014-03-16&to=2014-03-17&test=%2FtestSecurity%2FtestSafeBrowsingNotificationBar.js&func=testNotificationBar

I asked on releng channel, and from what I understood from bhearsum, we will have an holy build if:
1) there's a newer revision than the last time we did nightlies
2) the builds on that revision are green

So if we still expect new holly builds we should fix this test for holly-branch too.

Ben is that correct?

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

To update the question.
Will any more holly builds be pushed on ftp or we expect the repository to be removed soon?
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-holly/

Comment 23

[[e-mail] (:bhearsum)]

(In reply to Cosmin Malutan from comment #22)
> To update the question.
> Will any more holly builds be pushed on ftp or we expect the repository to
> be removed soon?
> https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-holly/

Holly is not being used for Australis anymore, I highly doubt you want to be testing against it. It's being used for Electrolysis work now, which is probably going to fail tons of tests.

Comment 24

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

(In reply to Ben Hearsum [:bhearsum] from comment #23)
> Holly is not being used for Australis anymore, I highly doubt you want to be
> testing against it. It's being used for Electrolysis work now, which is
> probably going to fail tons of tests.

We are recycle project branches? I don't think that this is a good idea. Is it too hard to create a new project branch? You never know who really depends on it.

So for our case I will push the latest mozmill-ci changes to production and we should not be affected by this anymore starting by tomorrow.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Yesterday we didn't run holly anymore so everything was fine.

Comment 26

[Andrei Eftimie]

This started failing on Beta across all platforms:
http://mozmill-release.blargon7.com/#/remote/failure?app=All&branch=All&platform=All&from=2014-03-27&test=%2FtestSecurity%2FtestSafeBrowsingNotificationBar.js&func=testNotificationBar

Daniel, please check if the fix applies on the Beta branch and if it fixes the issue.

Comment 27

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

Bug 971240 was only fixed on m-c so far. So how could this even affect Aurora? I don't understand this.

Comment 28

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Bug 971240 was only fixed on m-c so far. So how could this even affect
> Aurora? I don't understand this.

That landed on mc on March 7th and we had a merge on March 17th. So it's both on 31 and 30.
These recent failures on Beta (29) do look suspicious

Comment 29

[Daniel Gherasim]

Looks like it was introduced recently on beta 

Here is the regression range:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=e5d922ae5641&tochange=363950e031c0

So bug 985623 seems to be the reason for this failure.

Comment 30

[Andrei Eftimie]

Nice find.

From what I see in bug 985623 some changes we're not backported to beta.
Mainly the changes to libpref: https://hg.mozilla.org/releases/mozilla-aurora/diff/e1282ac83e49/modules/libpref/src/init/all.js are missing from Beta.

Monica could you please check if this was intended or maybe it was missed while resolving conflict?

Comment 31

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

Good find, but then please file a new bug for this issue. There is no reason to keep this one open anytime longer.