Closed Bug 810820 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

(firefox18 unaffected, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- unaffected
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: AndreeaMatei, Assigned: daniela.p98911)

References

()

Details

(Whiteboard: [mozmill-test-failure] s=121112 u=failure c=security p=1)

Attachments

(2 files, 7 obsolete files)

Happened today on Nightly, with all OS X and Linux.
Whiteboard: [mozmill-test-failure]
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.
Priority: -- → P2
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=121112 u=failure c=security p=1
I would suggest we skip this test and investigate the problem tomorrow.
Attached patch skip v1.0 (obsolete) — Splinter Review
skipping it for now since there's no one else around
Attachment #680715 - Flags: review?(hskupin)
Attachment #680715 - Flags: review?(dave.hunt)
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.
Attachment #680715 - Flags: review?(hskupin)
Attachment #680715 - Flags: review?(dave.hunt)
Attachment #680715 - Flags: review-
Attached patch skip v2.0Splinter Review
removed test name in skip message
Attachment #680715 - Attachment is obsolete: true
Attachment #680753 - Flags: review?(hskupin)
Attachment #680753 - Flags: review?(dave.hunt)
Attachment #680753 - Flags: review?(hskupin)
Attachment #680753 - Flags: review?(dave.hunt)
Attachment #680753 - Flags: review+
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
Attachment #680753 - Flags: checkin+
Whiteboard: [mozmill-test-failure] s=121112 u=failure c=security p=1 → [mozmill-test-failure][mozmill-test-skipped] s=121112 u=failure c=security p=1
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?
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Why does it not happen anymore? Is it a regression or an expected change?
(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?
(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.
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)
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.
Blocks: 799009
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.
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.
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.
(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.
(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.
(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.
I will use https://quality.mozilla.org/community/ and add the check for the favicon in the test case.
(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.
Depends on: 812109
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.
(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?
Attached patch patch v1.0 (obsolete) — Splinter Review
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
Attachment #685608 - Flags: review?(hskupin)
Attachment #685608 - Flags: review?(dave.hunt)
Attachment #685608 - Flags: review?(andreea.matei)
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.
Attachment #685608 - Flags: review?(andreea.matei) → review-
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.
Attachment #685608 - Flags: review?(hskupin)
Attachment #685608 - Flags: review?(dave.hunt)
Attachment #685608 - Flags: review-
Attached patch patch v2.0 (obsolete) — Splinter Review
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
Attachment #685608 - Attachment is obsolete: true
Attachment #686096 - Flags: review?(hskupin)
Attachment #686096 - Flags: review?(dave.hunt)
Attachment #686096 - Flags: review?(andreea.matei)
Attached patch patch v2.1 (obsolete) — Splinter Review
Made a mistake when renaming tabbedbrowser
Attachment #686096 - Attachment is obsolete: true
Attachment #686096 - Flags: review?(hskupin)
Attachment #686096 - Flags: review?(dave.hunt)
Attachment #686096 - Flags: review?(andreea.matei)
Attachment #686104 - Flags: review?(hskupin)
Attachment #686104 - Flags: review?(dave.hunt)
Attachment #686104 - Flags: review?(andreea.matei)
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.
Attachment #686104 - Flags: review?(hskupin)
Attachment #686104 - Flags: review?(dave.hunt)
Attachment #686104 - Flags: review?(andreea.matei)
Attachment #686104 - Flags: review-
Attached patch patch v2.2 (obsolete) — Splinter Review
Attachment #686641 - Flags: review?(hskupin)
Attachment #686641 - Flags: review?(dave.hunt)
Attachment #686104 - Attachment is obsolete: true
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
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.
(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
Attachment #686641 - Flags: review?(andreea.matei)
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.
Attachment #686641 - Flags: review?(andreea.matei) → review-
Attachment #686641 - Flags: review?(hskupin)
Attachment #686641 - Flags: review?(dave.hunt)
Attached patch patch v2.4 (obsolete) — Splinter Review
changed "identity_popup" to identityPopup
Attachment #687720 - Attachment is obsolete: true
Attachment #687720 - Flags: review?(hskupin)
Attachment #687720 - Flags: review?(dave.hunt)
Attachment #687720 - Flags: review?(andreea.matei)
Attachment #687729 - Flags: review?(hskupin)
Attachment #687729 - Flags: review?(dave.hunt)
Attachment #687729 - Flags: review?(andreea.matei)
Attached patch patch 2.5Splinter Review
Re-added the empty line after the variable declarations in setupModule which was wrong in the previous patch
Attachment #687729 - Attachment is obsolete: true
Attachment #687729 - Flags: review?(hskupin)
Attachment #687729 - Flags: review?(dave.hunt)
Attachment #687729 - Flags: review?(andreea.matei)
Attachment #689118 - Flags: review?(hskupin)
Attachment #689118 - Flags: review?(dave.hunt)
Attachment #689118 - Flags: review?(andreea.matei)
Comment on attachment 689118 [details] [diff] [review]
patch 2.5

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

Looks good to me now. Thanks!
Attachment #689118 - Flags: review?(hskupin)
Attachment #689118 - Flags: review?(dave.hunt)
Attachment #689118 - Flags: review?(andreea.matei)
Attachment #689118 - Flags: review+
Keywords: checkin-needed
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=121112 u=failure c=security p=1 → [mozmill-test-failure] s=121112 u=failure c=security p=1
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/6a1fb6580e28

Please watch results so we can land on aurora too.
Keywords: checkin-needed
Whiteboard: [mozmill-test-failure] s=121112 u=failure c=security p=1 → [mozmill-test-failure][mozmill-test-skipped] s=121112 u=failure c=security p=1
No failures on testMixedContentPage since 12/06
http://hg.mozilla.org/qa/mozmill-tests/rev/cc703b470be6 (aurora)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=121112 u=failure c=security p=1 → [mozmill-test-failure] s=121112 u=failure c=security p=1
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Blocks: 838402
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.
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?
I don't understand why we are reopening bugs, which have been fixed nearly a year ago. File a new bug please.
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
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.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: