778608 - Move SafeBrowsing.jsm to toolkit

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Other apps (mobile, b2g, webrt) will likely want to be using the code in SafeBrowsing.jsm. So let's move it to Toolkit. This leaves very little stuff in in browser/components/safebrowsing, so I've folded it into browser/base/content.

This is on top of the patch in bug 778606, and is not yet tested. ;-)

Comment 1

[Justin Dolske [:Dolske]]

Note to self, after this we should probably move Bugzilla's Firefox::PhishingProtection component over to Toolkit.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Oops, forgot to fix a jar.mn path, but it seems to be working fine now.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Oops again, tiny fix to makefiles.sh.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Fixed few other small mistakes.

Mostly-green in https://tbpl.mozilla.org/?tree=Try&rev=dbacbd4d62f4
Fixed the M(oth) problem: https://tbpl.mozilla.org/?tree=Try&rev=028530f8507a

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 648582 [details] [diff] [review]
Patch v.4

It really pains me to add more tests to browser/base, particularly well-grouped tests that could live somewhere else - a browser chrome run of browser-base takes way too long already. I guess I don't have a better suggestion at the moment, though :/

Comment 6

[Justin Dolske [:Dolske]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fce1836888b4

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This patch caused "Timed out while waiting for: test #6 succesful finish" failures in browser_webconsole_bug_595934_message_categories.js on win debug; exactly like in your second try push.

Comment 8

[Ed Morley [:emorley]]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5d1daa2505

Comment 9

[Justin Dolske [:Dolske]]

Exciting. I can't reproduce on a local win-debug build, nor is it at all obvious how that test could possibly be affected by this patch!

Time to debug via Try, I guess. :(

Comment 10

[Justin Dolske [:Dolske]]

The test (referenced in comment 7) is loading an invalid XML page, and watching for "no element found" to be logged in the error console via a console observer. So far it appears that the test is successfully loading the page, but the parsing goop simply isn't logging this message. So, still baffled as to what's happening.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 648582 [details] [diff] [review]
Patch v.4

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

::: toolkit/components/url-classifier/Makefile.in
@@ +20,5 @@
> +# Normally the "client ID" sent in updates is appinfo.name, but for
> +# official Firefox releases from Mozilla we use a special identifier.
> +ifdef MOZILLA_OFFICIAL
> +ifdef MOZ_PHOENIX
> +DEFINES += -DUSE_HISTORIC_SAFEBROWSING_ID=1

Could we avoid the MOZ_PHOENIX, or maybe move it to a branding prefs file? (probably as a separate bug) ... it tends to be a pain when we have app specific ifdefs in core (e.g. for xulrunner style builds etc). It'd also avoid the need for preprocessing the file.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

As pointed out here https://bugzilla.mozilla.org/show_bug.cgi?id=741808#c4 there's also issues with this define for Fennec.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch without moving tests

A while back the thought occurred to me that the bizarre test failures this triggered before were likely just test timing issues. I trimmed down the patch to leave the tests in-place, and that's passed a few try runs at different times.

I keep forgetting to land it, though, so I'll do so now. Will file followup to move tests and finish debugging (if it's still reproducible).

Comment 14

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/70d9ba6b84f8

Filed bug 806757 to investigate moving the tests.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70d9ba6b84f8