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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: skip patch [backed-out]

We should disable this test for now and concentrate on the investigation tomorrow.

Comment 2

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

Replacing bug 802126 in our sprint with that one which is more important.

Comment 3

[u279076]

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

Looks good to me, please land.

Comment 4

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

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

http://hg.mozilla.org/qa/mozmill-tests/rev/8b5cec6391fb (default)

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

I have a proposed fix for this issue which is currently in internals.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Also, forgot to mention it passed the internal reviews

Comment 8

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

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.

Comment 9

[Andreea Matei [:AndreeaMatei]]

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?

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 11

[Andreea Matei [:AndreeaMatei]]

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.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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!

Comment 15

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

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.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 17

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

Right, once a new patch on bug 782918 fixed that we can unskip this test again.

Comment 18

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

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

That patch is not something we want to continue with.

Comment 19

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

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)

Comment 20

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

Looks ok now. No more test failures across platforms.