Tracking protection requires us to cancel speculative connections if they are found in the local blocklist. Malware/phishing sites however don't need that kind of protection and it could lead to performance problems if loading URLs whose partial hash conflicts with something in the Safe Browsing list. We should only look at the tracking table when doing the ClassifyLocal call.

This could help address the regression discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1122691#c26.

Bug 1180323 - Only look at TP table before cancelling speculative connections. r=sworkman

Comment on attachment 8629555 [details] MozReview Request: Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp Asking gcp for the review instead, since he reviewed the earlier fix (bug 1122691).

Comment on attachment 8629555 [details] MozReview Request: Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp Bug 1180323 - Only look at TP table before cancelling speculative connections. r=sworkman

Comment on attachment 8629555 [details] MozReview Request: Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp https://reviewboard.mozilla.org/r/12633/#review11115 ::: netwerk/protocol/http/nsHttpChannel.cpp:5004 (Diff revision 1) > - LOG(("nsHttpChannel::ClassifyLocal found principal on local " > + rv = classifier->ClassifyLocalWithTables(principal, tables, results); I think this removes the last external user of ClassifyLocal, so clean up the IDL API: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIURIClassifier.idl#63 ::: netwerk/protocol/http/nsHttpChannel.cpp:5006 (Diff revision 1) > + FindInReadable(NS_LITERAL_CSTRING("-track-"), results)) { You're already passing in the list of tables you're checking against, and it's only tracking tables. So do you actually need to scan the result? If it's not empty you have hit, right? ::: netwerk/protocol/http/nsHttpChannel.cpp:5002 (Diff revision 1) > - classifier->ClassifyLocal(principal, tp, &response); > + Preferences::GetCString("urlclassifier.trackingTable", &tables); Not sure what the overhead is for grabbing the pref, but we hit this on every URL lookup. Does it make sense to statically cache this somehow?

Comment on attachment 8629555 [details] MozReview Request: Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp

I've addressed your first two comments in the latest revision of my patch. (In reply to Gian-Carlo Pascutto [:gcp] from comment #5) > ::: netwerk/protocol/http/nsHttpChannel.cpp:5002 > (Diff revision 1) > > - classifier->ClassifyLocal(principal, tp, &response); > > + Preferences::GetCString("urlclassifier.trackingTable", &tables); > > Not sure what the overhead is for grabbing the pref, but we hit this on > every URL lookup. Does it make sense to statically cache this somehow? Not sure either about the overhead (it's actually better than before though because we only lookup one pref instead of three in BuildTables() which was called by ClassifyLocal()) but if we cache it than it means that add-ons that want to add a table to it will not be able to be restartless.

Comment on attachment 8629555 [details] MozReview Request: Bug 1180323 - Only look at TP table before cancelling speculative connections. r?gcp https://reviewboard.mozilla.org/r/12633/#review11809 Ship It!