1367110 - Expose XMLHttpRequest error cases to chrome JS

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Chris H-C :chutten]

When Telemetry upload fails, we get a generic "error" message in TelemetrySend's XHR error handler[1].

The "error" comes from XMLHttpRequestMainThread's mErrorLoad which is set in four places: OnStartRequest, OnStopRequest, InitiateFetch, OnRedirectVerifyCallback.[2]

If we replace the bool with an enum and then expose it to chrome JS we can be more descriptive in reporting errors in cases like TelemetrySend's.

[1]: http://reports.telemetry.mozilla.org/post/projects/telemetry_send_failures.kp
[2]: http://searchfox.org/mozilla-central/search?q=mErrorLoad&path=

Comment 1

[Chris H-C :chutten]

Attachment: Bug 1367110 - Make XHRMainThread's mErrorLoad more descriptive.

There are at least four ways XHRMT can error on load.

Let's be specific about it.

Review commit: https://reviewboard.mozilla.org/r/142514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142514/

Comment 2

[Chris H-C :chutten]

Attachment: Bug 1367110 - Expose XHRMT's ErrorCode to chrome JS.

This is presently only relevant for XHRMT, so XHRWorker will just report that
everything's a-ok for now.

Review commit: https://reviewboard.mozilla.org/r/142516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142516/

Comment 3

[Chris H-C :chutten]

Attachment: bug 1367110 - Collect Telemetry for different TelemetrySend failures data-r=bsmedberg

Expires in 61 for now until we can show its usefulness.

Review commit: https://reviewboard.mozilla.org/r/142518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142518/

Comment 4

[Benjamin Smedberg]

Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

https://reviewboard.mozilla.org/r/142518/#review146242

data-r=me

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

https://reviewboard.mozilla.org/r/142518/#review146336

This looks good, thanks!

Do you think it would be useful to add Telemetry test coverage (xpcshell) for this probe?

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8870956 [details]
Bug 1367110 - Make XHRMainThread's mErrorLoad more descriptive.

https://reviewboard.mozilla.org/r/142514/#review146348

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8870957 [details]
Bug 1367110 - Expose XHRMT's ErrorCode to chrome JS.

https://reviewboard.mozilla.org/r/142516/#review146350

::: dom/webidl/XMLHttpRequest.webidl:159
(Diff revision 1)
>  
>    [ChromeOnly, Exposed=Window]
>    void setOriginAttributes(optional OriginAttributesDictionary originAttributes);
>  
> +  [ChromeOnly]
> +  readonly attribute unsigned short errorCode;

Write a comment saying:

1. this is for testing only.
2. this just works on main-thread.
3. when this will be removed.
4. file a bug for removing this in 1 cycle.
5. write the bug ID here.

Comment 8

[Andrea Marchesini [:baku]]

I don't think we want this in release, am I wrong? Can nightly be enough for the detection of this issue?

Comment 9

[Chris H-C :chutten]

It's for somewhat more than testing. It's for measuring the health of TelemetrySend over (hopefully) the long term.

I haven't yet filed the bug for 60 to examine whether to remove or extend the lifetime of the TELEMETRY_SEND_FAILURE_TYPE probe (and revert the errorCode changes), but when I do I can certainly include it in the comment. I expect it will prove to be sufficiently useful to warrant upgrading it to permanent.

Having this in release might be a useful addition at a later time, but for now the data will not be recorded there. (Though the attribute will be read)

I worry maybe my intentions were misunderstood when I approached you about this issue. I'm hoping to use this to identify the causes of telemetry ping send failures over time: to analyze trends and identify problems as they occur. If this is the wrong way to go about it, then I'll happily rework the patch to use a better long-term mechanism.

Comment 10

[Chris H-C :chutten]

Comment on attachment 8870956 [details]
Bug 1367110 - Make XHRMainThread's mErrorLoad more descriptive.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142514/diff/1-2/

Comment 11

[Chris H-C :chutten]

Comment on attachment 8870957 [details]
Bug 1367110 - Expose XHRMT's ErrorCode to chrome JS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142516/diff/1-2/

Comment 12

[Chris H-C :chutten]

Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142518/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9983ac05d7d1
Make XHRMainThread's mErrorLoad more descriptive. r=baku
https://hg.mozilla.org/integration/autoland/rev/1b93ec532890
Expose XHRMT's ErrorCode to chrome JS. r=baku
https://hg.mozilla.org/integration/autoland/rev/244e7cfea731
Collect Telemetry for different TelemetrySend failures r=bsmedberg,Dexter data-r=bsmedberg

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures in TelemetrySend.jsm (must use doublequotes):

https://hg.mozilla.org/integration/autoland/rev/1941e33040491f2ba9c435cf10747e9200901083
https://hg.mozilla.org/integration/autoland/rev/e30c63dcb366d2e8f89751d3962a9cbe50e58a2e
https://hg.mozilla.org/integration/autoland/rev/83f02ddde71f96da055af41fd10c945be89dde67

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=244e7cfea7317c67c322a2b4fba6c2b2e64feb34&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=103473826&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:98:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:99:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:100:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:101:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:102:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:1121:23 | Strings must use doublequote. (quotes)

Comment 15

[Chris H-C :chutten]

Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142518/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d632ed767b41
Make XHRMainThread's mErrorLoad more descriptive. r=baku
https://hg.mozilla.org/integration/autoland/rev/35d360dce251
Expose XHRMT's ErrorCode to chrome JS. r=baku
https://hg.mozilla.org/integration/autoland/rev/91a308b8cbbe
Collect Telemetry for different TelemetrySend failures r=bsmedberg,Dexter data-r=bsmedberg

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d632ed767b41
https://hg.mozilla.org/mozilla-central/rev/35d360dce251
https://hg.mozilla.org/mozilla-central/rev/91a308b8cbbe

Comment 18

[Chris H-C :chutten]

I will assume that all is well. We have now used these error codes to diagnose and fix issues in Telemetry.