Closed Bug 905033 Opened 11 years ago Closed 11 years ago

Test failure "not-a-web-forgery report page is loaded" in /testSafeBrowsingNotificationBar.js

Categories

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

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Platform:
x86
All
Type:
defect

Tracking

(firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox-esr17 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox-esr17 --- fixed

People

(Reporter: cosmin-malutan, Assigned: mario.garbi)

References

(
URL
)

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(7 files, 7 obsolete files)

skipSecurity.patch
1.84 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
skipRelease.patch
2.09 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
skipESR17.patch
2.08 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
phishingUrlUpdate_2808.patch
8.46 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
phishingUrlUpdate_Beta.patch
8.48 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
phishingUrlUpdate_ESR17.patch
10.30 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
manifestUnskip.patch
1.02 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review 
This happens across all locales that we tested and it's due to a change in how Firefox 26 handles that particular page. More specifically when we ignore the warning we are redirected towars a page that includes the locale in the URL.
This seems to have started happening since yesterday runs and only on 26. It might be a new feature of firefox and not a regression.

I have a patch that fixes that, only needs a few more polishes.
Please don't forget to add tracking flags and whiteboard entries when submitting a new bug.
Assignee: nobody → mario.garbi
Status: NEW → ASSIGNED
status-firefox22: --- → unaffected
status-firefox23: --- → unaffected
status-firefox24: --- → unaffected
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox-esr17: --- → unaffected
OS: Windows 7 → All
Priority: -- → P1
Whiteboard: [mozmill-test-failure]
Mario, please also find the responsible changeset
Skip patch for this until I can finish the fix patch. Initially the patch handled the new locale element in the URL but it seems we had problems with zh-Tw and we might hit issues with other exotic locales as well.
Since this is related to the remote mozilla webpage could we consider changing the check from the url to another page element or perhaps even using "contains" to check only a part of the url.
Attachment #790120 - Flags: review?(hskupin)
Attachment #790120 - Flags: review?(dave.hunt)
Comment on attachment 790120 [details] [diff] [review]
skipSecurity.patch

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

Looks good. But lets get this investigated and fixed today.
Attachment #790120 - Flags: review?(hskupin)
Attachment #790120 - Flags: review?(dave.hunt)
Attachment #790120 - Flags: review+
Comment on attachment 790120 [details] [diff] [review]
skipSecurity.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/1b5b57b4d1ca
Attachment #790120 - Flags: checkin+
status-firefox26: affected → disabled
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
This affects all branches and not only Nightly! So I have backported the skip patch:

http://hg.mozilla.org/qa/mozmill-tests/rev/8b7e0ae2d8d8 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/14523938c75c (beta)

Mario, I need a new patch for release and most likely esr17 too.
status-firefox22: unaffected → affected
status-firefox23: unaffected → affected
status-firefox24: unaffected → disabled
status-firefox25: unaffected → disabled
status-firefox-esr17: unaffected → affected
Skip patch for Release branch
Attachment #790177 - Flags: review?(hskupin)
Attached patch skipESR17.patch — — Splinter Review 
Skip patch for ESR17 branch
Attachment #790178 - Flags: review?(hskupin)
Comment on attachment 790177 [details] [diff] [review]
skipRelease.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/4ab19fc63240 (release)
Attachment #790177 - Flags: review?(hskupin)
Attachment #790177 - Flags: review+
Attachment #790177 - Flags: checkin+
Comment on attachment 790178 [details] [diff] [review]
skipESR17.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/c9fdb8af06fc (esr17)
Attachment #790178 - Flags: review?(hskupin)
Attachment #790178 - Flags: review+
Attachment #790178 - Flags: checkin+
As a hint, next time please also include 'Skip' in the commit message. I totally missed that in the review.
status-firefox22: affected → ---
status-firefox23: affected → disabled
status-firefox-esr17: affected → disabled
Attached patch phishingUrlUpdate.patch (obsolete) — — Splinter Review 
This fixes the problem on some locales (zh-TW) but will still fail on others (ja). The tab change is due to how assertEqualLoadedUrl works since we used to get the url value for tab 0 without actually changing to tab 0.
This is a work in progress and I will come with a better version soon, I just wanted to ask your opinion on the direction I'm going with this.
Thanks.
Attachment #790245 - Flags: feedback?(hskupin)
(In reply to mario garbi from comment #13)
> Created attachment 790245 [details] [diff] [review]
> phishingUrlUpdate.patch
> 
> This fixes the problem on some locales (zh-TW) but will still fail on others
> (ja). The tab change is due to how assertEqualLoadedUrl works since we used
> to get the url value for tab 0 without actually changing to tab 0.

Can you please explain when we open a new tab? I don't see that in the test. Clicking on the ignore link will open the target page in the same tab. So why do we have to change to the first tab?
Comment on attachment 790245 [details] [diff] [review]
phishingUrlUpdate.patch

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

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +95,5 @@
>      tabBrowser.waitForTabPanel(tabBrowser.selectedIndex, '/{"value":"blocked-badware-page"}');
>      controller.waitThenClick(button);
>      controller.waitForPageLoad(controller.tabs.getTab(1));
> +    tabBrowser.selectedIndex = 0;
> +    utils.assertLoadedUrlEqual(controller, badUrl);

You want to compare the loaded URL with the page given in the preference we talked about earlier.

Also why do we have to switch tabs? Can't we do the check before clicking the button? Would that hide the notification bar?
Attachment #790245 - Flags: feedback?(hskupin) → feedback-
Damn. My fault, sorry. I missed to run the get_mozmill-tests job in mozmill-ci after landing the other two skip patches on release and esr17. Please ignore those failures.
The preference we found has the url "https://www.mozilla.org/%LOCALE%/firefox/phishing-protection/" and refers to the action triggered by the button "Why was this page blocked" which we don't click in our test (we click "ignore warning" and then the notification bar).

Could we handle this with a locale Map since that seems the most reliable way to handle the locales. The es-ES and ja-JP locales don't work with utils.assertLoadedUrlEqual but others that I've checked work fine.
No, I don't think we want that. Maps are really only the last resort! Instead run a network tracing tool or the HTTP log while doing the run in Firefox so you can catch the initial page we are trying to load for those specific locales.
Attached patch phishingUrlUpdate_1608.patch (obsolete) — — Splinter Review 
We cleaned the code a bit and check that the report page has been opened in both cases, as I think we should.

I have tested this with a couple of exotic locales (ja-JP, de, ro, zh-TW, es-ES, en-US) and we are green with no failures. If these changes are ok I will provide full testrun reports for all platforms so we might fix the test failures.

In the test we click the "this is not an attack" report button and for TEST_DATA[1] we check that the report page has been opened, for TEST_DATA[0] we checked only that the current URL is the same as the initial URL and not that we opened the correct link in tab 2. Here is also where the changes in the URL caused the failures this week.

Thanks.
Attachment #791262 - Flags: review?(hskupin)
Attached patch phishingUrlUpdate_1608.patch (obsolete) — — Splinter Review 
Noticed a couple of things I overlooked in the first patch and this should take care of them.
Thanks.
Attachment #790245 - Attachment is obsolete: true
Attachment #791262 - Attachment is obsolete: true
Attachment #791262 - Flags: review?(hskupin)
Attachment #791269 - Flags: review?(hskupin)
Attachment #791269 - Flags: review?(andreea.matei)
Comment on attachment 791269 [details] [diff] [review]
phishingUrlUpdate_1608.patch

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

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +11,5 @@
>  
>  const TEST_DATA = [
> +  // Phishing url object
> +  {
> +    label : "safebrowsing.notAForgeryButton.label",

What kind of label? Please give it a better name. Maybe we should move the whole getProperty() call up here?

@@ +13,5 @@
> +  // Phishing url object
> +  {
> +    label : "safebrowsing.notAForgeryButton.label",
> +    reportPage : "www.google.com/safebrowsing/report_error",
> +    url : "http://www.mozilla.org/firefox/its-a-trap.html"

If we make use of reportPage we should also give url a better name.

@@ +17,5 @@
> +    url : "http://www.mozilla.org/firefox/its-a-trap.html"
> +  },
> +  // Malware url object
> +  {
> +    label : "safebrowsing.notAnAttackButton.label",

Same here as above.

@@ +19,5 @@
> +  // Malware url object
> +  {
> +    label : "safebrowsing.notAnAttackButton.label",
> +    reportPage : 'www.stopbadware.org',
> +    url : "http://www.mozilla.org/firefox/its-an-attack.html"

Same here.

@@ +69,5 @@
>   *
> + * @param aData {Object} Object containing elements of testing page
> + *                       label - string of the element label
> + *                       url - URL string of the harmful page
> + *                       reportPage - URL string of the report page

Please see other modules how to define sub properties for jsdoc comments. Those will get their own @param line.

@@ +111,2 @@
>  
> +  // Verify the not-an-attack-site report page is loaded

Keep in mind that we also cover phishing now. So the comment has to be adjusted.

@@ +115,2 @@
>  
> +  expect.contain(currentURL, aData.reportPage, "Loaded URL is the report page");

So what's with the failures in testing the ignore page like:
http://www.mozilla.org/de/firefox/its-a-trap.html

I can't see that something like that happens here now. Why has this been completely removed? As of the language, it's not the locale of Firefox but the selected content locale. So we could force to en-US if we want.
Attachment #791269 - Flags: review?(hskupin)
Attachment #791269 - Flags: review?(andreea.matei)
Attachment #791269 - Flags: review-
The ignore warning page is being checked in the "checkIgnoreWarningButton" function at line 91. We checked it again in the "checkNoPhishingButton" after removing the permision with "utils.removePermission" and therefore blocking the redirect. This was the part I removed in the patch and replaced with a check that verifies that we open the correct report page when we click the notification bar button.
Comment on attachment 792710 [details] [diff] [review]
phishingUrlUpdate_2008.patch

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

So if we blocked the real reporting page from being loaded, then it was a good catch. Thanks Mario. Once the last remaining comments have been fixed, I will test it myself.

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +9,5 @@
>  var tabs = require("../../../lib/tabs");
>  var utils = require("../../../lib/utils");
>  
>  const TEST_DATA = [
> +  // Phishing url object

s/url/URL/ please.

@@ +12,5 @@
>  const TEST_DATA = [
> +  // Phishing url object
> +  {
> +    buttonLabel : utils.getProperty("chrome://browser/locale/browser.properties",
> +                                    "safebrowsing.notAForgeryButton.label"),

This was a question in my last review. I'm not sure if a complete move is that helpful given that we most likely exceed the max line length. Given that both properties are using the same file, we could leave it as it was before.

@@ +13,5 @@
> +  // Phishing url object
> +  {
> +    buttonLabel : utils.getProperty("chrome://browser/locale/browser.properties",
> +                                    "safebrowsing.notAForgeryButton.label"),
> +    reportPage : "www.google.com/safebrowsing/report_error",

Why without the http protocol?

@@ +71,5 @@
>   *
> + * @param {object} aData
> + *        Object containing elements of testing page
> + * @param {String} aData.buttonLabel
> + *        Report button label string

Remove 'string' which is superfluous here. Same for all the other lines.

@@ +101,5 @@
>   *
> + * @param {object} aData
> + *        Object containing elements of testing page
> + * @param {String} aData.buttonLabel
> + *        Report button label string

Same here as above.
Attachment #792710 - Flags: review-
Attached patch phishingUrlUpdate_2108.patch (obsolete) — — Splinter Review 
Made the requested changes and moved the utils.getProperty code block out of the Object and back into the test.
For the reportPage element we cannot use the http protocol in the string since the page that we gather doesn't have that protocol in the title. For google we get 'www.google.com/safebrowsing/report_error/?tpl=mozilla&hl=en-US&url=http%3A%2F%2Fwww.mozilla.org%2Ffirefox%2Fits-a-trap.html' and we check if the domain and correct subdomain are present.
Attachment #792710 - Attachment is obsolete: true
Attachment #793430 - Flags: review?(andreea.matei)
Comment on attachment 793430 [details] [diff] [review]
phishingUrlUpdate_2108.patch

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

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +119,3 @@
>  
> +  // Verify that the correct not a forgery or attack report page is loaded
> +  var locationBar = new elementslib.ID(controller.window.document, "urlbar");

While we are modifying that, can you please make use of the locationBar class?

@@ +123,2 @@
>  
> +  expect.contain(currentURL, aData.reportPage, "Loaded URL is the report page");

Looks like we can file a follow-up bug to get the locationbar.contains() method removed which is not necessary anymore, right?
Attached patch phishingUrlUpdate_2208.patch (obsolete) — — Splinter Review 
Thank you Henrik for pointing that out, we can indeed use locationBar in those two cases and it is more elegant that way. If this looks ok I will bring full reports on all platforms.

Sample report with Nightly 26 de on Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ed8b74f
Attachment #793430 - Attachment is obsolete: true
Attachment #793430 - Flags: review?(andreea.matei)
Attachment #793968 - Flags: review?(andreea.matei)
Hi Mario,
I have created patch for bug 816084. I have not modified testSafeBrowsingNotificationBar.js as tests are skipping for it.

Please take care of following things when you create patch for this bug:
(1)
testSafeBrowsingNotificationBar.js file should include 

   var locationBar = locationBar.getElement({type:"urlbar"}); instead of

Line 72: and Line 117:     
   var locationBar = new elementslib.ID(controller.window.document, "urlbar");

It is already available in library toolbar.js 

(2)
As we are using library, testSafeBrowsingNotificationBar.js file should include 
   var toolbars = require("../../../lib/toolbars");
   aModule.locationBar = new toolbars.locationBar(aModule.controller);

So, total 4 lines, 2 new lines should be added and 2 lines should be modified.
Comment on attachment 793968 [details] [diff] [review]
phishingUrlUpdate_2208.patch

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

Resetting review based on the given addition to make.
Attachment #793968 - Flags: review?(andreea.matei)
Hello,

>    var locationBar = locationBar.getElement({type:"urlbar"}); instead of
> 
> Line 72: and Line 117:     
>    var locationBar = new elementslib.ID(controller.window.document,
> "urlbar");
> 
> It is already available in library toolbar.js 
> 
We stopped using locationBar to get the currentURL in the latest patch and used locationBar.contains() method of the library we imported.

> (2)
> As we are using library, testSafeBrowsingNotificationBar.js file should
> include 
>    var toolbars = require("../../../lib/toolbars");
>    aModule.locationBar = new toolbars.locationBar(aModule.controller);

This is already present in the patch, what exactly is wrong here? If there is something I missed please let me know and I will update the patch so we can get this issue fixed.
If that is the case, please re-request for review Mario. Sorry for the noise!
Comment on attachment 793968 [details] [diff] [review]
phishingUrlUpdate_2208.patch

Thank you Henrik, setting review request.
Attachment #793968 - Flags: review?(hskupin)
Attachment #793968 - Flags: review?(andreea.matei)
Comment on attachment 793968 [details] [diff] [review]
phishingUrlUpdate_2208.patch

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

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +38,5 @@
>    utils.removePermission("www.mozilla.org", "safe-browsing");
>  }
>  
>  var testNotificationBar = function() {
>    TEST_DATA.forEach(function(data) {

Please rename the parameter to aData.

@@ +111,5 @@
> +  // Click on the web forgery report button
> +  var label = utils.getProperty("chrome://browser/locale/browser.properties", aData.buttonLabel);
> +  var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
> +                                             '/{"value":"blocked-badware-page"}/{"label":"' +
> +                                             label + '"}');

Lets move on the next line everything related to label, after "/". It will be easier to read.
Attachment #793968 - Flags: review?(hskupin)
Attachment #793968 - Flags: review?(andreea.matei)
Attachment #793968 - Flags: review-
Attached patch phishingUrlUpdate_2508.patch (obsolete) — — Splinter Review 
Made the required changes and checked that the patch still applies correctly.
Attachment #793968 - Attachment is obsolete: true
Attachment #795330 - Flags: review?(andrei.eftimie)
Attachment #795330 - Flags: review?(andreea.matei)
Comment on attachment 795330 [details] [diff] [review]
phishingUrlUpdate_2508.patch

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

Patch works fine.
r+ from me with the following nit fixed

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +89,1 @@
>  

nit: remove this empty line
Attachment #795330 - Flags: review?(andrei.eftimie)
Attachment #795330 - Flags: review?(andreea.matei)
Attachment #795330 - Flags: review+
Removed the extra blank line as requested
Attachment #795330 - Attachment is obsolete: true
Attachment #796610 - Flags: review?(andrei.eftimie)
Attachment #796610 - Flags: review?(andreea.matei)
Mario, it looks good to me now. Please add reports for each platform. Thanks!
Comment on attachment 796610 [details] [diff] [review]
phishingUrlUpdate_2808.patch

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

Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/2fe31b89d381 (default)

Mario, please check if it applies cleanly on the other branches.
Attachment #796610 - Flags: review?(andrei.eftimie)
Attachment #796610 - Flags: review?(andreea.matei)
Attachment #796610 - Flags: review+
Attachment #796610 - Flags: checkin+
Comment on attachment 802322 [details] [diff] [review]
phishingUrlUpdate_Beta.patch

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

Landed:

http://hg.mozilla.org/qa/mozmill-tests/rev/454f9f3ea54b (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2b4ca2685e43 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/12cf4890e145 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/beb12645bef7 (mozilla-esr24)

We still need to backport this to ESR17
Attachment #802322 - Flags: review?(andrei.eftimie)
Attachment #802322 - Flags: review?(andreea.matei)
Attachment #802322 - Flags: review+
Attachment #802322 - Flags: checkin+
(In reply to mario garbi from comment #45)
> We don't run l10n builds with ESR17 and this only fails with those. ESR17

Huh, really? That shouldn't be the case. Why that hasn't been detected yet? Lets get an issue filed on github for mozmill-ci.
(In reply to Henrik Skupin (:whimboo) from comment #46)
> (In reply to mario garbi from comment #45)
> > We don't run l10n builds with ESR17 and this only fails with those. ESR17
> 
> Huh, really? That shouldn't be the case. Why that hasn't been detected yet?
> Lets get an issue filed on github for mozmill-ci.

Actually that should have been covered in the weekly report you are doing. As it has been turned out yesterday this was a build problem on RelEng side, and we could have been reported that way earlier. So please make sure to check all branches for l10n results in the future.
Oh, it will be fixed in bug 915356.
(In reply to Henrik Skupin (:whimboo) from comment #47)
> Actually that should have been covered in the weekly report you are doing.

We don't check localised ESR17 builds, because we *never* run them.

Here is all of 2013:
http://mozmill-ci.blargon7.com/#/functional/reports?branch=17.0&platform=All&from=2013-01-01&to=2013-09-12
http://mozmill-daily.blargon7.com/#/functional/reports?branch=17.0&platform=All&from=2013-07-01&to=2013-09-12

I've checked all our weekly reports and we haven't spotted 1 single ESR localised run. (I guess that's why we've took them out of the report 4-5 weeks ago)

I've looked now over the Jenkins config files, and I'm not sure.
It does appear that we *should* run on localised ESR17 builds. But we never actually do.
I'm not sure I'm reading the config files correctly.
Right, and that's why the alarm bells should have rang. But anyway it's too late for it now and the l10n bustage is fixed. In the future just question yourself why some tasks are not getting performed and don't remove checks silently without further discussion.
So please get this test backported to esr17. Thanks.
Comment on attachment 803616 [details] [diff] [review]
phishingUrlUpdate_ESR17.patch

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

I see we had to port the whole test over since it hasn't been done earlier.
Good work.

Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/a4d2bdda6e7a (mozilla-esr17)
Attachment #803616 - Flags: review?(andrei.eftimie)
Attachment #803616 - Flags: review?(andreea.matei)
Attachment #803616 - Flags: review+
Attachment #803616 - Flags: checkin+
With this we can close this issue.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox-esr17: disabled → fixed
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure]
It seems I overlooked the manifest skip with the previous patches, here is a fix for that. It applies cleanly on default to esr24 branches.
Attachment #8335870 - Flags: review?(andrei.eftimie)
Attachment #8335870 - Flags: review?(andreea.matei)
Attachment #8335870 - Flags: review?(andrei.eftimie)
Attachment #8335870 - Flags: review?(andreea.matei)
Attachment #8335870 - Flags: review+
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: