In order to implement the support for new lists, it's important to cleanup the current URLClassifier code. I want to propose a way to do it. I hope to receive some positive feedback. 1. UrlClassifierFeatureFactory - is a factory which returns a list of features from a channel. 2. nsIUrlClassifierFeature is xpcom interface which represents a Url-Classifier feature. For instance: tracking-annotation and tracking-protection are 2 features. Each one has a list of tables, a set preferences, and 1 name. 3. A feature is able to process a channel when needed: tracking-protection will cancel the channel, tracking-annotation will annotate it. 4. nsIURIClassifier.asyncClassifyLocalWithFeatures() is a new function that receives an URL and a list of features, a blacklist/whitelist boolean and a callback. The callback is a nsIUrlClassifierFeatureCallback interface. 5. nsIUrlClassifierFeatureCallback is called when the lookup is completed. It receives the list of matching features. Now, I'll show some code. It's not completed, but it gives an idea of the refactoring.

This first patch is just about moving some prefs in StaticPrefs. I need them for the following patches.

Here we have the feature factory. There is a base class for all 2 existing features: tracking-protection and tracking-annotation

Here the nsUrlClassifierDBService::AsyncClassifyLocalWithFeatures. I had to touch IPC and other details...

nsChannelClassifier cleanup.

Much better approach using enum in xpcom IDL.

All of this works fine. I also support the skiplist. The remaining bit is to inform the UI about the tracking/annotation. I'll work on that part too.

Comment on attachment 9029131 [details] [diff] [review] part 1 - urlClassifierFeature and factory Review of attachment 9029131 [details] [diff] [review]: ----------------------------------------------------------------- I think the idea here is great! It really makes things more modualized and clear thank you, baku! Personally I would prefer putting the idl in toolkit/components/url-classifier/ and the feature implementation in the corresponding folder, for example, netwerk/base here. Because lots of stuff in the feature implementation are not really related to the url-classifier and are more network related in this case(It may also easier for people to know who should review the code :) ) But I guess probably because the factory, base and common, it is easier to put these in a central place. Please let me know how do you think.

Comment on attachment 9029132 [details] [diff] [review] part 2 - nsUrlClassifierDBService::AsyncClassifyLocalWithFeatures Review of attachment 9029132 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +1955,5 @@ > > + // Let's convert the current params in a list of features. > + nsTArray<RefPtr<nsIUrlClassifierFeature>> features; > + > + for (uint32_t i = 0; i < aExtraTablesByPrefs.Length(); ++i) { I guess we can remove these parameters now. They are moved inside the feature. @@ +1965,5 @@ > } > > + nsTArray<nsCString> tables; > + Classifier::SplitTables(aTables, tables); > + for (uint32_t i = 0; i < tables.Length(); ++i) { If you agree the comment below to have the ability to know all the tables match for a feature. We may just use one DummyFeature here @@ +2824,5 @@ > + for (uint32_t i = 0; i < results.Length(); ++i) { > + bool hasTable; > + rv = feature->HasTable(results[i]->mTableName, aListType, > + &hasTable); > + if (NS_SUCCEEDED(rv) && hasTable) { One thing we are missing here is the ability to know all the tables are matched. I think this is necessary because the feature may need to decide what to do in ProcessChannel based on the tables matched. For example, a url matched both "Analytics" table and "Fingerprinting" table. @@ +2874,5 @@ > + if (matchingFeatures.IsEmpty()) { > + return false; > + } > + > + // If we have some match using the preferences, we don't need to continue. Is this correct? I think the host preference only applies to the feature it is defined. For example, we can have a preference to ignore certain host for tracking protection but we still want to see if it is in tracking annotation table. I guess the reason we separate host preference and table preference is to be able to skip finding the URI in the feature's table. Maybe what we want to do here is remove those features found a match here and keep finding the remaining feature from table preference.

