Closed Bug 1332770 Opened 3 years ago Closed 3 years ago

The "google4" provider is showing up as "other" in some telemetry pings

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: francois, Assigned: dimi)

References

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → dlee
Status: NEW → ASSIGNED
The root cause is different for URLCLASSIFIER_UPDATE_ERROR & URLCLASSIFIER_UPDATE_REMOTE_STATUS2.

For URLCLASSIFIER_UPDATE_ERROR, we show google, mozilla, google4 and other. The "other" comes from update of test table[1], i think the solution here maybe add a "test" provider for test tables and ignore the results from test provider to be added to telemetry.

For URLCLASSIFIER_UPDATE_REMOTE_STATUS2, we show google, mozilla and other. The key used to record this telemetry is by converting mStreamTable to provider[2], and mStreamTable will only be set when UpdateUrlRequested is called[3]. So for v2 update, the status of first OnStartRequest of update will always be recorded to "other"(Since mStreamTable is empty), and then ProtocolParser got "u:redirect-url" and trigger another OnStartRequest for the real update url with correct provider recorded into telemetry. As for v4, there is no redirect, so every v4 update request is recorded to "other" category.

[1] https://dxr.mozilla.org/mozilla-beta/source/toolkit/components/url-classifier/SafeBrowsing.jsm#382
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#642
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#567
Blocks: 1332780
Attached patch WIP Patch (obsolete) — Splinter Review
No longer blocks: SafeBrowsingv4Telemetry
Attachment #8829792 - Attachment is obsolete: true
Comment on attachment 8830162 [details]
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

https://reviewboard.mozilla.org/r/107058/#review108458

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h:77
(Diff revision 1)
>    bool mIsUpdating;
>    bool mInitialized;
>    bool mDownloadError;
>    bool mBeganStream;
> +
> +  // Notice that mStreamTable is only used by v2, it is empty for v4 update.

nit: "Note that" instead of "Notice that"

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h:106
(Diff revision 1)
>    nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
>    nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
>    nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
> +
> +  // The provider for current update request.
> +  nsCString mProvider;

Maybe that should be called `mTelemetryProvider` to make sure we don't try to reuse it for something else (it would show up as "other" for any other providers that may be added by add-ons).

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:293
(Diff revision 1)
> +  nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
> +    do_GetService(NS_URLCLASSIFIERUTILS_CONTRACTID);
> +
> +  nsTArray<nsCString> tables;
> +  mozilla::safebrowsing::Classifier::SplitTables(aRequestTables, tables);
> +  urlUtil->GetTelemetryProvider(tables.SafeElementAt(0, EmptyCString()), mProvider);  

nit: whitespace at the end of the line

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:282
(Diff revision 1)
>  nsUrlClassifierUtils::GetProvider(const nsACString& aTableName,
>                                    nsACString& aProvider)
>  {
>    MutexAutoLock lock(mProviderDictLock);
>    nsCString* provider = nullptr;
> -  if (mProviderDict.Get(aTableName, &provider)) {
> +  if (StringBeginsWith(aTableName, NS_LITERAL_CSTRING("test-"))) {

Some of the test tables are starting to have something else between `test` and `-` so we should test for a string starting with just `"test"`.
Attachment #8830162 - Flags: review?(francois) → review-
Comment on attachment 8830162 [details]
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

https://reviewboard.mozilla.org/r/107058/#review109494
Attachment #8830162 - Flags: review?(francois) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e23905ef624
Fix the google4 provider is showing up as other in some telemetry pings. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e23905ef624
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.