test_app_rep.js fails for applications which have safe browsing disabled (aka Permanent orange | TEST-UNEXPECTED-FAIL | false = true)

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({intermittent-failure})

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25 fixed, firefox26 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Posted patch The fixSplinter Review
Since bug 837199 landed (but only just detected due to other issues), on Thunderbird, we've been getting failures in test_app_rep.js due to safe browsing being disabled there:

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js | false == true - See following stack:
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js :: onComplete :: line 68
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js :: test_shouldBlock :: line 67

The fix is simple - set the preferences at the start of the test to ensure that safe browsing (the element under test) is enabled.
Attachment #785350 - Flags: review?(paolo.mozmail)
Comment on attachment 785350 [details] [diff] [review]
The fix

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

Thanks for catching this and I'm sorry I missed it the first time.

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +21,5 @@
>                               "http://localhost:4444/download");
> +  // Ensure safebrowsing is enabled for this test, even if the app
> +  // doesn't have it enabled.
> +  Services.prefs.setBoolPref("browser.safebrowsing.enabled", true);
> +  Services.prefs.setBoolPref("browser.safebrowsing.malware.enabled", true);

You only need to set this second one. The first one (browser.safebrowsing.enabled) is used for phishing checks only.
Attachment #785350 - Flags: review+

Comment 2

6 years ago
Comment on attachment 785350 [details] [diff] [review]
The fix

What Monica said :-)

Plus, it seems worth calling Services.pref.clearUserPref on that particular
preference, inside a do_register_cleanup function. Feel free to land a patch
including this change.
Attachment #785350 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 3

6 years ago
Thanks to you both for the comments:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab55dbf6ee9
Target Milestone: --- → mozilla25
(Assignee)

Updated

6 years ago
Target Milestone: mozilla25 → mozilla26
https://hg.mozilla.org/mozilla-central/rev/eab55dbf6ee9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.