Closed Bug 785041 Opened 10 years ago Closed 10 years ago

Mozmill test failure testSecurity/testGreyLarry.js with "Controller.assertJSProperty(ID: page-proxy-favicon) : http://localhost:43336/images/mozilla_favicon.ico == http://localhost:43336/favicon.ico "

Categories

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

defect

Tracking

(firefox-esr10 wontfix)

RESOLVED WONTFIX
Tracking Status
firefox-esr10 --- wontfix

People

(Reporter: vladmaniac, Assigned: AndreeaMatei)

References

()

Details

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

Attachments

(2 files, 6 obsolete files)

This is failing on mozilla-esr10, with Firefox 10.0.7pre with the detailed log message as follows: 

([object Object],"src","http://localhost:43336/images/mozilla_favicon.ico")@resource://mozmill/modules/controller.js:1035 ()@resource://mozmill/modules/frame.js -> file:///tmp/tmps0ZTPG.mozmill-tests/tests/functional/testSecurity/testGreyLarry.js:25 ((function () {controller.open(LOCAL_TEST_PAGE);controller.waitForPageLoad();var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");controller.assertJSProperty(favicon, "src", LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico");controller.assertValue(new elementslib.ID(controller.window.document, "identity-icon-label"), "");controller.click(new elementslib.ID(controller.window.document, "identity-box"));var doorhanger = new elementslib.ID(controller.window.document, "identity-popup");controller.waitFor(function () {return doorhanger.getNode().state === "open";}, "Identity popup has been opened");controller.assertJSProperty(doorhanger, "className", "unknownIdentity");var moreInfoButton = new elementslib.ID(controller.window.document, "identity-popup-more-info-button");controller.click(moreInfoButton);utils.handleWindow("type", "Browser:page-info", checkSecurityTab);}))@resource://mozmill/modules/frame.js:591 ([object Object])@resource://mozmill/modules/frame.js:661 ([object Object])@resource://mozmill/modules/frame.js:707 ("/tmp/tmps0ZTPG.mozmill-tests/tests/functional/testSecurity/testGreyLarry.js")@resource://mozmill/modules/frame.js:540 ("/tmp/tmps0ZTPG.mozmill-tests/tests/functional/testSecurity/testGreyLarry.js")@resource://mozmill/modules/frame.js:719 ((function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:179 ("03e5c76a-ed19-11e1-aea0-005056a2afd5",(function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:183 

Report link: http://mozmill-ci.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee69d4d25 

Note: this is not related to bug 741392, which is indeed aprox the same error, but was fixed easily by changing the link. This is not the case, I've tested. Its intermittent and we would probably have to wait for the icon like we do in testDVCertificate.js under the mozmill-tests/tests/functional/testSecurity folder structure.
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-test-failure]
* the same fix as for DV Cert test
* only for esr branch at the moment since here we fail sometimes
Attachment #654586 - Flags: review?(hskupin)
Comment on attachment 654586 [details] [diff] [review]
[mozilla-esr10] proposed fix patch v1.0

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

::: tests/functional/testSecurity/testGreyLarry.js
@@ +23,5 @@
>    // Check the favicon
>    var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
> +  controller.waitFor(function () {
> +    return favicon.getNode().src.indexOf("images/mozilla_favicon.ico") !== -1;
> +  }, "Favicon is loaded");

I don't see why we have to handle that differently as the other bug. Here another favicon URL is present before. Which means another page had been loaded.

I would suggest we really close all the tabs in setupModule before starting the test. That way we can do a check != ''.
Attachment #654586 - Flags: review?(hskupin) → review-
I really agree with your comment 2 judging by the error message in the summary. 

