1409534 - Replace `AsyncResource` with `fetch`

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler [:lina]]

Thom points out in bug 1409111, comment 1 that it's not currently safe to issue concurrent requests using `Resource`. It seems like `fetch` is the way forward, though we should heed Richard's caution in bug 1355677, comment 6 to make sure we get the headers, status codes, and character encodings right.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1409534 - Replace Resource impl by fetch.

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

Comment 2

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review202480

::: services/sync/modules/policies.js:1011
(Diff revision 1)
>          }
>          break;
>      }
>  
> -    switch (resp.result) {
> -      case Cr.NS_ERROR_UNKNOWN_HOST:
> +    // It's an exception!
> +    if (resp.message && resp.message.startsWith("NetworkError")) {

Note: we do lose some precision here.

::: services/sync/modules/resource.js
(Diff revision 1)
>     * be used instead of the global one.
>     */
>    authenticator: null,
>  
> -  // Wait 5 minutes before killing a request.
> -  ABORT_TIMEOUT: 300000,

If we really care about timeout, we should open a follow-up (AbortController is behind a pref/doesn't work yet)

::: services/sync/tests/unit/xpcshell.ini
(Diff revision 1)
>  [test_prefs_tracker.js]
>  [test_tab_engine.js]
>  [test_tab_store.js]
>  [test_tab_tracker.js]
>  
> -[test_warn_on_truncated_response.js]

If there's a content-length mismatch, fetch throws with AbortError.

Comment 3

[Edouard Oger [:eoger]]

Will also tag markh for review once he's back from PTO.

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review202582

This looks good and is a surprisingly small amount of code compared to what I would have guessed. Does TPS fully pass?

I'd also want to be sure we're handling other sorts of errors fetch might throw correctly, as well as double checking that timeouts aren't critical. I think we need to implement them manually at a minimum though, so that we can properly backoff and give the server room to breath, etc. I think this is the only major issue.

I'm clearing review just because of these unanswered questions, which I think we need to look into before we can land it. Also, as you mention, markh should take a look too.

::: services/sync/modules/policies.js:1011
(Diff revision 1)
>          }
>          break;
>      }
>  
> -    switch (resp.result) {
> -      case Cr.NS_ERROR_UNKNOWN_HOST:
> +    // It's an exception!
> +    if (resp.message && resp.message.startsWith("NetworkError")) {

We treat all these effectively the same anyway. I wouldn't sweat it. I think we should be sure we're handling the case somewhere that it's an error that doesn't start with "NetworkError" though. (At the very least, reporting it as an unexpectederror in telemetry).

::: services/sync/modules/resource.js
(Diff revision 1)
>    _logName: "Sync.Resource",
>  
> -  // ** {{{ Resource.serverTime }}} **
> -  //
> -  // Caches the latest server timestamp (X-Weave-Timestamp header).
> -  serverTime: null,

Oh right, yeah, this doesn't go here does it.

::: services/sync/modules/resource.js
(Diff revision 1)
>     * be used instead of the global one.
>     */
>    authenticator: null,
>  
> -  // Wait 5 minutes before killing a request.
> -  ABORT_TIMEOUT: 300000,

I think we should implement timeout on our end with setTimeout or similar in the meantime, to prevent syncs from taking many minutes.

(It's also possible that this isn't something we can live without. I think we should have telemetry how often requests fail with timeout.)

::: services/sync/modules/resource.js:165
(Diff revision 1)
> -      this._log.debug("Caught exception visiting headers in _onComplete", ex);
> -    }
>  
>      // Changes below need to be processed in bug 1295510 that's why eslint is ignored
>      // eslint-disable-next-line no-new-wrappers
> -    let ret     = new String(data);
> +    const ret   = new String(data);

This is awful too, but I guess we have a contributor working on it in bug 1295510.

::: services/sync/modules/resource.js:196
(Diff revision 1)
>    },
>  
> -  get() {
> -    return this._doRequest("GET", undefined);
> -  },
> -
> +  _logResponse(response, method, data) {
> +    const { status, ok: success, url } = response;
> +    this._log.trace(`Status: ${status}`);
> +    this._log.trace(`Success: ${success}`);

Mozreview's diff algorithm is very confused here.

::: services/sync/modules/resource.js:203
(Diff revision 1)
> -    this._log.trace("Channel for " + channel.requestMethod + " " + uri + ": " +
> -                    "isSuccessCode(" + status + ")? " + statusSuccess);
>  
> -    if (this._data == "") {
> -      this._data = null;
> +    // Additionally give the full response body when Trace logging.
> +    if (this._log.level <= Log.Level.Trace) {
> +      this._log.trace(`${method} body: ${data}`);

Nit: can you do this as `this._log.trace(\`${method} body:\`, data);` or similar? So we avoid the big format when trace logging is off, among other things.

::: services/sync/modules/resource.js:213
(Diff revision 1)
> -    this._onComplete = this._onProgress = null;
>    },
>  
> -  onDataAvailable: function Channel_onDataAvail(req, cb, stream, off, count) {
> -    let siStream;
> -    try {
> +  _processHeaders({ headers, ok: success }) {
> +    if (headers.has("X-Weave-Timestamp")) {
> +      Resource.serverTime = headers.get("X-Weave-Timestamp") - 0;

You didn't write it, but while you're here, could you use a `parseFloat()` (or parseInt, which would be consistent wiht the backoff handling below).

::: services/sync/tests/unit/xpcshell.ini
(Diff revision 1)
>  [test_prefs_tracker.js]
>  [test_tab_engine.js]
>  [test_tab_store.js]
>  [test_tab_tracker.js]
>  
> -[test_warn_on_truncated_response.js]

Do we have insight into how often this happens? I'm a bit worried for going from warning right to hard error without any data.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review202652

This looks fantastic, Ed! So much cruft removed. Mostly nits on the first pass; I expect Mark and Thom will have more substantive feedback.

::: services/sync/modules/policies.js:1011
(Diff revision 1)
>          }
>          break;
>      }
>  
> -    switch (resp.result) {
> -      case Cr.NS_ERROR_UNKNOWN_HOST:
> +    // It's an exception!
> +    if (resp.message && resp.message.startsWith("NetworkError")) {

Nit: Would `resp.name == "NetworkError"` work here? IIUC, we don't report network errors in the UI, so I think it's OK if we don't know what kind of error it is anymore.

::: services/sync/modules/resource.js
(Diff revision 1)
>     * be used instead of the global one.
>     */
>    authenticator: null,
>  
> -  // Wait 5 minutes before killing a request.
> -  ABORT_TIMEOUT: 300000,

Bug 1402317 suggests `AbortController` might be shipping in 58?

::: services/sync/modules/resource.js:52
(Diff revision 1)
> -  // ** {{{ Resource.headers }}} **
> -  //
>    // Headers to be included when making a request for the resource.
>    // Note: Header names should be all lower case, there's no explicit
>    // check for duplicates due to case!
>    get headers() {

Do we rely on this getter and setter now? If not, could we could make `this._headers` be a `Headers` object directly?

::: services/sync/modules/resource.js:106
(Diff revision 1)
> -      if ("undefined" != typeof(data))
> -        this._data = data;
> -
> -      // PUT and POST are treated differently because they have payload data.
> +    // PUT and POST are treated differently because they have payload data.
> -      if ("PUT" == action || "POST" == action) {
> -        // Convert non-string bodies into JSON
> +    if (("PUT" == method || "POST" == method) && !headers.has("content-type")) {
> +      headers.append("content-type", "text/plain");

Do we ever upload `text/plain`, or is it always JSON?

::: services/sync/modules/resource.js:131
(Diff revision 1)
> -        }
> -      });
> +   */
> +  _createRequest(method, data) {
> +    const headers = this._buildHeaders(method);
> +    const init = {
> +      cache: "no-store", // No cache.
> +      credentials: "omit", // No cookies.

I think `"omit"` is the default, right?

::: services/sync/modules/resource.js:142
(Diff revision 1)
> -      // clients without hurting performance.
> -      if (headers["x-weave-backoff"]) {
> -        let backoff = headers["x-weave-backoff"];
> -        this._log.debug("Got X-Weave-Backoff: " + backoff);
> -        Observers.notify("weave:service:backoff:interval",
> -                         parseInt(backoff, 10));
> +      if (typeof data !== "string") {
> +        data = JSON.stringify(data);
> +      }
> +      this._log.debug(`${method} Length: ${data.length}`);
> +      this._log.trace(`${method} Body: ${data}`);
> +      init.body = data

Missing semicolon, but `./mach eslint` should catch that.

::: services/sync/modules/resource.js:165
(Diff revision 1)
> -      this._log.debug("Caught exception visiting headers in _onComplete", ex);
> -    }
>  
>      // Changes below need to be processed in bug 1295510 that's why eslint is ignored
>      // eslint-disable-next-line no-new-wrappers
> -    let ret     = new String(data);
> +    const ret   = new String(data);

I can't wait to remove this eventually.

::: services/sync/modules/resource.js:213
(Diff revision 1)
> -    this._onComplete = this._onProgress = null;
>    },
>  
> -  onDataAvailable: function Channel_onDataAvail(req, cb, stream, off, count) {
> -    let siStream;
> -    try {
> +  _processHeaders({ headers, ok: success }) {
> +    if (headers.has("X-Weave-Timestamp")) {
> +      Resource.serverTime = headers.get("X-Weave-Timestamp") - 0;

Nit: While you're here, let's change `- 0` to `parseFloat(...)`.

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review202982

::: services/sync/modules/policies.js:1011
(Diff revision 1)
>          }
>          break;
>      }
>  
> -    switch (resp.result) {
> -      case Cr.NS_ERROR_UNKNOWN_HOST:
> +    // It's an exception!
> +    if (resp.message && resp.message.startsWith("NetworkError")) {

resp.name returns TypeError :/

::: services/sync/modules/resource.js
(Diff revision 1)
>     * be used instead of the global one.
>     */
>    authenticator: null,
>  
> -  // Wait 5 minutes before killing a request.
> -  ABORT_TIMEOUT: 300000,

Opened bug 1415682

::: services/sync/modules/resource.js:52
(Diff revision 1)
> -  // ** {{{ Resource.headers }}} **
> -  //
>    // Headers to be included when making a request for the resource.
>    // Note: Header names should be all lower case, there's no explicit
>    // check for duplicates due to case!
>    get headers() {

Yup we do in some tests, we should open a follow-up so we can remove these accessors entirely.

::: services/sync/modules/resource.js:106
(Diff revision 1)
> -      if ("undefined" != typeof(data))
> -        this._data = data;
> -
> -      // PUT and POST are treated differently because they have payload data.
> +    // PUT and POST are treated differently because they have payload data.
> -      if ("PUT" == action || "POST" == action) {
> -        // Convert non-string bodies into JSON
> +    if (("PUT" == method || "POST" == method) && !headers.has("content-type")) {
> +      headers.append("content-type", "text/plain");

It's a bit weird, this header is actually not set in postQueue, so we default to text/plain. However, looking at the network tab, we do send a content-type: application/json header, which I guess is infered by the network stack. We should investigate in a follow-up.

::: services/sync/tests/unit/xpcshell.ini
(Diff revision 1)
>  [test_prefs_tracker.js]
>  [test_tab_engine.js]
>  [test_tab_store.js]
>  [test_tab_tracker.js]
>  
> -[test_warn_on_truncated_response.js]

Sadly I don't think we have telemetry for that since it's only a warning :/

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

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

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review203094

Thanks! Feel free to drop my comments entirely, or handle them in a follow-up.

::: services/sync/modules/resource.js:137
(Diff revision 2)
> +      method
> +    };
>  
> -      // This is a server-side safety valve to allow slowing down
> -      // clients without hurting performance.
> -      if (headers["x-weave-backoff"]) {
> +    if (data) {
> +      if (typeof data !== "string") {
> +        data = JSON.stringify(data);

I'm guessing not, but, just in case...do we have any cases where where we might accidentally pass and double-stringify a `new String(...)` object?

::: services/sync/tests/unit/test_telemetry.js:496
(Diff revision 2)
>    } finally {
>      await cleanAndGo(engine, server);
>      Service.engineManager.unregister(engine);
>    }
>  });
>  

Should we also test with `AbortError`?

::: services/sync/tests/unit/test_telemetry.js:516
(Diff revision 2)
>        service: SYNC_FAILED_PARTIAL,
>        sync: LOGIN_FAILED_NETWORK_ERROR
>      });
>      let enginePing = ping.engines.find(e => e.name === "steam");
>      deepEqual(enginePing.failureReason, {
> -      name: "nserror",
> +      name: "unexpectederror",

Hmm, I wonder if we should use your `NetworkError` check and report these separately in telemetry, too...I imagine we'll see a lot of these.

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

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

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review203504

https://sql.telemetry.mozilla.org/queries/48904#131787 shows frequency of abort requests. It looks like 10k-20k per day, which isn't trivial, so, it's unclear to me if we care. (I was hoping it would be really small). I think maybe we should just emulate the timeout, and stop listening to the request after a bit if we can't use AbortController, and file a follow up for adding it. My concern is mostly syncs taking much longer, only to fail anyway.

I'll concede if someone has a compelling argument for why we don't care about this, though.

::: services/sync/modules/telemetry.js:715
(Diff revision 3)
>      if (error.result) {
>        return { name: "nserror", code: error.result };
>      }
> +
> +    if (error.message && error.message.startsWith("NetworkError")) {
> +      return { name: "networkerror", message: error.message };

I think we need to `cleanErrorMessage(error.message)` here since it's the message from a JS error and thus could contain PII.

::: services/sync/tests/unit/sync_ping_schema.json:166
(Diff revision 3)
>      },
> +    "networkError": {
> +      "required": ["name", "message"],
> +      "properties": {
> +        "name": { "enum": ["networkerror"] },
> +        "message": { "type": "string" }

The convention we try to use is to use the property name "code" for integer error codes and "error" for string errors.

Could you change this/the telemetry recording to follow that?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review203630

Sorry, tried to make a start here and didn't get far - but the errors worry me, so thought I'd post it anyway.

::: services/sync/modules/policies.js:1011
(Diff revision 3)
>          }
>          break;
>      }
>  
> -    switch (resp.result) {
> -      case Cr.NS_ERROR_UNKNOWN_HOST:
> +    // It's an exception!
> +    if (resp.message && resp.message.startsWith("NetworkError")) {

Is this the right way to detect these errors? https://fetch.spec.whatwg.org/#concept-network-error implies it may not be.

It's also a real shame IMO about this loss of error granularity - seeing in logs that NS_ERROR_UNKNOWN_HOST vs, say NS_ERROR_PROXY_CONNECTION_REFUSED, is super valuable IMO and something I'm really reluctant to give up. I wonder if there is some chrome-only way of getting more details here?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review205682

This looks great to me, but clearing review request as this patch will change with bug 1416842.

::: services/sync/modules/resource.js
(Diff revision 3)
>     * be used instead of the global one.
>     */
>    authenticator: null,
>  
> -  // Wait 5 minutes before killing a request.
> -  ABORT_TIMEOUT: 300000,

Bug 1402317 implies the AbortController is available in 58 without a pref.

::: services/sync/modules/resource.js:55
(Diff revision 3)
>    // Note: Header names should be all lower case, there's no explicit
>    // check for duplicates due to case!
>    get headers() {
>      return this._headers;
>    },
>    set headers(value) {

not directly related to this patch, but I think both |setHeader| and |set headers| is a foot gun and not actually used (except for a single test specifically for this function). How do you feel about changing the setter to throw?

::: services/sync/modules/resource.js:153
(Diff revision 3)
> -                         parseInt(headers["x-weave-quota-remaining"], 10));
> -      }
> +   * @param {string} [data] HTTP body
> +   * @returns {Response}
> +   */
> +  async _doRequest(method, data = null) {
> +    const request = this._createRequest(method, data);
> +    const response = await fetch(request);

might be worth a comment noting that this may throw on a network error? (it's fairly obvious, but probably can't hurt to call it out)

::: services/sync/modules/resource.js:210
(Diff revision 3)
> -                    ", HTTP success? " + channel.requestSucceeded);
> -    this._onComplete(null, this._data, channel);
> -    this._onComplete = this._onProgress = null;
>    },
>  
> -  onDataAvailable: function Channel_onDataAvail(req, cb, stream, off, count) {
> +  _processHeaders({ headers, ok: success }) {

I think calling this `_processResponseHeaders` might be clearer? (this is somewhat obvious too on reading the code, but it can't hurt to be clearer)

::: services/sync/modules/telemetry.js:715
(Diff revision 3)
>      if (error.result) {
>        return { name: "nserror", code: error.result };
>      }
> +
> +    if (error.message && error.message.startsWith("NetworkError")) {
> +      return { name: "networkerror", message: error.message };

yeah, this will hopefully become saner with bug 1416842

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review205682

> not directly related to this patch, but I think both |setHeader| and |set headers| is a foot gun and not actually used (except for a single test specifically for this function). How do you feel about changing the setter to throw?

Done, removing setHeader will be way more work and requires refactoring, let's do that in a follow-up!

Comment 14

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

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

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review206618

LGTM although I think the error semantics aren't quite correct yet (but I might also be missing something!)

::: services/sync/modules/resource.js:16
(Diff revision 4)
>  Cu.import("resource://services-common/utils.js");
> -Cu.import("resource://services-sync/constants.js");
>  Cu.import("resource://services-sync/util.js");
> +Cu.importGlobalProperties(["fetch"]);
> +/* global AbortController */
> +const {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {});

nit: I think all the Cu.imports should be together and the importGlobalProperties should come after.

::: services/sync/modules/resource.js:60
(Diff revision 4)
>    // Note: Header names should be all lower case, there's no explicit
>    // check for duplicates due to case!
>    get headers() {
>      return this._headers;
>    },
> -  set headers(value) {
> +  setHeader(header, value) {

Don't we want a setter that throws? I suspect trying to set "headers" might create an attribute?

::: services/sync/tests/unit/test_resource.js:498
(Diff revision 4)
>  add_task(async function test_timeout() {
>    _("Ensure channel timeouts are thrown appropriately.");
>    let res19 = new Resource(server.baseURI + "/json");
>    res19.ABORT_TIMEOUT = 0;
>    await Assert.rejects(res19.get(), error => {
> -    do_check_eq(error.result, Cr.NS_ERROR_NET_TIMEOUT);
> +    do_check_eq(error.name, "AbortError");

This is a semantic change - both policies.js and test_errorhandler_sync_checkServerError.js use this constant. I think for now we should just catch the AbortError and convert it to NS_ERROR_NET_TIMEOUT

But I'm missing where that translation actually takes place - I'm expecting something that converts the rejection with a raw nsresult into an object shaped like checkServerError expects?

Comment 16

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197306/diff/4-5/

Comment 17

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review207122

Looks great modulo nits/concerns.

::: services/sync/modules/resource.js:16
(Diff revision 5)
>  Cu.import("resource://services-common/utils.js");
> -Cu.import("resource://services-sync/constants.js");
>  Cu.import("resource://services-sync/util.js");
> +const {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {});
> +Cu.importGlobalProperties(["fetch"]);
> +/* global AbortController */

Surprised that we don't need to importGlobalProperties for this, but it hardly matters.

::: services/sync/modules/resource.js:61
(Diff revision 5)
>    // check for duplicates due to case!
>    get headers() {
>      return this._headers;
>    },
> -  set headers(value) {
> -    this._headers = value;
> +  set headers(_) {
> +    throw new Error("headers can't be mutated directly. Please use setHeader.");

Not ideal, since this won't fire if someone does `foo.headers['Bar-Baz'] = "quank"`. Or `Object.assign` onto the headers.

But, well, there's only so much you can do, and I'm not sure how much that matters anyway.

::: services/sync/modules/resource.js:145
(Diff revision 5)
> +      mozErrors: true // Return nsresult error codes instead of a generic
> +                      // NetworkError when fetch rejects.
> +    };
>  
> -      if (success && headers["x-weave-quota-remaining"]) {
> -        Observers.notify("weave:service:quota:remaining",
> +    if (data) {
> +      if (!(typeof data == "string" || data instanceof String)) {

Eugh. I guess in practice this will be a String object some of the time?

::: services/sync/modules/resource.js:238
(Diff revision 5)
> -    this._onComplete(null, this._data, channel);
> -    this._onComplete = this._onProgress = null;
>    },
>  
> -  onDataAvailable: function Channel_onDataAvail(req, cb, stream, off, count) {
> -    let siStream;
> +  _processResponseHeaders({ headers, ok: success }) {
> +    if (headers.has("X-Weave-Timestamp")) {

Small nit: I think it would be nice if we were consistent about the casing arguments we pass to `headers.has`/`get`/etc. e.g. all the others seem to be lower case (which makes sense given that we actually store them as lower case).

::: services/sync/tests/unit/test_resource.js:132
(Diff revision 5)
>  }
>  
>  function server_headers(metadata, response) {
>    let ignore_headers = ["host", "user-agent", "accept", "accept-language",
>                          "accept-encoding", "accept-charset", "keep-alive",
> -                        "connection", "pragma", "cache-control",
> +                        "connection", "pragma", "origin", "cache-control",

Did we start sending origin headers with this? If so, what is the listed origin in those cases?

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review207122

> Did we start sending origin headers with this? If so, what is the listed origin in those cases?

Null, I believe this will not cause problems for us.

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197306/diff/5-6/

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

https://reviewboard.mozilla.org/r/197306/#review207948

Awesome, thanks Ed!

::: services/sync/modules/resource.js:221
(Diff revision 6)
> -    this._deferred.resolve(ret);
> +    return ret;
>    },
>  
> -  get() {
> -    return this._doRequest("GET", undefined);
> -  },
> +  _logResponse(response, method, data) {
> +    const { status, ok: success, url } = response;
> +    this._log.trace(`Status: ${status}`);

I don't think we really need these 2 .trace lines - the .debug below has the same info.

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8926088 [details]
Bug 1409534 - Replace Resource impl by fetch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197306/diff/6-7/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d140f71aa8f4
Replace Resource impl by fetch. r=kitcambridge,markh,tcsc

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d140f71aa8f4