453723 - Short-circuit known-clean hosts in the urlclassifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

Attachment: v1

After a lookup on a clean host, we don't need to keep checking every url on that host.  Attached patch keeps a small cache of clean host keys, and shortcuts lookups on these hosts.

The cache is reset after an update.

Comment 1

[Tony Chang (Google)]

Comment on attachment 336932 [details] [diff] [review]
v1

>+  PRLock* mCleanHostKeysLock;

Can you add a comment explaining what the lock is used to protect?

>+  // First, if this key has been checked since our last update and had
>+  // no entries, we can exit early.  We also do this check before
>+  // posting the lookup to this thread, but in case multiple lookups
>+  // are queued at the same time, it's worth checking again here.
>+  nsAutoLock lock(mCleanHostKeysLock);
>+
>+  if (mCleanHostKeys.Has(hostKey))
>+    return NS_OK;
>+
>+  lock.unlock();

Maybe put this code in a local scope so you don't need to manually call lock.unlock()?  Same with the use of the autolock below.

> nsresult
> nsUrlClassifierDBService::LookupURI(nsIURI* uri,
>-                                    nsIUrlClassifierCallback* c)
>+                                    nsIUrlClassifierCallback* c,
>+                                    PRBool forceLookup,
>+                                    PRBool *didLookup)

Can we put all the in params before the out params (so uri, forceLookup, c, didLookup)?  Isn't that what normally happens in other Moz code?

>+class nsUrlClassifierFragmentSet

There should be a comment describing this class (linked list).


Should there be new unittests or do the existing ones cover it?  E.g., querying the same host twice or query host, update, query host.

Comment 2

[Dave Camp (:dcamp)]

(In reply to comment #1)

> > nsresult
> > nsUrlClassifierDBService::LookupURI(nsIURI* uri,
> >-                                    nsIUrlClassifierCallback* c)
> >+                                    nsIUrlClassifierCallback* c,
> >+                                    PRBool forceLookup,
> >+                                    PRBool *didLookup)
> 
> Can we put all the in params before the out params (so uri, forceLookup, c,
> didLookup)?  Isn't that what normally happens in other Moz code?

uri, c, and forcelookup are all in parameters...

> >+class nsUrlClassifierFragmentSet
> 
> There should be a comment describing this class (linked list).
> Should there be new unittests or do the existing ones cover it?  E.g., querying
> the same host twice or query host, update, query host.

Oops, didn't hg add the unit test file.  Will post a new patch soon...

Comment 3

[Dave Camp (:dcamp)]

Attachment: v2

This new version adds a couple more small caches.  The first is a clean fragment cache.  So if we're doing checks on a host that does have malware, we'll avoid hashing/looking up fragments that have already been checked.  This should help avoid excess hashing on pages that load a lot of similar urls:

http://foo.bar.com/app/scripts/foo.js
http://foo.bar.com/app/scripts/bar.js
http://foo.bar.com/app/scripts/baz.js

Previously, this would generate hashes for each of foo.bar.com/, foo.bar.com/app, and foo.bar.com/app/scripts for each of these lookups.  With this patch, we'll only generate those once (during the first check), and then only check the leaf nodes on subsequent loads.

Finally, we cache the last db lookup of a host key.  So for the examples above, only the first load will result in a db lookup for foo.bar.com.  Subsequent checks will just look at the results from the earlier foo.bar.com query.

Between these three caches, a typical page load will still only result in one or two db lookups, and subsequent checks for script, css, etc should do minimal work.

Comment 4

[Tony Chang (Google)]

Comment on attachment 340462 [details] [diff] [review]
v2

looks good!

Comment 5

[Dave Camp (:dcamp)]

Landed as http://hg.mozilla.org/mozilla-central/rev/1a971f9406f8

Comment 6

[Dave Camp (:dcamp)]

unit test failed, this was backed out.

Comment 7

[Dave Camp (:dcamp)]

Attachment: followup

Thunderbird and seamonkey unit tests were failing because they don't turn on the malware/phishing prefs.  New patch turns them on explicitly in the unit tests, and also clears the caches during a database reset.

Comment 8

[Tony Chang (Google)]

Comment on attachment 341007 [details] [diff] [review]
followup

OK

Comment 9

[Dave Camp (:dcamp)]

Landed as http://hg.mozilla.org/mozilla-central/rev/74aad43f37a5

Comment 10

[Dave Camp (:dcamp)]

Attachment: test workaround

There's a race in the test cases (starting a lookup directly after a resetDatabase call).  I'm landing this workaround until the race (bug 457790) can be properly fixed.

Comment 11

[Dave Camp (:dcamp)]

Reverted again, due to bug 457828.

Comment 12

[Dave Camp (:dcamp)]

Attachment: v3

New patch includes a fix for the crash, and a test that triggers the crash without this fix.  Interdiff coming up...

Comment 13

[Dave Camp (:dcamp)]

Attachment: interdiff

Comment 14

[Tony Chang (Google)]

Comment on attachment 343641 [details] [diff] [review]
v3

make it so

Comment 15

[Dave Camp (:dcamp)]

(re-re-)landed as http://hg.mozilla.org/mozilla-central/rev/7825b962281d

Comment 16

[Dave Camp (:dcamp)]

err, actually it was http://hg.mozilla.org/mozilla-central/rev/82af81d68210