But still please dump(favicon.getNode().src); and see that the page has not changed. 
(Keep in mind that this is esr branch, on the other branches this test is slightly different)
Not sure what you are trying to say. Since when the page has not changed? What we miss here is the right test initialization.
Assignee: vlad.mozbugs → andreea.matei
Attached patch patch v2 [esr10] (obsolete) — Splinter Review
Taking this since Vlad's on PTO. 
We are now closing all tabs before starting the test and check that the value of favicon.getNode().src != ''. 
This is for ESR, on other branches I don't see failures.
Attachment #654586 - Attachment is obsolete: true
Attachment #655914 - Flags: review?(hskupin)
Attachment #655914 - Flags: review?(dave.hunt)
Comment on attachment 655914 [details] [diff] [review]
patch v2 [esr10]

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

In general it looks fine and yes it only affects the esr10 branch because newer branches do no longer show the favicon but a lock. There is one thing I would like to get addressed.

::: tests/functional/testSecurity/testGreyLarry.js
@@ +24,5 @@
>    controller.waitForPageLoad();
>  
>    // Check the favicon
>    var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
> +  controller.assertNotJSProperty(favicon, "src" , '');

Please do not flip the test. We want to explicitly compare with the target URL of the favicon. You should also probably use expect.* or waitFor().
Attachment #655914 - Flags: review?(hskupin)
Attachment #655914 - Flags: review?(dave.hunt)
Attachment #655914 - Flags: review-
Attached patch patch v3 [esr10] (obsolete) — Splinter Review
We are waiting now for the right favicon (mozilla_favicon.ico) to load.
Attachment #655914 - Attachment is obsolete: true
Attachment #655923 - Flags: review?(hskupin)
Attachment #655923 - Flags: review?(dave.hunt)
Comment on attachment 655923 [details] [diff] [review]
patch v3 [esr10]

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

