810820 - Test failure "Modal dialog has been found and processed" in /testSecurity/testEncryptedPageWarning.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Happened today on Nightly, with all OS X and Linux.

Comment 1

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

Fails across platforms on Nightly. Putting on this weeks sprint (will remove bug 763159). Please get this investigated ASAP.

Andreea, please dont' forget to add as much as possible details to the bug.

Comment 2

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

I would suggest we skip this test and investigate the problem tomorrow.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: skip v1.0

skipping it for now since there's no one else around

Comment 4

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

Comment on attachment 680715 [details] [diff] [review]
skip v1.0

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

Please do not list the test name in the message for the skip code and the manifest. That's really not necessary.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Attachment: skip v2.0

removed test name in skip message

Comment 6

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

Comment on attachment 680753 [details] [diff] [review]
skip v2.0

Next time please use a better commit message, which reflects what the patch is actually doing.

Landed skip patch on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/e7b1bb75a413

Comment 7

[Daniela Petrovici]

I have verified the test and it fails due to the fact that https://mail.mozilla.org does not produce a security alert modal dialog anymore.

Do we need to find another site that produces the security alert modal dialog or will this site change back to the original behavior?

Comment 8

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

Why does it not happen anymore? Is it a regression or an expected change?

Comment 9

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Why does it not happen anymore? Is it a regression or an expected change?

I ran the test against the nightly build from 11/11 when the failure did not reproduce. It didn't reproduce with this build now.

Could bug 799009 be the cause of the failure?

Comment 10

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

(In reply to Daniela Petrovici from comment #9)
> I ran the test against the nightly build from 11/11 when the failure did not
> reproduce. It didn't reproduce with this build now.

Please give us a whole pushlog and as best run tinderbox builds to narrow down the range. All that is listed in the guidelines how to handle regressions like this.

> Could bug 799009 be the cause of the failure?

Sounds most likely. But please do the above steps so we can be sure it's this bug. In that case we have to adjust our test so it matches the new behavior.

Comment 11

[Daniela Petrovici]

Push log link: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf7575ceef5a&tochange=ba4903cc7523

I have found two tinderbox builds from 11/11:
- the bug does not reproduce in build 1352632069 (buildID: cf7575ceef5a)
- the bug reproduces in build 1352635242 (buildID: ba4903cc7523)

Comment 12

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

Thanks Daniela. So it's indeed a fallout from bug 799009. Please tell us how you would fix the problem so we can re-enable the test soon.

Comment 13

[Daniela Petrovici]

The prefs were removed completely from the Nightly builds. They still remain in other branches (like Aurora).

Since the test is checking if the security warning appears - which does not happen now due to bug 799009, I think we should leave the test as skipped or remove it completely for nightly branch.

Comment 14

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

We will not leave it as skipped, but we can remove it if we know that it is already covered by another automated test. As best you talk to the developer and figure out what's needed here.

Comment 15

[Daniela Petrovici]

This is not covered in any other test. I have asked Brian Smith about another option to get the security warning back in Firefox nightly build.

Comment 16

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

(In reply to Daniela Petrovici from comment #15)
> This is not covered in any other test. I have asked Brian Smith about
> another option to get the security warning back in Firefox nightly build.

What do you mean with 'other test'? We do not have any mochitests which cover that? Also we don't want to get back any warning, but will have to update our test if necessary to meet the current behavior.

Comment 17

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #12)
> Thanks Daniela. So it's indeed a fallout from bug 799009. Please tell us how
> you would fix the problem so we can re-enable the test soon.

Steps to fix the issue:
1) find a site that has mixed content. Please tell me if it is ok to use: https://blog.mozilla.org/theden/2012/10/30/8-tips-for-safe-online-shopping/
2) click on the favicon
3) check the message that appears for mixed content

Please tell me if it is ok to fix the problem using this site and steps.

Comment 18

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

(In reply to Daniela Petrovici from comment #17)
> 1) find a site that has mixed content. Please tell me if it is ok to use:
> https://blog.mozilla.org/theden/2012/10/30/8-tips-for-safe-online-shopping/

We have to use a page which lives on mozqa.com. In this case we need a new test file uploaded to the litmus-data repository.

> 2) click on the favicon
> 3) check the message that appears for mixed content

I miss the check for the favicon.

Comment 19

[Daniela Petrovici]

I will use https://quality.mozilla.org/community/ and add the check for the favicon in the test case.

Comment 20

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

