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 "

RESOLVED WONTFIX

Status

P3
normal
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: vladmaniac, Assigned: AndreeaMatei)

Tracking

unspecified
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox-esr10 wontfix)

Details

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

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-test-failure]
(Reporter)

Updated

6 years ago
status-firefox-esr10: --- → affected
(Reporter)

Comment 1

6 years ago
Created attachment 654586 [details] [diff] [review]
[mozilla-esr10] proposed fix patch v1.0

* 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)
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
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-
(Reporter)

Comment 3

6 years ago
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)

Updated

6 years ago
Assignee: vlad.mozbugs → andreea.matei
(Assignee)

Comment 5

6 years ago
Created attachment 655914 [details] [diff] [review]
patch v2 [esr10]

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)
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
status-firefox17: affected → ---
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-
(Assignee)

Comment 7

6 years ago
Created attachment 655923 [details] [diff] [review]
patch v3 [esr10]

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-
(Assignee)

Comment 9

6 years ago
Created attachment 655942 [details] [diff] [review]
patch v4 [esr10]

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-
(Assignee)

Comment 11

6 years ago
Created attachment 655980 [details] [diff] [review]
patch v4.1 [esr10]

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-
(Assignee)

Comment 13

6 years ago
Created attachment 656395 [details] [diff] [review]
patch v4.2 [esr10]

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
Last Resolved: 6 years ago
status-firefox-esr10: affected → fixed
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 → ---
status-firefox-esr10: fixed → affected
(Assignee)

Comment 18

6 years ago
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
(Assignee)

Comment 20

6 years ago
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
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 22

6 years ago
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
(Assignee)

Comment 25

6 years ago
Created attachment 667869 [details] [diff] [review]
skip patch [esr10]

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-
(Assignee)

Comment 27

6 years ago
Created attachment 667895 [details] [diff] [review]
skip patch (esr10) [checked-in]

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
(Assignee)

Updated

6 years ago
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)

Updated

6 years ago
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
(Assignee)

Comment 31

6 years ago
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
(Assignee)

Comment 33

6 years ago
Hasn't failed these days. I think it's safe to close.
(Assignee)

Comment 35

6 years ago
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.

Comment 37

6 years ago
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
(Assignee)

Comment 39

6 years ago
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.
(Assignee)

Comment 44

6 years ago
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?
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 48

6 years ago
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
(Assignee)

Comment 50

6 years ago
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
Last Resolved: 6 years ago6 years ago
status-firefox-esr10: affected → wontfix
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.