Closed
Bug 1381718
Opened 7 years ago
Closed 7 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)
Toolkit
Safe Browsing
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
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Whiteboard: #sbv4-m8
Comment 1•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Summary: Dump the request when to check 400s response from server → Dump the request to check 400s response from server
Updated•7 years ago
|
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
Assignee | ||
Comment 2•7 years ago
|
||
400 response has not occurred these days (somehow server had an outage on July 16-17 and we got a lot of 400s).
Assignee | ||
Comment 3•7 years ago
|
||
(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
Assignee | ||
Comment 4•7 years ago
|
||
WIP : print headers and URL request for 400 update error
Comment 5•7 years ago
|
||
(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.
Updated•7 years ago
|
Assignee: nobody → tnguyen
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Hmn, looking at the log, then we could see the cause of 400 error code https://public-artifacts.taskcluster.net/RWMC4V47RQOIs9kpoeHyxA/0/public/logs/live_backing.log nsUrlClassifierStreamUpdater::OnStartRequest [request = https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$ct=application/x-protobuf&key=try-build-has-no-secrets&$httpMethod=POST&$req=ChUKE25hdmNsaWVudC1hdXRvLWZmb3gaCggFEAQiAiACKAEaCggBEAQiAiACKAEaCggDEAQiAiACKAEaCggHEAQiAiACKAEaCggJEAQiAiACKAE=] The key was "try-build-has-no-secrets" and server rejected the request.
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
mozreview-review |
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 10•7 years ago
|
||
mozreview-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-
Comment 11•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Thanks for your review.
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df78d081914e0afc094a138a5f6e20e72931bfc9
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/965f1bcd6d74 Dump request when receiving 400 error r=hchang
Keywords: checkin-needed
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/965f1bcd6d74
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•