Comment on attachment 9029133 [details] [diff] [review] part 3 - cleanup nsChannelClassifier Review of attachment 9029133 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +684,2 @@ > > + rv = mChannelClassifier->IsTrackerWhitelisted(whitelistURI, mFeatures, Not sure if I understand correctly. Here we check the whitelist for all the features in blacklist. Should we only check whitelist for those found in blacklist? @@ +781,5 @@ > nsCOMPtr<nsIURIClassifier> uriClassifier = > do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID, &rv); > NS_ENSURE_SUCCESS(rv, rv); > > + return uriClassifier->AsyncClassifyLocalWithFeatures(aWhiteListURI, aFeatures, One thing I think we can think about is the whitelist. Right now in tracking protection, we use a pairwise whitelist. The current API design doesn't allow us to look for blocklist and whitelist in a single lookup. I guess the most general way is to be able to specify the combination of allow/block for each feature. But I am not sure if this is necessary because maybe there won't be such a use case here.

Comment on attachment 9029535 [details] [diff] [review] part 0 - StaticPref Review of attachment 9029535 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/TrackingDummyChannel.cpp @@ +60,5 @@ > > nsCOMPtr<nsIURI> uri; > aChannel->GetURI(getter_AddRefs(uri)); > > + if (StaticPrefs::privacy_trackingprotection_annotate_channels()) { We can also remove the |ChannelNeedsToBeAnnotated| function entirely now

Comment on attachment 9029536 [details] [diff] [review] part 1 - urlClassifierFeature and factory Review of attachment 9029536 [details] [diff] [review]: ----------------------------------------------------------------- Hi baku, This looks great, thanks for working on this! r- because I would like to discuss with you about where to put the feature implementation. I suggest we put UrlClassifierFeatureTrackingXXX, UrlClassifierCommon to netwerk/base, UrlClassifierFeatureLoginReputation to toolkit/components/reputationservice and UrlClassifierFeatureFlash to dom/base The feature implementation depends on the code around and is not what I expect to see in the url-classifier folder. I think this way it is more clear because url-classifier only contains things related to SafeBrowsing protocol implementation and those features are also put to where they most related to. To achieve this, maybe we could provide some kind of "register" interface in urlClassifierFeatureFactory so a feature can register it to the factory and the factory knows who to call Init, Shutdown, Create ...etc How do you think? ::: toolkit/components/url-classifier/UrlClassifierCommon.cpp @@ +2,5 @@ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Prefer changing the file name to UrlChannelClassifierCommon.cpp so we know that channel related stuff are put here(to distinguish with the nsUrlClassifierUtils.cpp) But if we move these to network/base then the file name is ok. @@ +22,5 @@ > + > +const nsCString::size_type UrlClassifierCommon::sMaxSpecLength = 128; > + > +// MOZ_LOG=UrlClassifier:5 > +LazyLogModule UrlClassifierCommon::sLog("UrlClassifier"); We used to use "nsChannelClassifier:5" to debug code in ChannelClassifier. We can change this to LazyLogModule UrlChannelClassifierCommon::sLog("UrlChannelClassifier") to make it more clear that this print channel related debug message @@ +91,5 @@ > + aBlockingPurpose == AntiTrackingCommon::eTrackingAnnotations); > + > + nsCOMPtr<nsIHttpChannelInternal> channel = do_QueryInterface(aChannel); > + if (!channel) { > + UC_LOG(("UrlClassifierCommon: Not an HTTP channel")); From past experience, it is quite useful to debug the code with a fixed format that shows the current channel LOG(("nsChannelClassifier[%p]: ...)); It will be really useful if we change here and other places to UC_LOG(("UrlChannelClassifier[%p]: ...", aChannel)); UC_LOG(("UrlClassifierFeatureXXX[%p]: ...", aChannel)); @@ +146,5 @@ > + // Tracking protection will be disabled so update the security state > + // of the document and fire a secure change event. If we can't get the > + // window for the channel, then the shield won't show up so we can't send > + // an event to the securityUI anyway. > + UrlClassifierCommon::NotifyTrackingProtectionDisabled(aChannel); Probably not related to this bug because in this patch we just refactor the code and make sure the code flow is exactly the same as before. It is just strange to me that we did an action, "NotifyTrackingProtectionDisabled", inside a ShouldXXX function. So whenever someone uses the Shouldxxx and then it generates a notification. Maybe this is something worth refactoring in the future? ::: toolkit/components/url-classifier/UrlClassifierFeatureBase.cpp @@ +18,5 @@ > + MOZ_ASSERT(aArray); > + > + nsAutoCString value; > + Preferences::GetCString(aPrefName, value); > + Classifier::SplitTables(value, *aArray); Classifier::SplitTables now is used to separate any comma separated string, sort, and then remove the duplicates. This utility function are not really related to anything inside the "class Classifier". I'll file a bug to rename it and maybe move it to a better place. @@ +110,5 @@ > +UrlClassifierFeatureBase::GetTables(nsIUrlClassifierFeature::listType aListType, > + nsTArray<nsCString>& aTables) { > + if (aListType != nsIUrlClassifierFeature::blacklist && > + aListType != nsIUrlClassifierFeature::whitelist) { > + return NS_ERROR_FAILURE; NS_ERROR_INVALID_ARG @@ +125,5 @@ > + NS_ENSURE_ARG_POINTER(aResult); > + > + if (aListType != nsIUrlClassifierFeature::blacklist && > + aListType != nsIUrlClassifierFeature::whitelist) { > + return NS_ERROR_FAILURE; ditto @@ +140,5 @@ > + NS_ENSURE_ARG_POINTER(aResult); > + > + if (aListType != nsIUrlClassifierFeature::blacklist && > + aListType != nsIUrlClassifierFeature::whitelist) { > + return NS_ERROR_FAILURE; ditto ::: toolkit/components/url-classifier/UrlClassifierFeatureFactory.cpp @@ +44,5 @@ > + > + // Note that the order is extremely important here because if 2 features match > + // the channel's URI, we return the error code of the first one only. This > + // means that 'Tracking Protection' is more important than 'Tracking > + // Annotation'. If I understand correctly, what you are saying here is because of the order here affects the order we call feature::ProcessChannel and the order of the ProcessChannel may affect the result, is that correct? If yes, it will be very useful and clear if you can add more comments here to describe this. ::: toolkit/components/url-classifier/UrlClassifierFeatureResult.h @@ +21,5 @@ > + const nsACString& aList); > + > + nsIUrlClassifierFeature* Feature() const { return mFeature; } > + > + const nsCString& List() const { return mList; } Add a comment about this is comma separated tables ::: toolkit/components/url-classifier/UrlClassifierFeatureTrackingAnnotation.cpp @@ +59,5 @@ > +} > + > +/* static */ already_AddRefed<UrlClassifierFeatureTrackingAnnotation> > +UrlClassifierFeatureTrackingAnnotation::MaybeCreate(nsIChannel* aChannel) { > + MOZ_ASSERT(gFeatureTrackingAnnotation); MOZ_ASSERT(aChannel) ::: toolkit/components/url-classifier/UrlClassifierFeatureTrackingProtection.cpp @@ +55,5 @@ > +} > + > +/* static */ already_AddRefed<UrlClassifierFeatureTrackingProtection> > +UrlClassifierFeatureTrackingProtection::MaybeCreate(nsIChannel* aChannel) { > + MOZ_ASSERT(gFeatureTrackingProtection); MOZ_ASSERT(aChannel); ::: toolkit/components/url-classifier/nsIUrlClassifierFeature.idl @@ +51,5 @@ > + readonly attribute ACString skipHostList; > +}; > + > +/** > + * The result of the classifier operation is this interface. I think it is worth adding comment that this is used by asyncClassifyLocalWithFeatures in nsIURIClassifier.idl @@ +63,5 @@ > + readonly attribute ACString list; > +}; > + > +/** > + * Callback function for nsIURIClassifier lookups. ditto

Comment on attachment 9029537 [details] [diff] [review] part 2 - nsUrlClassifierDBService::AsyncClassifyLocalWithFeatures Review of attachment 9029537 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks! ::: dom/ipc/PContent.ipdl @@ +815,2 @@ > // The async version of ClassifyLocal. > + async PURLClassifierLocal(URIParams uri, IPCURLClassifierFeature[] aFeatures); maybe just "features" to sync the style with surrounding code ::: dom/ipc/URLClassifierChild.h @@ +52,5 @@ > + > + nsTArray<URLClassifierLocalResult> results = std::move(aResults); > + for (URLClassifierLocalResult& result : results) { > + for (nsIUrlClassifierFeature* feature : mFeatures) { > + nsCString name; Nit. nsAutoCString ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +1959,5 @@ > + nsTArray<nsCString> tables; > + Classifier::SplitTables(aTables, tables); > + for (uint32_t i = 0; i < tables.Length(); ++i) { > + RefPtr<DummyFeature> feature = new DummyFeature(tables[i]); > + features.AppendElement(feature); Can we merge all the tables into a single DummyFeature? But maybe this is not important because I saw you remove here entirely in another patch. @@ +2721,5 @@ > + nsAutoCString skipHostList; > + rv = feature->GetSkipHostList(skipHostList); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } I think it would be safer if we use "continue" instead of "return" in this loop so an error won't cause us to give up the lookup entirely @@ +2805,5 @@ > + MOZ_ASSERT(!tableName.IsEmpty()); > + > + RefPtr<mozilla::UrlClassifierFeatureResult> result = > + new mozilla::UrlClassifierFeatureResult(feature, tableName); > + results.AppendElement(result); A LOG message here would be great so we know we found a match. @@ +2813,5 @@ > + if (results.IsEmpty()) { > + return false; > + } > + > + // If we have some match using the preferences, we don't need to continue. I understand that we can skip searching URL if we found any match here because this is used by testcase only. Maybe we could add some comments about the behavior and the host preference is only for testcase in UrlClassifierFeatureBase so anyone wants to add that for his "feature" is more likely to see it.

Comment on attachment 9029972 [details] [diff] [review] part 3 - AsyncUrlClassifier Review of attachment 9029972 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +284,5 @@ > // Should only be called in the parent process. > MOZ_ASSERT(XRE_IsParentProcess()); > > // Don't cache tracking classifications because we support allowlisting. > if (status == NS_ERROR_TRACKING_URI || mIsAllowListed) { This flag is not set now because it is moved to AsyncChannelClassifier. Not sure if this is still required because this seems to be a fix for mochitest test_classified_annotations.html? [1] Does this patch pass this test? [1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1054572&attachment=8476293 ::: netwerk/base/nsChannelClassifier.h @@ +13,5 @@ > #include <functional> > > class nsIChannel; > class nsIHttpChannelInternal; > class nsIDocument; nsIHttpChannelInternal & nsIDocument can be removed ::: toolkit/components/url-classifier/AsyncUrlChannelClassifier.cpp @@ +35,5 @@ > + nsresult rv = > + r->Feature()->ProcessChannel(aChannel, r->List(), &shouldContinue); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } Error return here causes us miss the callback. Maybe add a LOG here and keep executing next channel's ProcessChannel when there is an error. @@ +182,5 @@ > + > + // Maybe we have to skip this host > + nsAutoCString skipList; > + nsresult rv = blacklistFeature->GetSkipHostList(skipList); > + if (NS_WARN_IF(NS_FAILED(rv))) { Error return without calling |mChannelCallback| in this function. I guess we just treat this error as URI is not in host skip list. @@ +221,5 @@ > + > + // The lookup failed to match at least one of the active whitelists > + // (tracking protection takes precedence over tracking annotations) > + > + // Change our error code if the URL has been whitelisted. We don't need this comment now. @@ +321,5 @@ > + nsCOMPtr<nsIUrlClassifierFeatureCallback> callback = > + new WhitelistClassifierCallback(mChannel, mURI, aResults, > + std::move(mChannelCallback)); > + > + rv = IsTrackerWhitelisted(whitelistURI, mFeatures, callback); We are looking for all the features. I think we can just look for features found in blacklist here. @@ +342,5 @@ > + > +} // namespace > + > +/* static */ nsresult AsyncUrlChannelClassifier::CheckChannel( > + nsIChannel* aChannel, std::function<void()>&& aCallback) { MOZ_ASSERT(XRE_IsParentProcess()); MOZ_ASSERT(aChannel); ::: toolkit/components/url-classifier/UrlClassifierFeatureTrackingProtection.cpp @@ +107,5 @@ > + NS_ENSURE_ARG_POINTER(aChannel); > + NS_ENSURE_ARG_POINTER(aShouldContinue); > + > + // This is a blocking feature. > + *aShouldContinue = false; Just an idea that maybe we can remove |aShouldContinue| parameter and use other status flag to see if we should do things in ::ProcessChannel. For example, maybe channel->GetStatus can be used in TrackingAnnotation::ProcessChannel, so we know if we should/can call stuff like lowerNetwrokPriority ... etc based on that. ::: toolkit/components/url-classifier/nsIUrlClassifierFeature.idl @@ +60,5 @@ > + * Returns if we should process other feature results or not. For instance, > + * tracking-protection cancel the channel, and after that we should stop > + * processing other features. > + */ > + [noscript] boolean processChannel(in nsIChannel channel, in ACString list); Nit. aChannel & aList

Comment on attachment 9030285 [details] [diff] [review] part 4 - lookup optimization Review of attachment 9030285 [details] [diff] [review]: ----------------------------------------------------------------- Hi baku, I would like to discuss this with you first before digging into the code there. I don't like the idea that moves the ::GetLookupFragments outside the Classifier::Check. This is because the fragment stuff is more like a SafeBrowsing spec thing so we used to put it inside the Classifier.cpp and can also make sure anyone uses this internal API follow the spec. Is it possible that we merge all the tables in those features, remove the duplicate table and then call DoLocalLookup. After return, we just check those matched tables are in which features.

> I understand that we can skip searching URL if we found any match here > because this is used by testcase only. I would like devtools to use this feature to enable/disable/classify custom URLs. Would be a really great feature! Maybe we want something for webextensions as well.

Comment on attachment 9030662 [details] [diff] [review] part 4 - lookup optimization Review of attachment 9030662 [details] [diff] [review]: ----------------------------------------------------------------- Hi baku, I can't find |nsUrlClassifierDBServiceWorker::DoLocalLookupWithURI| Can you help check? ::: toolkit/components/url-classifier/Classifier.cpp @@ +417,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > LookupCacheArray cacheArray; > for (const nsCString& table : aTables) { > LOG(("Checking table %s", table.get())); Remove this LOG, we already print it in CheckURIFragments @@ +431,5 @@ > +} > + > +nsresult Classifier::CheckURIFragments( > + const nsTArray<nsCString>& aSpecFragments, const nsACString& aTable, > + LookupResultArray& aResults) { To make sure we follow the spec, we can add a MOZ_ASSERT here: // A URL can form up to 30 different fragments MOZ_ASSERT(aSpecFragments.Length() <= (MAX_HOST_COMPONENTS * (MAX_PATH_COMPONENTS + 2))); sorry for the strange "+2" here, I'll fix this in another bug, let's just use this magic value here (or just use 30). @@ +432,5 @@ > + > +nsresult Classifier::CheckURIFragments( > + const nsTArray<nsCString>& aSpecFragments, const nsACString& aTable, > + LookupResultArray& aResults) { > + Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_CL_CHECK_TIME> timer; We need to move this telemetry to Classifier::CheckURI, otherwise, the telemetry may drop significantly ::: toolkit/components/url-classifier/Classifier.h @@ +62,5 @@ > + nsresult CheckURI(const nsACString& aSpec, const nsTArray<nsCString>& tables, > + LookupResultArray& aResults); > + > + /** > + * Check URL fragments against a specified table. Add a comment about the fragments should be generated by |LookupCache::GetLookupFragments| ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +177,5 @@ > > return NS_OK; > } > > void GetResults(nsTArray<RefPtr<nsIUrlClassifierFeatureResult>>& aResults) { Could you help add a comment about this function is used to convert the LookupResultArray from ::DoSingleLocalLookupWithURIFragments to nsIUrlClassifierFeatureResult

(In reply to Andrea Marchesini [:baku] from comment #27) > > I understand that we can skip searching URL if we found any match here > > because this is used by testcase only. > > I would like devtools to use this feature to enable/disable/classify custom > URLs. Would be a really great feature! Maybe we want something for > webextensions as well. So if this may not only be used by testcase in the future, should we keep matching the URL? For example, if devtools use the host preference to skip certain URL being annotated for some reason, url-classifier should still look for matches for other features, like fingerpriting, cryptomining ... etc. Does this make sense?

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/03a9dde1c3bc Cleanup URL-Classifier code - part 0 - use of StaticPrefs, r=dimi https://hg.mozilla.org/integration/mozilla-inbound/rev/d31813cff664 Cleanup URL-Classifier code - part 1 - nsIUrlClassifierFeature, r=dimi https://hg.mozilla.org/integration/mozilla-inbound/rev/1350c6351772 Cleanup URL-Classifier code - part 2 - nsUrlClassifierDBService::AsyncClassifyLocalWithFeatures, r=dimi https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc5e8d1a09d Cleanup URL-Classifier code - part 3 - AsyncUrlChannelClassifier, r=dimi https://hg.mozilla.org/integration/mozilla-inbound/rev/9480b74e612f Cleanup URL-Classifier code - part 4 - lookup optimization, r=dimi

> Does this make sense? Yes. So far we don't have a real use-case. We can keep the current implementation and fix/improve it when devtools will need more than what we currently have.

https://hg.mozilla.org/mozilla-central/rev/03a9dde1c3bc https://hg.mozilla.org/mozilla-central/rev/d31813cff664 https://hg.mozilla.org/mozilla-central/rev/1350c6351772 https://hg.mozilla.org/mozilla-central/rev/0cc5e8d1a09d https://hg.mozilla.org/mozilla-central/rev/9480b74e612f