Closed Bug 1381718 Opened 4 years ago Closed 4 years ago

Dump the full update or completion request when we receive a 400 from the Safe Browsing server

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: tnguyen, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m8)

Attachments

(1 file, 1 obsolete file)

Follow up bug 1374268 comment 25, we still see 400 error code from server
As discuss with Francois and Henry, we should dump the request a printf behind a MOZ_DUMP_SAFEBROWSING_ERRORS environment variable, for further investigation
Priority: -- → P1
Whiteboard: #sbv4-m8
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #0)
> As discuss with Francois and Henry, we should dump the request a printf
> behind a MOZ_DUMP_SAFEBROWSING_ERRORS environment variable, for further
> investigation

And also set that environment variable in https://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py of course.
Summary: Dump the request when to check 400s response from server → Dump the request to check 400s response from server
Summary: Dump the request to check 400s response from server → Dump the full update or completion request when we receive a 400 from the Safe Browsing server
400 response has not occurred these days (somehow server had an outage on July 16-17 and we got a lot of 400s).
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #2)
> 400 response has not occurred these days (somehow server had an outage on
> July 16-17 and we got a lot of 400s).

Still happens, on July 21
WIP : print headers and URL request for 400 update error
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #0)
> behind a MOZ_DUMP_SAFEBROWSING_ERRORS environment variable

Given that this is not likely to happen very often and that it would be very useful to be able to easily collect these errors from users as they happen, maybe we should not put this behind an environment variable and instead always print it to the terminal (via NS_WARNING or printf, whatever is most appropriate).

If it's too noisy in practice, we can always add the environment variable later.
Assignee: nobody → tnguyen
Comment on attachment 8889360 [details] [diff] [review]
0001-Bug-1381718-Dump-the-full-update-or-completion-reque.patch

I don't know whether it's still worth to push this debug dump to branch as we knew the problem. Probably we still need it to debug in the future.
Attachment #8889360 - Attachment is obsolete: true
Comment on attachment 8889768 [details]
Bug 1381718 : Dump request when receiving 400 error

https://reviewboard.mozilla.org/r/160848/#review166172

Looks good. We only need to make sure the usage of this patch. It would not be very useful for collecting data from try machines but might be useful for personal or local debugging. Thanks!

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:769
(Diff revision 1)
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS2").
>        add(this.telemetryProvider, httpStatusToBucket(httpStatus));
> +    // Bug 1381718 investigating 400 error code
> +    if (httpStatus == 400 && loggingEnabled) {

Do we need to check "loggingEnabled" here? If this patch is used to collect those failed request, we shouldn't expect 'browser.safebrowsing.debug' is enabled, shall we?
Attachment #8889768 - Flags: review?(hchang) → review+
Comment on attachment 8889768 [details]
Bug 1381718 : Dump request when receiving 400 error

https://reviewboard.mozilla.org/r/160848/#review166470

Let's change the error messages to explicitly mention the fact that we encountered a 400:

- "Safe Browsing server returned a 400 during update: request=%s"
- "Safe Browsing server returned a 400 during completion: request=%s"

Also, this can be done in a follow-up bug if it's going to delay this patch, but can we also dump the request payload when it's not a V4 server?

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:768
(Diff revision 1)
>      let success = Components.isSuccessCode(aStatusCode);
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS2").
>        add(this.telemetryProvider, httpStatusToBucket(httpStatus));
> +    // Bug 1381718 investigating 400 error code

nit: This comment is not necessary since the bug number will be easily accessible via blame if it's needed.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:769
(Diff revision 1)
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS2").
>        add(this.telemetryProvider, httpStatusToBucket(httpStatus));
> +    // Bug 1381718 investigating 400 error code
> +    if (httpStatus == 400 && loggingEnabled) {

I agree with Henry, we should always dump the URL, not just when logging is enabled.

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:769
(Diff revision 1)
>        uint32_t requestStatus;
>        rv = httpChannel->GetResponseStatus(&requestStatus);
>        NS_ENSURE_SUCCESS(rv, rv);
>        mozilla::Telemetry::Accumulate(mozilla::Telemetry::URLCLASSIFIER_UPDATE_REMOTE_STATUS2,
>                                       mTelemetryProvider, HTTPStatusToBucket(requestStatus));
> +      // Bug 1381718 investigating 400 error code

ditto
Attachment #8889768 - Flags: review-
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> I don't know whether it's still worth to push this debug dump to branch as
> we knew the problem. Probably we still need it to debug in the future.

I think it will be very useful to debug any future problems.
Thanks for your review.
See Also: → 1384405
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/965f1bcd6d74
Dump request when receiving 400 error r=hchang
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/965f1bcd6d74
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.