358299 - Built-in phishing detection

Status: RESOLVED FIXED

(Camino Graveyard :: Security, enhancement)

Description

[Christopher Moeller]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/418.9 (KHTML, like Gecko) Safari/419.3
Build Identifier: Camino 1.0a1

I really like using Camino and look forward to future versions and builds to see different features that it will have.  I noticed that Safari 3 will have built-in phishing protection when Mac OS X 10.5 comes out and would also like to see that as a feature in Camino.  I know Apple is using Google but I am not sure what would be best to use.

Thanks for a great browser!

Reproducible: Always

Comment 1

[Sean Murphy]

As per the 2008-03-05 status meeting, I'm going to take a look at this for 2.0 using Google's Safe Browsing API.

Comment 2

[Chris Lawson (gone)]

Sean, just FYI, 1Password recently introduced this feature, and they're using the PhishTank API. I suppose it's something we might want to consider as a possible alternative or supplement to the Google Safe Browsing API.

http://www.phishtank.com/api_documentation.php

Comment 3

[Sean Murphy]

Thanks for the tip, cl.

I should give a little status update here, with what we recently discussed in past meetings.  Our initial approach was to directly implement our own interaction with Google's API.  The other choice for using Google centered around the fact that Firefox's data model implementation for the API is actually available in Core's url-classifier toolkit component.  The public version of the Google API protocol at the time was fairly straightforward though, and among other reasoning, rolling our own version actually did outweigh the complications of building and linking against the newer mozilla/toolkit libraries.

To coincide with the release of Firefox 3, Google drastically modified the API protocol, to the point where it was much more complex for client interaction.  Furthermore, Google was unable to grant us an API key for either protocol version.  This prompted me to reconsider using the Core url-classifier toolkit component.  I have been able to export and link against the url-classifier, in what I hope will be an acceptable process.  So, I'm now working on some controller classes to drive the component, the about:blocked error page, and other items.

I actually did strongly consider using PhishTank, as their API is very simple and involves only a few server requests to query a url, as opposed to the maintenance of a local blacklist database necessary with Google.  Plus, Opera does utilize their data, which meant that the API/server could handle the load.  For now though, we're gonna try the Core approach and keep PhishTank as a backup option.  

Comment 4

[Samuel Sidler (old account; do not CC)]

