Closed Bug 901198 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Attached 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 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+
Thanks to you both for the comments:

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