683897 - Failure in /testSecurity/testSafeBrowsingWarningPages.js | could not validate element ID: urlbar with value

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

4. Include the test function, error message, date when the error occurred, date with the error first occurred, number of times the error has been reported, and the branches it occurs in the DESCRIPTION field

EXAMPLE:
TEST: /testSecurity/testSafeBrowsingWarningPages.js:testWarningPages
ERROR: could not validate element ID: urlbar with value http://www.mozilla.com/firefox/its-a-trap.html
WHEN: 2011-08-31
FIRST: 2011-08-25
FREQ: 
BRANCHES: release, 1.9.2

5. Remove these instructions

6. Click SUBMIT BUG
----- END INSTRUCTIONS -----

TEST: 
ERROR: 
WHEN: 
FIRST: 
FREQ:

Comment 1

[Remus Pop (:RemusPop)]

Attachment: proposed fix (1.9.2)

Sorry about forgetting to remove the lines from the template.

Basically the problem with this failure is that we get a redirect at a point in the test so I've come up with a workaround, hard-coding the urls.

Comment 2

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

Can you please specify where we redirect to? Means source and target URL. Thanks. assertURLEqual is able to handle that. So not sure what's wrong here.

Comment 3

[Remus Pop (:RemusPop)]

When finally visiting http://www.mozilla.com/firefox/its-a-trap.html in checkIgnoreWarningButton function, a screen appears in which we click the "ignore warning" button. After that we are redirected to http://www.mozilla.org/firefox/its-a-trap.html and when we assert it with the given parameter in checkIgnoreWarningButton we receive the error mentioned in comment 1. 

We could have used the controller.assertValue but I thought utils was more appropiate because we use it in another function.

Yes, the behavior is strange because the test in the default branch works fine and it is almost the same code; differences appear in setting preferences as far as I can remember.

Comment 4

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17795857

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 557492 [details] [diff] [review]
proposed fix (1.9.2)


> 
>+  // When accesing one of the pages we get a redirect so here is a workaround

Please XXX: this comment because it's a temporary workaround 
Also, re-word to // XXX: Workaround 
                 //      We dispose of the redirected url when accessing the page

>+  if (url == "http://www.mozilla.com/firefox/its-a-trap.html")
>+    url = "http://www.mozilla.org/firefox/its-a-trap.html";
>+  else 
>+    if (url == "http://www.mozilla.com/firefox/its-an-attack.html")
>+      url = "http://www.mozilla.org/firefox/its-an-attack.html";

Please declare url values as constants. You can you two constant to show exaclty what 
you want to do: example INITIAL_URL and REDIRECT_URL. Provide other naming if you like, 
only make sure it's readable. 

> 
> /**
>  * Map test functions to litmus tests
>  */
> // testWarningPages.meta = {litmusids : [9014]};

We don't map to the litmus tests anymore so you can delete the above code 
This applies to every test we update/fix/create

Otherwise looks good. Good catch with the redirected url

Comment 6

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

(In reply to Remus Pop from comment #3)
> When finally visiting http://www.mozilla.com/firefox/its-a-trap.html in
> checkIgnoreWarningButton function, a screen appears in which we click the
> "ignore warning" button. After that we are redirected to
> http://www.mozilla.org/firefox/its-a-trap.html and when we assert it with
> the given parameter in checkIgnoreWarningButton we receive the error
> mentioned in comment 1. 

When we are clicking an element we are not getting redirected. It's an expected page load in this case. So please add some sleep calls for debugging purposes and exactly watch what this test is doing and why we are failing. Also which line is it exactly? Given the error I don't think it's the assertURLEqual call in this case.

Comment 7

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

Somehow the merge from beta to release wasn't complete. I missed to change a single line which is now responsible for this failure. Patch upcoming.

The upcoming patch will fix both the warning pages and notification bar test, but not the issue reported on bug 681920.

Comment 8

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

Attachment: Patch (mozmill-release)

Fixes the issue on mozilla-release. I will check for the backport later.

Comment 9

[u279076]

Attachment: Patch v2 (mozilla-release) [checked-in]

Patch was missing commit message -- here is an updated patch which has been checked in.

Comment 10

[u279076]

Comment on attachment 558952 [details] [diff] [review]
Patch v2 (mozilla-release) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3a73dc57e7dc (mozilla-release)

Comment 11

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

Attachment: Backport (1.9.2) v1 [checked-in]

Ok, so the permanent redirect from .com to .org is hitting us here. When you click the ignore button we will be redirected to the .org domain and the notification bar closes immediately. The ignore permission doesn't get saved that way and reopening the fraudulent page shows us again the page with the ignore button. So lets directly check for the .org page to be loaded.

Does the safebrowsing notification bar test also fail? If yes, it would need the same fix.

Comment 12

[u279076]

Comment on attachment 558977 [details] [diff] [review]
Backport (1.9.2) v1 [checked-in]

>   // Verify the warning button is not visible and the location bar displays the correct url
>-  var locationBar = new elementslib.ID(controller.window.document, "urlbar");
>-
>-  controller.assertValue(locationBar, url);
>+  // Since we permanently redirect all .com addresses to .org we have to directly check
>+  // for the .org URL. Otherwise we are stuck on the fraudulent page. This is only necessary
>+  // for Firefox 5.x and lower because those builds don't set the permissions for the page.
>+  utils.assertLoadedUrlEqual(controller, url.replace(/\.com/, ".org"));
>   controller.assertNodeNotExist(ignoreWarningButton);
>   controller.assertNode(new elementslib.ID(controller.tabs.activeTab, "main-feature"));
> }

My concern here is that your '// Since' comment might bleed into the '// Verify' comment above. I would suggest adding 'XXX: ' to the beginning of your comment. I won't block check-in on this however.

Comment 13

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

Comment on attachment 558977 [details] [diff] [review]
Backport (1.9.2) v1 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/364f16f1485f

Comment 14

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

Marking as resolved fixed. This test shouldn't fail now anymore across branches.

Comment 15

[u279076]

No failure instances found in recent reports; VERIFIED FIXED