(In reply to comment #3)
> I actually did strongly consider using PhishTank, as their API is very simple
> and involves only a few server requests to query a url, as opposed to the
> maintenance of a local blacklist database necessary with Google.  Plus, Opera
> does utilize their data, which meant that the API/server could handle the load.
> For now though, we're gonna try the Core approach and keep PhishTank as a
> backup option.

Just for more background: While there's certainly nothing inherently "wrong" with PhishTank, we'd prefer to use something that clearly works and has a much larger user base. Firefox has more users than Opera (by far) and using the backend they use makes the most sense. By using Google – and specifically by using url-classifier from toolkit – we're on a highly supported, well maintained base. For now, this seems like the best option.

(As a slight aside, OpenDNS, which runs PhishTank, has done some things that are viewed as questionable. While that's not necessarily pertinent to this discussion, it's something we might want to consider in the future should we decide to move from Google's Safe Browsing to PhishTank.)

Comment 5

[Sean Murphy]

Attachment: Draft, v1

Just wanted to get a work-in-progress patch up for now...

Implemented in this draft patch is a SafeBrowsingService class (which is a wrapper around Core's url-classifier toolkit component), an nsIAboutModule specifically for Camino which appears in the content area when pages are blocked (a Core behavior), and some event listening support throughout the embedding classes to respond to XUL button click events from the about:safebrowsingblocked page.

The changes required to actually build the url-classifier component along with camino are not included, I'm still tweaking that process a bit.

The code is not intended for review, so don't worry too much about the specifics just yet; there are at least a few areas I'll go back and modify (being a perfectionist, I'll examine each and every line again!).

Comment 7

[Sean Murphy]

Attachment: Patch, v1

This patch utilizes the Mozilla toolkit url-classifier component as the safe browsing backend.  Rather than attempting to build and link the entire toolkit component library with Camino, I decided to create a module up in our tree which selectively builds a component library for the url-classifier only.  Thus, there is only one Makefile.in line outside of the mozilla/camino that needed to be touched, which causes the toolkit/components/url-classifier sources to be built into an intermediary static library that is linked by the camino safe browsing component.  I felt this was the cleanest approach we could take at this point, since no other toolkit dependencies were necessary.

The patch includes a SafeBrowsingDatabaseService class which mostly wraps the url-classifier nsIUrlListManager service and gives our application control over the maintenance and update process of the local safe browsing database.

The url-classifier safe browsing component works by keeping a local database, urlclassifier3.sqlite in the profile directory, of blacklisted web sites.  If the url-classifier service is available, this local database (only) is then consulted by the doc shell during page loads.  The component is designed to be able to pull incremental safe browsing data at regular intervals from a data provider server implementing Google's Safe Browsing Protocol v2 (http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec).  Communication with the data provider is only for populating the local client side database, on-demand lookups are not queried against the data provider's server.

When a visited location is known to be blacklisted, the behavior of the url-classifier component is to display an "about:" type error page.  I designed a custom nsIAboutModule for camino, about:safebrowsingblocked, which implements this overlay.  Currently, the page and localizable string entities are stored in embed-replacements/locale/en-US/global/safebrowsing and available via a chrome url.  Style information was added to netError.css, since the layouts are very similar.

For testing, a raw list of blacklisted URLs can be obtained from Google:
http://sb.google.com/safebrowsing/update?client=firefox-testing&version=goog-black-url:1:-1.  The local database needs to be running for a bit before it is fully populated.  Bug 402469 is tracking that process.

Comment 8

[Sean Murphy]

Google seems to be having server trouble again, especially with their "gethash" URL.  This happened before earlier in the month, which Dave Camp told me "So, they were having significant problems with their gethash servers, especially in some geographic regions (as far as I know, it mostly only worked in the pacific northwest :))."

I'll keep an eye on this problem.  I just wanted to mention the issue because of the unfortunate timing of me just submitting this patch - it will of course prevent the local database from populating.

Comment 9

[Sean Murphy]

Attachment: about:safebrowsingblocked error overlay screenshot.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We also talked in the channel the need to get SiteIconProvider to use a different (new) site icon for the blocked pages instead of reusing the (safe) warning icon used by other error pages, and about possibly adding a warning bar to the top of the content area while browsing a site after deciding to ignore the warning/override the blocked.

Comment 11

[Samuel Sidler (old account; do not CC)]

Sean, I thought there were two potential pages: one for "malware" and one for "phishing". I noticed you only added one in your patch. Is there a second to be added later or did we combine them or... ?

Comment 12

[Sean Murphy]

(In reply to comment #11)
> Sean, I thought there were two potential pages: one for "malware" and one for
> "phishing". I noticed you only added one in your patch. Is there a second to be
> added later or did we combine them or... ?

Hey Sam, I borrowed most of blockedSite.xhtml from Firefox, and the page includes both malware and phishing blocked information, using javascript at runtime to selectively remove some DOM elements based on the type of site blocked.  The about:safebrowsingblocked url is appended certain parameters indicating the blocked type, original URI, etc...

Comment 13

[Samuel Sidler (old account; do not CC)]

Ah, great. I'll admit I didn't test the patch, so I was just guessing based on the patch. Thanks for the explanation!

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 323393 [details] [diff] [review]
Patch, v1

Just a couple of drive-by comments:

>+// HTML report pages
>+pref("browser.safebrowsing.provider.0.reportGenericURL", "http://{moz:locale}.phish-generic.mozilla.com/?hl={moz:locale}");
>+pref("browser.safebrowsing.provider.0.reportErrorURL", "http://{moz:locale}.phish-error.mozilla.com/?hl={moz:locale}");
>+pref("browser.safebrowsing.provider.0.reportPhishURL", "http://{moz:locale}.phish-report.mozilla.com/?hl={moz:locale}");
>+
>+// FAQ URL
>+pref("browser.safebrowsing.warning.infoURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/phishing-protection/");
>

Which of these (if any) are Google-provided, and which will we have to supply ourselves?  

We'll need another bug about hooking up at least "report a suspect site" in the UI, too (at least that seems how we'd handle that one), and we'll have to make sure whatever code substitutes the locales in there works with our locale code.

>\ No newline at end of file

You introduced these into a couple of files with this patch ;)

I also think we need to use Hicks's blocked icon ;)

Comment 15

[Sean Murphy]

(In reply to comment #14)
> (From update of attachment 323393 [details] [diff] [review])
> Just a couple of drive-by comments:
> 
> >+// HTML report pages
> >+pref("browser.safebrowsing.provider.0.reportGenericURL", "http://{moz:locale}.phish-generic.mozilla.com/?hl={moz:locale}");
> >+pref("browser.safebrowsing.provider.0.reportErrorURL", "http://{moz:locale}.phish-error.mozilla.com/?hl={moz:locale}");
> >+pref("browser.safebrowsing.provider.0.reportPhishURL", "http://{moz:locale}.phish-report.mozilla.com/?hl={moz:locale}");
> >+
> >+// FAQ URL
> >+pref("browser.safebrowsing.warning.infoURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/phishing-protection/");
> >
> 
> Which of these (if any) are Google-provided, and which will we have to supply
> ourselves?  

I was thinking we could create our own safe browsing information page on our site, for warning.infoURL.  Google does provide some useful "End-user visible warnings" on http://code.google.com/apis/safebrowsing/developers_guide.html.

As for reporting pages that are either mistakingly blocked, or not yet blocked, we'll need to eventually provide that information to Google.  See http://code.google.com/apis/safebrowsing/developers_guide.html#ReportingData.

> We'll need another bug about hooking up at least "report a suspect site" in the
> UI, too (at least that seems how we'd handle that one), and we'll have to make
> sure whatever code substitutes the locales in there works with our locale code.

Good idea, I'll file a follow up bug.

> 
> >\ No newline at end of file
> 
> You introduced these into a couple of files with this patch ;)

Arg, sorry about that.  I'll make sure to fix those.

> I also think we need to use Hicks's blocked icon ;)

Me too!  The existing "do not enter" symbol I used as part of the alert icon was basically meant to be a placeholder anyway, as it's borrowed from Firefox/toolkit.

Comment 16

[Sean Murphy]

Comment on attachment 323393 [details] [diff] [review]
Patch, v1

(In reply to comment #8)
> Google seems to be having server trouble again, especially with their "gethash"
> URL.  This happened before earlier in the month, which Dave Camp told me "So,
> they were having significant problems with their gethash servers, especially in
> some geographic regions (as far as I know, it mostly only worked in the pacific
> northwest :))."

While Goggle was in fact experiencing some 404 trouble over the weekend, the update pulling failure still exhibited by my patch is due to a mistake on my part.

Setting the data provider URLs with the nsIUrlListManager has the side effect of clearing out all existing registered tables it knows about (not in the actual database, I should make clear, just mere references to table names the list manager knows it should take control over updating).  Since I made a late change which caused the table names to be registered prior to the data provider values, this ultimately meant nsUrlClassifierStreamUpdater::DownloadUpdates() was being called with no tables and no request body, hence the 400 reply from Google.

So, I'm pulling back the review request while I make the few necessary minor changes to address this cleanly.  The good news is that, with the API's sequential behavior in mind, the patch is working excellently and the database is populating very quickly.

I'll also take this opportunity to slightly re-work the preference following preference following behavior of the current patch as well:

Currently, the SafeBrowsingDatabaseService does not really distinguish a phishing blacklist from a malware blacklist, and updates-enabled status is determined instead more generally based upon the "browser.safebrowsing.enabled" preference.  When filing 436968, however, I realized that we might want to follow such a distinction, for more fine grained control over this feature.

Lookup behavior, since it happens through the nsIDocShell's direct communication with the nsIUrlClassifierDBService, is always governed by both the phishing (sb.enabled) and malware (sb.malware.enabled) preferences separately.  The choice of distinguishing between the two types of lists within the SafeBrowsingDatabaseService concerns only which tables shall be actively pulling in updates to the database.

Comment 17

[Sean Murphy]

Attachment: Patch, v2

This patch introduces the ability to differentiate between various list types, (e.g. phishing and malware) rather than just offering update control over all blacklists in general.

Also fixed is the sequential method calling problem where the data provider properties were set after list names were registered, which caused the nsIUrlListManager to forget about the lists and, ultimately, not pull in any updates.

Note that the patch was not generated with the standard cvs diff procedure, since it depends on the not yet checked-in bug 439682.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can any of the URLs end up in WebsiteDefaults.strings like the rest of our URLs?  The URLs are not really prefs, and if we ever want to change them, we can just change WebsiteDefaults.strings instead of having to write pref migration code.  Or does the Gecko urlclassifier stuff call prefs directly?

I spun the site icon part of comment 10 into bug 441732, though we may want it fixed when this lands (and certainly before bug 436968 does).

The warning bar after override/ignore part of comment 10 is now bug 441733.

I built and ran with this patch late last week and over the weekend, and database updates seemed to be pretty quick, despite Fx3 launch. It only took about 3 hours before all of the sites I had randomly picked from the blocked site datastream showed up as blocked (except for one site, which Firefox also refuses to block still, despite it still being in the datastream).  The feature's pretty slick.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

More for "let's find all the anti-phishing-related bugs quickly" than because of hard block/dep relationships, I added the other less-blocked-by-this bugs to the Blocks list.

Comment 20

[Stuart Morgan]

Comment on attachment 326018 [details] [diff] [review]
Patch, v2

This looks great! r=me code-wise with some minor changes and maybe a follow-up bug or two.

On the paperwork side, a few questions:

> Google was unable to grant us an API key for either protocol version

Where are we with this? Do we have permission to actually implement this feature yet, and a 10,000-user-cap exemption?

"Your application is not permitted to show warnings to end users unless it has requested an update in the last 30 minutes without receiving an error response"

Does core take care of this for us? (I'm assuming it handles all of the required back-off logic)

"Our Terms of Service require that if you indicate to users that your service provides malware or phishing protection, you must also let them know that the protection is not perfect. This notice must be visible to them before they enable the protection [...]"

Does this prevent us from defaulting protection to on? How does Firefox handle this, as it's not in their pref UI?

------

General build question: why are we making a new dylib, rather than statically linking your source and the url-classifier library? Wouldn't that be the same number of dependencies on toolkit?

>-HEADER_SEARCH_PATHS = [...]
>+HEADER_SEARCH_PATHS = [...]

The search paths are in alphabetical order so they are easier to maintain; please add the new path accordingly.

>+// Location to more information about the safe browsing feature

Missing a word here

>+pref("browser.safebrowsing.warning.infoURL", "http://en-us.www.mozilla.com/en-US/firefox/phishing-protection/");

Do we really want to point to such a Firefox-centric page? I'd rather we host something on cb.o. (I'm fine with that being a follow-up bug though)

>+    nsresult rv;
>+    NS_ENSURE_ARG_POINTER(aResult);
>+    NS_ENSURE_NO_AGGREGATION(aOuter);
>+    [...]

Two space indenting, not four.

>+DEPTH     = ../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		  = @srcdir@

Don't use tabs for this kind of alignment.

>+- (void)setupSafeBrowsing;

setUp (setup is a noun)

>+- (void)setupSafeBrowsing
>+{
>+  SafeBrowsingListManager *safeBrowsingService = [SafeBrowsingListManager sharedInstance];
>+  [safeBrowsingService registerListWithName:@"goog-phish-shavar" type:eSafeBrowsingPhishingListType];
>+  [safeBrowsingService registerListWithName:@"goog-malware-shavar" type:eSafeBrowsingMalwareListType];
>+  [safeBrowsingService enableUpdateCheckingAccordingToPrefs];
>+}

I'm not clear yet on how the pieces all fit together, but the registration steps here appear to duplicate information in the prefs file (urlclassifier.gethashtables). Do those two things have to stay in sync? If so, we should probably do the registration based on the pref value.

>+- (void)runAwayFromSafeBrowsingBlockedSite
>+{
>+  [self home:self];
>+}

If there is history for this tab, would "back" be a better choice than "home"? (Also something we can take care of in a follow-up.)

>+  NSString *blockingInformationURL = 
>+  [[PreferenceManager sharedInstance] getStringPref:kGeckoPrefSafeBrowsingInformationURL 
>+                                        withSuccess:NULL];

Indent the spillover line two spaces.

>+// Called when the user clicks the "Get Me Out" and "Show More Information" 
>+// buttons on the about:safebrowsingblocked error page, respectively.

Omit the button names; they are likely to get out of sync as we change strings, and I think people can figure out which method is which.

>+CHSafeBrowsingAboutModule::CreateSafeBrowsingAboutModule(nsISupports *aOuter, REFNSIID aIID, void **aResult)
>+{
>+  CHSafeBrowsingAboutModule *aboutModule = new CHSafeBrowsingAboutModule();
>+  if (aboutModule == nsnull)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  NS_ADDREF(aboutModule);
>+  nsresult rv = aboutModule->QueryInterface(aIID, aResult);
>+  NS_RELEASE(aboutModule);
>+  return rv;
>+}

Why the addref+release?

>+// The SafeBrowsingListManager can automatically enable or disable update checking for 
>+// lists when appropriate safe browsing enabled preferences are changed.
>+// This value is set to YES by default.
>+- (BOOL)autoTogglesUpdateCheckingOnChangesToPrefs;
>+- (void)setAutoTogglesUpdateCheckingOnChangesToPrefs:(BOOL)aFlag;

Is there any circumstance where we would expect to disable this? If not, let's not include the API or the implementation plumbing.

>+  if (mName != aName) {
>+    [mName release];
>+    mName = [aName retain];

The existing codebase is somewhat sloppy about this, but for a string it should really be |copy| rather than |retain|, since it may be mutable.

>+    mRegisteredLists = [[NSMutableArray alloc] initWithCapacity:1];

Why 1? If we don't have a strong expectation that we know the right value, we should just leave it to use its default.

>+    if ([aListName isEqualToString:[currentList name]]) {
>+      NSLog(@"SafeBrowsingListManager failed to register list: %@; already managing a list with the same name", aListName);

This is harmless; why log it as an error?

Comment 21

[Sean Murphy]

Attachment: Patch, v2.1

Thanks for the review Stuart!

> > Google was unable to grant us an API key for either protocol version
> 
> Where are we with this? Do we have permission to actually implement this
> feature yet, and a 10,000-user-cap exemption?

I emailed Google with our formal request today, explaining that we are now merely utilizing the same source code as Firefox to access the API.  Google was unable to grant us our own key previously because we sought to write our own implantation from scratch.  They stated that the v2 protocol included some licensed data that they could not give out publicly.

> "Your application is not permitted to show warnings to end users unless it has
> requested an update in the last 30 minutes without receiving an error response"
> 
> Does core take care of this for us? (I'm assuming it handles all of the
> required back-off logic)

Yes, all of the core url-classifier components obey the required back-off behavior.  As for displaying warnings, that feature is driven by the component as well, so it should be obeying that mandate as well.  (Back-off I have confirmed by looking at the code, I still have to find out for sure if the update error behavior is taken care of.)

> "Our Terms of Service require that if you indicate to users that your service
> provides malware or phishing protection, you must also let them know that the
> protection is not perfect. This notice must be visible to them before they
> enable the protection [...]"
> 
> Does this prevent us from defaulting protection to on? How does Firefox handle
> this, as it's not in their pref UI?

Firefox displays this information in the EULA that appears when mounting their disk image:

"5.  WEBSITE INFORMATION SERVICES.  Mozilla and its contributors, licensors and partners work to provide the most accurate and up-to-date phishing and malware information.  However, they cannot guarantee that this information is comprehensive and error-free: some risky sites may not be identified, and some safe sites may be identified in error."

I suppose we'll have to do the same if we want to enable the feature by default, as it's one of the only ways to non-obtrusively display the warning beforehand.

> General build question: why are we making a new dylib, rather than statically
> linking your source and the url-classifier library? Wouldn't that be the same
> number of dependencies on toolkit?

The url-classifier is an intermediary static library, and without using the entire toolkit library we needed to handle the registration and creation process of the various components implemented by it.  Rather than having Camino searching for headers (in toolkit/components/url-classifier/src) and linking to other dependencies, creating another component dylib using the Mozilla build process seemed like the cleanest way to go. If that doesn't seem like the best way, can you elaborate on the alternative statically linked approach you mentioned?

> >+pref("browser.safebrowsing.warning.infoURL", "http://en-us.www.mozilla.com/en-US/firefox/phishing-protection/");
> 
> Do we really want to point to such a Firefox-centric page? I'd rather we host
> something on cb.o. (I'm fine with that being a follow-up bug though)

Yes, we should definitely have our own - Forgot to file that before; Under bug 451092.

> >+- (void)setupSafeBrowsing
> >+{
> >+  SafeBrowsingListManager *safeBrowsingService = [SafeBrowsingListManager sharedInstance];
> >+  [safeBrowsingService registerListWithName:@"goog-phish-shavar" type:eSafeBrowsingPhishingListType];
> >+  [safeBrowsingService registerListWithName:@"goog-malware-shavar" type:eSafeBrowsingMalwareListType];
> >+  [safeBrowsingService enableUpdateCheckingAccordingToPrefs];
> >+}
> 
> I'm not clear yet on how the pieces all fit together, but the registration
> steps here appear to duplicate information in the prefs file
> (urlclassifier.gethashtables). Do those two things have to stay in sync? If so,
> we should probably do the registration based on the pref value.

Yeah, I wasn't in love with statically listing the list names in code either.  The problem with the "gethashtables" pref is that it, while those are the exact two list names we'll use, the preference is referring to a specific implementation of the url-classifier.  The fact that those tables, and the lists we're interested in, are identical is just a fact of how the url-classifier is implemented right now, with no guarantee that the names in that pref will always mirror exactly the lists we want.

We could create a new pref, listing the necessary lists, to keep from having to recompile and release upon changes on the provider end.  The other problem is that the type of list is also necessary, but this can be deduced from the -phish- or -malware- part, though.

> >+- (void)runAwayFromSafeBrowsingBlockedSite
> >+{
> >+  [self home:self];
> >+}
> 
> If there is history for this tab, would "back" be a better choice than "home"?
> (Also something we can take care of in a follow-up.)

I thought that was a good idea, and it was simple to implement right now.

> >+CHSafeBrowsingAboutModule::CreateSafeBrowsingAboutModule(nsISupports *aOuter, REFNSIID aIID, void **aResult)
> >+{
> >+  CHSafeBrowsingAboutModule *aboutModule = new CHSafeBrowsingAboutModule();
> >+  if (aboutModule == nsnull)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+  NS_ADDREF(aboutModule);
> >+  nsresult rv = aboutModule->QueryInterface(aIID, aResult);
> >+  NS_RELEASE(aboutModule);
> >+  return rv;
> >+}
> 
> Why the addref+release?

Most component constructors seem to follow this approach, especially all of Mozilla's nsIAboutModules; the NS_GENERIC_FACTORY_CONSTRUCTOR, for example, does it as well. I guess it's to ensure the module stays around though QueryInterface.

All other changes were made.  Thanks again for taking time for the review.

Comment 22

[Mike Pinkerton (not reading bugmail)]

+class CHSafeBrowsingAboutModule : public nsIAboutModule
+{
+public:

other classes in this patch indent @public/@private 1 space, might as well be consistent.

SafeBrowsingList's dealloc should be up by the inits, not at the end of the impl.

+#define STRCOMP_EQUAL 0

why not make this a const int?

sr=pink

Comment 23

[Sean Murphy]

Attachment: Patch, For Check In

Updated with pink's comments.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Pushing out, since we can't get permission from Google.

Comment 25

[Sean Murphy]

Attachment: Updated Patch

Updated the patch to work with our current source tree, and Google's Safe Browsing API v2.2.

Note that, until we get activated on Google's API server, the server will return a "503" error when trying to retrieve updates.  I tested that the patch works though, by masquerading as Firefox temporarily.

For testing, you can prevent the 503 errors by changing the API urls in user preferences:

user_pref("browser.safebrowsing.provider.0.updateURL", "http://safebrowsing.clients.google.com/safebrowsing/downloads?client=Firefox&appver=3.0.5&pver=2.2");
user_pref("browser.safebrowsing.provider.0.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=Firefox&appver=3.0.5&pver=2.2");
user_pref("browser.safebrowsing.provider.0.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=Firefox&appver=3.0.5&pver=2.2");

Also, you can tell the underlying Gecko list manager to log everything that's going on by changing G_GDEBUG to true, on line 9 of nsUrlClassifierLib.js.

Note that no major changes were performed since the previous patch,  I only updated the Gecko preference URLs and refreshed it to apply to our current source tree.

Comment 26

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: As checked in

I removed some Xcode-is-stupid cruft from the project changes; this is the patch as checked in.

Landed on cvs trunk.

Thanks for all your work on this, Sean!  On to the follow-up bugs :P

Comment 27

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Just for the record, Ts on boxset went from the 1385ms area to the 1640ms area, or about an 18% regression (did the symbol table really get that much bigger)?  

cb-xserve01 has not noticed a thing, h-103 has edged just a little bit higher, and as of this writing, cb-minibinus has still not completed the build(!) Somehow this took approximately the same time as a clobber build for everyone who's finished.

When all is said and done, I think we'll see a slight Ts increase on the slower boxen (static), and just have to hope we don't ever tax dyld more on shared.

Comment 28

[Eiichi]

I make a test with my build and when launching Camino, Console logs;
------------
Camino[15334] Error initializing SafeBrowsingListManager: Failed to get the '@mozilla.org/url-classifier/listmanager' service
------------

What's wrong?

Comment 29

[Eiichi]

I test with official 2009-01-29-00-2.0-M1.9 using fresh profile, but that's the same.

Console logs;
------------
Camino[15612] Error initializing SafeBrowsingListManager: Failed to get the '@mozilla.org/url-classifier/listmanager' service
------------

Comment 30

[Sean Murphy]

(In reply to comment #28)

Eiichi, thanks for alerting us of this.  Is your own test build a static build of Camino?  I suspect there might be a problem statically linking the safe browsing component.  The nightly build of Camino is static as well.

Much of our testing was on a dynamically linked build, so I'm currently doing a static build to see if we need to fix the safe browsing component under it.

Comment 31

[philippe (part-time)]

(In reply to comment #30)
> (In reply to comment #28)
> Is your own test build a static build
> of Camino?  I suspect there might be a problem statically linking the safe
> browsing component.  The nightly build of Camino is static as well.

My home made build is static as well, and gives the same console error.

ac_add_options --enable-static

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #27)
> When all is said and done, I think we'll see a slight Ts increase on the slower
> boxen (static), and just have to hope we don't ever tax dyld more on shared.

No noticeable Ts change on cb-minibinus01, but boxset also has experienced a linear-looking Tp increase that so far has taken it from ~535ms to ~550ms.

Comment 33

[Eiichi]

(In reply to comment #30)
> Eiichi, thanks for alerting us of this.  Is your own test build a static build
> of Camino? 

Yes, my test build is a static build as well as an official nightly build of Camino.

Comment 34

[Eiichi]

Just for reference;
I can't find "../dist/bin/components/libcaminosafebrowsing.dylib" in my source tree.

Comment 35

[Sean Murphy]

(In reply to comment #34)
> Just for reference;
> I can't find "../dist/bin/components/libcaminosafebrowsing.dylib" in my source
> tree.

Yeah, Sorry about this guys, it's definitely a problem statically linking the safe browsing XPCOM component.

There is a static library, libcaminosafebrowsing.a, properly built with static builds in place of the .dylib, which I should have linked the StaticApp target with instead to begin with (this causes Xcode's warning about the dylib).

Just performing that simple fix doesn't solve the problem by itself though, like I thought it would - we still can't instantiate the '@mozilla.org/url-classifier/listmanager' service.  I'll figure out what the deal is asap!

Comment 36

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Since we don't really care about the shared build, reopening until static is fixed.

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix the static build

Sean did all the hard work debugging this (I did all the bonehead-I-should-know-better-project-hacker-should-have-caught-this stuff), and this patch works for me.  He was still seeing some problems in his tree, so we'll use this patch/review to track making sure things are working on his end.

Note for those of you playing along at home: you won't see anything (except the lack of console error) unless you've configured your prefs as mentioned above, but please only use that for testing; the UI is still unfinished and Google hasn't activated us yet.  Also, it seems like it takes about 2 hours for the db to reach "full size" so that it catches most phishing and malware sites you can find, so don't expect to see anything immediately after starting to test.

Comment 38

[Eiichi]

(In reply to comment #37)

Thanks, Smokey.
I applied the patch:Created an attachment (id=360020) and made the test build.

Results:
1) Console logs no errors.
2) "urlclassifier3.sqlite" and "urlclassifierkey3.txt" are made in "~/Library/Application Support/Camino/".

Comment 39

[philippe (part-time)]

Builds fine (gcc4.2/10.5sdk). After a few hours, urlclassifier3.sqlite has grown to a 19.3Mb file (that seems ok, about the same as Minefield on a fresh profile).
I didn't notice any slowdown while the db gets its food. But I have a fatty 'net connection.

Comment 40

[Sean Murphy]

Comment on attachment 360020 [details] [diff] [review]
Fix the static build

Thanks Smokey for getting the patch in order for this.  Definitely works for me now here too, and everything in the Xcode proj looks good.

Thanks Eiichi and philippe for noticing the static build issue, and for your help testing.

Comment 41

[Stuart Morgan]

Comment on attachment 360020 [details] [diff] [review]
Fix the static build

sr=smorgan; sorry I didn't catch this.

Comment 42

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #41)
> (From update of attachment 360020 [details] [diff] [review])
> sr=smorgan; sorry I didn't catch this.

Checked in on cvs trunk; really FIXED now :)