Test failure 'Lock icon is visible in identity box - '' should equal 'false' ' in /testSecurity/testDVCertificate.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: whimboo, Assigned: AndreeaMatei)

Tracking

unspecified

Firefox Tracking Flags

(firefox18 unaffected, firefox19 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Not sure why this failure hasn't been filed yet, but it's affecting all of our tests on Nightly across platforms. This is most likely a change in Firefox which means that we have to update the test.

Lock icon is visible in identity box - '' should equal 'false' 

If we can't find the fix we should skip the test for now.
(Reporter)

Comment 1

6 years ago
Created attachment 674034 [details] [diff] [review]
skip patch [backed-out]

We should disable this test for now and concentrate on the investigation tomorrow.
Attachment #674034 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 2

6 years ago
Replacing bug 802126 in our sprint with that one which is more important.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=121022 u=failure c=security p=1
Comment on attachment 674034 [details] [diff] [review]
skip patch [backed-out]

Looks good to me, please land.
Attachment #674034 - Flags: review?(anthony.s.hughes) → review+
(Reporter)

Comment 4

6 years ago
Comment on attachment 674034 [details] [diff] [review]
skip patch [backed-out]

http://hg.mozilla.org/qa/mozmill-tests/rev/8b5cec6391fb (default)
Attachment #674034 - Attachment description: skip patch → skip patch [checked-in]
(Reporter)

Updated

6 years ago
Whiteboard: [mozmill-test-failure] s=121022 u=failure c=security p=1 → [mozmill-test-failure][mozmill-test-skipped] s=121022 u=failure c=security p=1
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
I have a proposed fix for this issue which is currently in internals.
Created attachment 674215 [details] [diff] [review]
proposed fix v1.0

* proposed fix
* looked in inspector and seen that 'hidden' is a Javascript property so we are using element.getNode().propertyname to fix this
Attachment #674215 - Flags: review?(hskupin)
Attachment #674215 - Flags: review?(dave.hunt)
Also, forgot to mention it passed the internal reviews
(Reporter)

Comment 8

6 years ago
Comment on attachment 674215 [details] [diff] [review]
proposed fix v1.0

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #6)

> * looked in inspector and seen that 'hidden' is a Javascript property so we
> are using element.getNode().propertyname to fix this

Why is it not a DOM attribute anymore? What has been changed in Firefox? Please give us the bug number for it, which shouldn't be too hard to figure out. As long as I don't see that it has been changed on purpose we should not review this patch.
Attachment #674215 - Flags: review?(hskupin)
Attachment #674215 - Flags: review?(dave.hunt)
(Assignee)

Comment 9

6 years ago
I was looking at this failure, ran the test over builds before Oct 19th (where the reports started) and it reproduces. The weird thing is we have no reports failing before.

Indeed if we change with favicon.getNode().hidden, the test works.

So I've checked the pushlogs and found this:
https://bugzilla.mozilla.org/show_bug.cgi?id=773780
that was fixed on 19th, could this be the cause?
(Assignee)

Updated

6 years ago
Assignee: vlad.mozbugs → andreea.matei
So looking at the pushlog between the last success and first failure I can see a likely candidate. The pushlog is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0ff60bfb3442&tochange=ff4af83233dc

From this, I suspect "Bug 772808 - menuitems don't inherit the "hidden" attribute from commands"

Andreea: Could you run tests again against a build from before the 19th? I wouldn't expect it to pass. Make sure you don't have the attached patch applied.
(Assignee)

Comment 11

6 years ago
Dave, indeed it's not passing. That is what looked strange to me while investigating, the test failed also with builds before 19th, though we had no reports in mozmill-ci for that. So I figured the change from 19th must have been applied to all.
(Assignee)

Updated

6 years ago
Attachment #674215 - Flags: review?(hskupin)
Attachment #674215 - Flags: review?(dave.hunt)
I'm not comfortable making this change until we've identified the cause. I've comfirmed that this test is failing when run on a build prior to October 19th, but this does not align with the Mozmill CI results. I will trigger a build on our staging CI.
Unfortunately triggering a build on the CI won't help because we've skipped this test. I did some more digging to see what changed on or around the 19th October and finally found bug 787252, which restored the certificate. At first I thought the dates matched but then noticed that the certificate was restored exactly one month before the failures started to occur.
Looking into this further, the assertion that's failing claims to be checking that the lock icon is visible, however all it's doing is verifying that the favicon is visible. This is true even when the icon is not the padlock.

Using DOM Inspector, I can see that we could enhance this test to check for pageproxystate="valid" on this node. I still have no idea why .getAttribute('hidden') was working before, but now only .hidden will work, but checking that something is not hidden that will never actually be hidden seems a bit like a false positive to me.

Henrik: Could you give your thoughts on this? Thanks!
(Reporter)

Comment 15

6 years ago
Sounds fine to me. But it should still be a regression in Firefox. Andreea please use tinderbox builds to nail down the regression range. I haven't seen that yet and it's our usual task to do in such a situation.
status-firefox18: --- → unaffected
status-firefox19: --- → affected
So to provide an update: It would seem that bug 782918 caused a regression here.

Specifically:
-  controller.assert(function () {
-    return favicon.getNode().getAttribute("hidden") == false;
-  }, "Lock icon is visible in identity box");
+  expect.equal(favicon.getNode().getAttribute("hidden"), "false",
+               "Lock icon is visible in identity box");

Instead of checking that getAttribute returned false, we're now checking that it returns "false". I will comment on bug 782918 to get a follow-up patch.
Depends on: 782918
(Reporter)

Comment 17

6 years ago
Right, once a new patch on bug 782918 fixed that we can unskip this test again.
(Reporter)

Comment 18

6 years ago
Comment on attachment 674215 [details] [diff] [review]
proposed fix v1.0

That patch is not something we want to continue with.
Attachment #674215 - Attachment is obsolete: true
Attachment #674215 - Flags: review?(hskupin)
Attachment #674215 - Flags: review?(dave.hunt)
(Reporter)

Comment 19

6 years ago
Comment on attachment 674034 [details] [diff] [review]
skip patch [backed-out]

Backed-out the skip patch:
http://hg.mozilla.org/qa/mozmill-tests/rev/8bc615d2739e (default)
Attachment #674034 - Attachment description: skip patch [checked-in] → skip patch [backed-out]
Attachment #674034 - Flags: checkin+
(Reporter)

Comment 20

6 years ago
Looks ok now. No more test failures across platforms.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=121022 u=failure c=security p=1 → [mozmill-test-failure] s=121022 u=failure c=security p=1
You need to log in before you can comment on or make changes to this bug.