(In reply to Daniela Petrovici from comment #19)
> I will use https://quality.mozilla.org/community/ and add the check for the
> favicon in the test case.

Please read my comment 18 carefully. We are not going to use QMO for that because it will kill our ability to get this test running on our upcoming ESX cluster in PHX. So please follow the steps I have proposed.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The dialogs are gone and are not coming back.

I am not sure what the test is supposed to be testing. If it is just testing that the dialog is shown, then the test should be removed. If the test is testing that we show some UI indication of mixed content, then the thing to look for is that the lock icon (next to the URL) becomes a globe when there is mixed content.

Comment 22

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

(In reply to Brian Smith (:bsmith) from comment #21)
> testing that we show some UI indication of mixed content, then the thing to
> look for is that the lock icon (next to the URL) becomes a globe when there
> is mixed content.

That's exactly what we are targeting for. Additionally we would also check the content of the doorhanger. Brian, so there is really no mochitest available which already checks that?

Comment 23

[Daniela Petrovici]

Attachment: patch v1.0

Attached the patch for testing a page containing mixed content.
I did not remove the skip from the patch. Please tell me if I need to remove it or it will be backout

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 685608 [details] [diff] [review]
patch v1.0

>diff --git a/tests/functional/testSecurity/testEncryptedPageWarning.js b/tests/functional/testSecurity/testEncryptedPageWarning.js
>--- a/tests/functional/testSecurity/testEncryptedPageWarning.js
>+++ b/tests/functional/testSecurity/testEncryptedPageWarning.js
I believe the test should be renamed, since we no longer have a warning alert.

> var testEncryptedPageWarning = function() {
Same as the above.

>+  // Load an encrypted page and wait for the security alert
There is no security alert anymore.

>+  controller.waitFor(function() {
Please use assert.waitFor(), the globe image needs to be there in order to continue the test.

>+  var property = utils.getProperty("chrome://browser/locale/browser.properties",
>+                                  "identity.mixed_content");
The indentation is wrong: one space to the left here.
 
> setupModule.__force_skip__ ="Bug 810820 - Test failure 'Modal dialog has been found and processed'";
> teardownModule.__force_skip__ ="Bug 810820 - Test failure 'Modal dialog has been found and processed'";
>-
Please leave the blank end line at the end of the test, as required by the style guide.

Thanks.

Comment 25

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

Comment on attachment 685608 [details] [diff] [review]
patch v1.0

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

Andreea mentioned nearly everything nicely. But I have two more items for you to fix.

::: tests/functional/testSecurity/testEncryptedPageWarning.js
@@ +21,5 @@
> +  // Load an encrypted page and wait for the security alert
> +  controller.open(TEST_PAGE);
> +  controller.waitForPageLoad();
> +
> +  var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");

We do not want to have element ids used in the test. Please use the locationBar class instead, which you can find in toolbars.js

@@ +30,5 @@
> +  }, "There is a globe image");
> +
> +  controller.click(favicon);
> +
> +  var owner = new elementslib.ID(controller.window.document, "identity-popup-encryption");

I would love it if we could move this to the lib too. It's already used in DVcertificate too. You can file at least a follow-up bug for it if you want.

Comment 26

[Daniela Petrovici]

Attachment: patch v2.0

Modified lib/toolbars for this test case only. I will log a new issue for modifying the lib for testDVCertificate.js and updating the test

Comment 27

[Daniela Petrovici]

Attachment: patch v2.1

Made a mistake when renaming tabbedbrowser

Comment 28

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

Comment on attachment 686104 [details] [diff] [review]
patch v2.1

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

Some more things to fix here. Also please provide test results across platforms for uploaded reports to the dashboard.

::: lib/toolbars.js
@@ +22,5 @@
>  const URLBAR_INPUTBOX = URLBAR_CONTAINER + '/id("urlbar")' +
>                                             '/anon({"anonid":"textbox-container"})' +
>                                             '/anon({"anonid":"textbox-input-box"})';
>  const CONTEXT_MENU = URLBAR_INPUTBOX + '/anon({"anonid":"input-box-contextmenu"})';
> +const IDENTITY_POPUP = 'identity-popup-encryption';

There is no need to define that popup as a constant. It's used only once. Please add it directly to the case.

::: tests/functional/testSecurity/testEncryptedPageWarning.js
@@ +8,2 @@
>  var utils = require("../../../lib/utils");
> +var toolbars = require("../../../lib/toolbars");

nit: please keep the alphabetical order.

@@ +20,5 @@
>   */
> +function testMixedContentPage() {
> +  // Load a mixed content page and wait for the page to be loaded
> +  controller.open(TEST_PAGE);
> +  controller.waitForPageLoad();

There is no need for this comment. The code here is clear enough.

@@ +25,2 @@
>  
> +  var locationBar = new toolbars.locationBar(controller);

This has to be placed in setupModule().

@@ +28,2 @@
>  
> +  assert.waitFor(function() {

nit: remove the empty line above and add a blank behind function. This should have been covered by the internal review.

Comment 29

[Daniela Petrovici]

Attachment: patch v2.2

Comment 30

[Daniela Petrovici]

Added new patch. Reports for MAC and Windows are:
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167287d44
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d3716724ee04

I cannot run on Ubuntu due to bug 797389. Please tell me if I should skip the test that cause this bug in my patch or if it is not necessary

Comment 31

[Andreea Matei [:AndreeaMatei]]

Daniela, this bug has nothing to do with bug 797389. You can still run functional testrun and provide to us the first report (for the non-restart tests). The restart tests cause a hang, but those have a different report.

Comment 32

[Daniela Petrovici]

(In reply to Andreea Matei [:AndreeaMatei] from comment #31)
> Daniela, this bug has nothing to do with bug 797389. You can still run
> functional testrun and provide to us the first report (for the non-restart
> tests). The restart tests cause a hang, but those have a different report.

Sorry about that, didn't know the report was generated if the restart test were not run. 

This is the report for Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d3716727a298

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 686641 [details] [diff] [review]
patch v2.2

Looks good, just a little update is needed:
 
>+      case "identity_popup":
>+        elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption");
>+        break;

Please move this above, in order to be alphabetically sorted.


>+function setupModule() {
>   controller = mozmill.getBrowserController();
>+  locationBar = new toolbars.locationBar(controller);
>+  
>+  tabs.closeAllTabs(controller);

Here you have whitespaces.

Also, I would advice giving a more suggestive name to the patch.
Thanks.

Comment 34

[Daniela Petrovici]

Attachment: patch v2.3

Modified patch based on review

New reports:
MAC - http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167738ec3
Windows - http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167736117
Ubuntu - http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d3716774e322

Comment 35

[Daniela Petrovici]

Attachment: patch v2.4

changed "identity_popup" to identityPopup

Comment 36

[Daniela Petrovici]

Attachment: patch 2.5

Re-added the empty line after the variable declarations in setupModule which was wrong in the previous patch

Comment 37

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 689118 [details] [diff] [review]
patch 2.5

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

Looks good to me now. Thanks!

Comment 38

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

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/6a1fb6580e28

Please watch results so we can land on aurora too.

Comment 39

[Daniela Petrovici]

No failures on testMixedContentPage since 12/06

Comment 40

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

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

Comment 41

[Daniela Petrovici]

This started failing on Mozilla release branch FF 19.0 across all platforms. The change done in this bug was not added to the mozilla-release repository and this seems to be the case of the failure.

Happened on:
Windows Vista SP2 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf885b6a
Windows 7 SP1 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf882118
http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf883047
Windows XP SP3 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf883f00
Windows 8 x64 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf885863
Windows 8 x86 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf885603
Linux x86 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf858c0c
http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf857f31
Linux x64 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf857383
http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf855365
MAC 10.7.5 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf84decd
MAC 10.6.8 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf84ea48
MAC 10.8.2 - http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf84ce63

Comment 42

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

(In reply to Daniela Petrovici from comment #41)
> This started failing on Mozilla release branch FF 19.0 across all platforms.
> The change done in this bug was not added to the mozilla-release repository
> and this seems to be the case of the failure.

No, all that has been covered by the merge from beta to release done by Anthony on Friday:

http://hg.mozilla.org/qa/mozmill-tests/rev/9862e805ef65

But yes, the merge happened too late here! Builds were already out. So we really have to do the merge earlier. For reference I will comment on bug 841803.

Comment 43

[Tanvi Vyas[:tanvi]]

Bug 838402 changes some of the identity modes, and these test cases will likely break:

tests/functional/testSecurity/manifest.ini
tests/functional/testSecurity/testEncryptedPageWarning.js
tests/functional/testSecurity/testMixedContentPage.js

Not sure what the right way to report on this is, but since they were added in this bug, I'm commenting here.

Comment 44

[Andrei Eftimie]

A couple failures on 17.0.9 (en-US, Ubuntu 13.04):
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee6972198f8744
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee6972198fd059

Comment 45

[Andrei Eftimie]

Interesting, another failure, in a different test:
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee6972198fda6f

I'm adding the report here since all these 3 failures are on the same machine.

Erm, why are these 3 failures on mozmill-crowd?

Comment 46

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

I don't understand why we are reopening bugs, which have been fixed nearly a year ago. File a new bug please.

Comment 47

[Andrei Eftimie]

No need to open a new one, it seems these were triggered by Cosmin running ondemand testruns for bug 916380

I think they shouldn't send status messages to the mozmill-ci mailing list.