::: tests/functional/testSecurity/testGreyLarry.js
@@ +10,5 @@
>  const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
>  
>  var setupModule = function(module) {
>    controller = mozmill.getBrowserController();
> +  

nit: two blanks

@@ +32,1 @@
>    controller.assertJSProperty(favicon, "src" , LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico");

You wanna wait for the full URL as done in assertJSProperty right now. Once done we don't need the latter anymore.
Attachment #655923 - Flags: review?(hskupin)
Attachment #655923 - Flags: review?(dave.hunt)
Attachment #655923 - Flags: review-
Attached patch patch v4 [esr10] (obsolete) — Splinter Review
Waiting for the full URL and removed the assertJS.
Attachment #655923 - Attachment is obsolete: true
Attachment #655942 - Flags: review?(hskupin)
Attachment #655942 - Flags: review?(dave.hunt)
Comment on attachment 655942 [details] [diff] [review]
patch v4 [esr10]

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

::: tests/functional/testSecurity/testGreyLarry.js
@@ +26,5 @@
>    // Check the favicon
>    var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
> +  controller.waitFor(function () {
> +    return favicon.getNode().src == LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico";
> +  }, "Favicon is loaded.");

Two more things here: Please construct the URL before you call waitFor(), so it hasn't to be concatenated for each iteration. Also the comparison has to be done via a triple operator.
Attachment #655942 - Flags: review?(hskupin)
Attachment #655942 - Flags: review?(dave.hunt)
Attachment #655942 - Flags: review-
Attached patch patch v4.1 [esr10] (obsolete) — Splinter Review
Created the url outside waitFor and used triple operator for comparison.
Attachment #655942 - Attachment is obsolete: true
Attachment #655980 - Flags: review?(hskupin)
Attachment #655980 - Flags: review?(dave.hunt)
OS: Linux → All
Hardware: x86 → All
Comment on attachment 655980 [details] [diff] [review]
patch v4.1 [esr10]

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

::: tests/functional/testSecurity/testGreyLarry.js
@@ +24,5 @@
>    controller.waitForPageLoad();
>  
>    // Check the favicon
>    var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon");
> +  var targetUrl = LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico";

I haven't mentioned it in my last review but would you mind moving this up as constant with a better name?
Attachment #655980 - Flags: review?(hskupin)
Attachment #655980 - Flags: review?(dave.hunt)
Attachment #655980 - Flags: review-
Updated the URL as const.
Attachment #655980 - Attachment is obsolete: true
Attachment #656395 - Flags: review?(hskupin)
Attachment #656395 - Flags: review?(dave.hunt)
Comment on attachment 656395 [details] [diff] [review]
patch v4.2 [esr10]

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

::: tests/functional/testSecurity/testGreyLarry.js
@@ +8,5 @@
>  
>  const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/");
>  const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
>  
> +const FAVICON_URL = LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico";

Those constants should be in a single block because they are bound together. I will remove the empty line before the check-in.
Attachment #656395 - Flags: review?(hskupin)
Attachment #656395 - Flags: review?(dave.hunt)
Attachment #656395 - Flags: review+
(In reply to Henrik Skupin (:whimboo) from comment #14)
> >  const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
> >  
> > +const FAVICON_URL = LOCAL_TEST_FOLDER + "images/mozilla_favicon.ico";
> 
> Those constants should be in a single block because they are bound together.
> I will remove the empty line before the check-in.

Well, we can leave it that way given that the favicon is a target and not a source.

Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/454f62bef29f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We got a new test failure today:
http://mozmill-ci.blargon7.com/#/functional/report/671677a5d9d5ca25f3cf5ae1c45b5c31

"Favicon is loaded."

So the proposed patch here isn't working. Now we even have lesser information. I'm going to backout the patch so we can work on a better solution.

http://hg.mozilla.org/qa/mozmill-tests/rev/4e5d1812f44a
Status: RESOLVED → REOPENED
Priority: -- → P2
Resolution: FIXED → ---
Since we are getting rid of assertJSProperty() in bug 789916, maybe we should use an assert here too, checking for favicon src. There are many assertJSProperty() in this test.
It will not help us in getting the failure fixed. It will only allow us to see more detailed information. We had something like that with the last patch but as it seems it didn't help. So there is something else which is wrong here.
Status: REOPENED → NEW
While I was waiting for a feedback I've changed all assertJSProperty() with asserts, did testrun and 100-times-single runs on Mac OS X and Ubuntu and wasn't able to reproduce it. This change will be done anyway in the other bug.
Will look further, inspecting again.
Andreea, thanks. But please work on the scrumbugs first.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=q3 u=failure c=security p=1
Status: NEW → ASSIGNED
I did testruns on Mac OS X 10.7.4 heavy loaded and Ubuntu 12.04 64 bit and it didn't reproduced. Still running them though.
You can see reports here:
http://mozmill-crowd.blargon7.com/#/functional/reports?branch=10.0&platform=All&from=2012-09-20&to=2012-09-20

On mozmill-ci it's rare too, the last report failing was on Sept 15th on Windows 8.
Please also create a minimized testcase which iterates over the affected code hundreds of times. I would assume that will make it better reproducible.
This failure appears daily now. So please lets get it skipped immediately and we can work on a fix by next week.
Whiteboard: [mozmill-test-failure] s=q3 u=failure c=security p=1 → [mozmill-test-failure] s=121008 u=failure c=security p=1
Attached patch skip patch [esr10] (obsolete) — Splinter Review
Disables test on ESR10.
Attachment #667869 - Flags: review?(hskupin)
Attachment #667869 - Flags: review?(dave.hunt)
Comment on attachment 667869 [details] [diff] [review]
skip patch [esr10]

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

::: tests/functional/testSecurity/manifest.ini
@@ +1,5 @@
>  [testDVCertificate.js]
>  [testEncryptedPageWarning.js]
>  [testGreenLarry.js]
>  [testGreyLarry.js]
> +disabled = Bug 785041 - Mozmill test failure testSecurity/testGreyLarry.js with "Controller.assertJSProperty(ID: page-proxy-favicon) : http://localhost:43336/images/mozilla_favicon.ico == http://localhost:43336/favicon.ico"

There is no need to list the full path of the test. It's already part of the filename of the test itself. Just strip the whole first part.

::: tests/functional/testSecurity/testGreyLarry.js
@@ +86,5 @@
>   * Map test functions to litmus tests
>   */
>  // testLarryGrey.meta = {litmusids : [8806]};
> +
> +setupModule.__force_skip__= "Bug 785041 - Mozmill test failure testSecurity/testGreyLarry.js " +

Same here.
Attachment #667869 - Flags: review?(hskupin)
Attachment #667869 - Flags: review?(dave.hunt)
Attachment #667869 - Flags: review-
Updated as requested.
Attachment #667869 - Attachment is obsolete: true
Attachment #667895 - Flags: review?(hskupin)
Attachment #667895 - Flags: review?(dave.hunt)
Attachment #667895 - Flags: review?(hskupin)
Attachment #667895 - Flags: review?(dave.hunt)
Attachment #667895 - Flags: review+
Comment on attachment 667895 [details] [diff] [review]
skip patch (esr10) [checked-in]

Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/52cc9f7aaef2
Attachment #667895 - Attachment description: skip patch [esr10] → skip patch (esr10) [checked-in]
Lets get the test disabled on Litmus for Firefox 10.
Flags: in-litmus?(andreea.matei)
Whiteboard: [mozmill-test-failure] s=121008 u=failure c=security p=1 → [mozmill-test-failure][mozmill-test-skipped] s=121008 u=failure c=security p=1
Flags: in-litmus?(andreea.matei) → in-litmus+
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=121008 u=failure c=security p=1 → [mozmill-test-failure][mozmill-test-skipped]
Assignee: andreea.matei → nobody
Status: ASSIGNED → NEW
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped] s=121112 u=failure c=security p=1
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
This is not reproducible anymore for me. Tried on OS X and Linux, latest and older builds of ESR10 to find a regression but no luck.
Also tried running the part of the code that fails 100 times (removing history and closing tabs after each iteration) on heavy loaded system and it didn't fail.
I have backed-out the test on esr10. Lets see if it sticks. If we don't get failures lets close the bug by end of the week.

http://hg.mozilla.org/qa/mozmill-tests/rev/e7094e38f102 (backout)
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
Hasn't failed these days. I think it's safe to close.
Looked over this on Windows 8 and what I've noticed manually is that the page's favicon is the seamonkey one (blue). This is also available if I open the esr10 with mozmill, the src has the seamonkey_favicon.ico
But when ran the test, the favicon is the mozilla_favicon.ico


This is available on linux also, not just windows 8.
The page we are using overrides the favicon:
http://hg.mozilla.org/qa/mozmill-tests/file/d30f57afd481/data/layout/mozilla.html

Could be that this happens now because I have copied the mozilla favicon into the servers root folder which is now the default. But that doesn't explain why it's differently used between manual vs. automatic.
Happened today on Ubuntu 12.04 (x86_64) and 10.0.11esrpre:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d371671b9076
Happened today with Firefox 10.0.11esrpre on Windows NT 6.1.7601 Service Pack 1 (x86)
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167cc98e6
Another one on Windows 7:
http://mozmill-ci.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db07fce1

For what I've investigated here and searched, when we fail the custom favicon is not loaded, it's default (the empty one with grey border lines). This is known to happen since Firefox 14 when URL favicons were removed due to security reasons and replaced with default lockpads and grey globes. 

By using an external webpage (Mozilla Mission online page) the test stops failing, just with our local page we fail.
I have tried reloading it in a for loop, dumping favicon src and clearing the history & cache, but it seems the page simply takes the default value instead of custom favicon.
Can this be reproduced manually? Also not sure what has been changed here in the meantime since the bug has been filed. If we wouldn't use any favicon as you state the src would be empty, but as the summary says we are using a different one.
If you can reproduce it yourself by doing manual steps it sounds like a bug in Firefox. Please find the regression range if possible.
(In reply to Henrik Skupin (:whimboo) from comment #42)
> If you can reproduce it yourself by doing manual steps it sounds like a bug
> in Firefox. Please find the regression range if possible.

And steps to reproduce reliably please.
For the error where favicon src = "" :
Checked again and I can reproduce manually on Release, ESR17, Beta, Aurora, Nightly with online pages as well, not just our local ones.
Simply opened the browser and from File - Open File selected our mozilla.html in data/layout folder from the repository and the favicon will be the grey globe (default favicon from version 14 up).

On ESR10 manually doing the above steps, the correct favicon loads.
If using apache server and open the page from localhost, our favicon from images folder doesn't get loaded. Instead it's a grey square with doted border (src = "") - that being the default one on esr10. But if we have another favicon in the root folder, that one gets loaded.

So the error from the summary is most likely because of what Henrik mentioned in comment 36, the favicon in the root has priority and gets loaded before what we have in our html page.

From reports, on Windows we get only the src = "" error and on Linux we have both (another favicon src or src = "").
(In reply to Andreea Matei [:AndreeaMatei] from comment #44)
> For the error where favicon src = "" :
> Checked again and I can reproduce manually on Release, ESR17, Beta, Aurora,
> Nightly with online pages as well, not just our local ones.
> Simply opened the browser and from File - Open File selected our
> mozilla.html in data/layout folder from the repository and the favicon will
> be the grey globe (default favicon from version 14 up).

Right. That's because we do not show the favicon anymore for pages served via HTTP. If we have such a check in the test it needs an update.

> On ESR10 manually doing the above steps, the correct favicon loads.

Right, because the before mentioned change went in for 16.0 afair.

> So the error from the summary is most likely because of what Henrik
> mentioned in comment 36, the favicon in the root has priority and gets
> loaded before what we have in our html page.

Can you please read the spec to see how we should behave correctly? Depending on we should udpate the test and/or test file. 

> From reports, on Windows we get only the src = "" error and on Linux we have
> both (another favicon src or src = "").

Sounds scary. We should figure out why that happens. As you say it works in Fx10? When did it regress?
FWIW, failed today with 10.0.12esr release candidate:
http://mozmill-ci.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129f503030
Flags: needinfo?(andreea.matei)
This is the step we're interested in, as shown on the testcase id 41365 for ESR10 from litmus (haven't found it on moztrap):
1. Load http://www.mozilla.com/en-us/

And expected result:
1. The site identity button to the left of the location bar should appear grey with a favicon and no site name

So if the page has a favicon, it should load it. If that fails, it will try to load the root one (that's the summary error). If there's no page favicon or fails at the root one as well, it will load the grey square (default, ith src = "").

The other tests using favicon checks (DVCertificate and GreenLary) aren't with local pages, this also triggers me. Can't we try with the mozqa.com page that has the same favicon?
Flags: needinfo?(andreea.matei)
(In reply to Andreea Matei [:AndreeaMatei] from comment #48)
> The other tests using favicon checks (DVCertificate and GreenLary) aren't
> with local pages, this also triggers me. Can't we try with the mozqa.com
> page that has the same favicon?

We can't move those other tests to local testcases because they need to run against real SSL sites which we cannot mimic locally. Recently this failure happened really rarely. So I assume it's still hard to debug, right? I feel this exercises a bug in Firefox 10 under certain conditions.
Priority: P2 → P3
Yes, but with those real sites, they don't fail at the favicon check.
So I was thinking about making this testGreyLarry to use mozqa.com, instead of the local page. It has a favicon (the same as our local page) and a grey larry.
We do not want to workaround a possible bug in Firefox. It may not be that important and it will most likely not be fixed but we should figure out why it happens and file a bug for it.
Support for Firefox 10.0esr will be dropped soon. We don't want to fix it for that branch.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → WONTFIX
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.