690170 - (responsestatus) [meta] Fix channel management to avoid nsIHTTPChannel.responseStatus => NS_ERROR_NOT_AVAILABLE in Sync HTTP requests.

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

This is seeming to be a little widespread, so here's a tracking bug.

Comment 1

[Richard Newman [:rnewman]]

We've now seen this on all three platforms. Bug 693531 has trace logging.

Comment 2

[Richard Newman [:rnewman]]

Thanks to bz, I think we've found the culprit.

When an HTTP request involves a proxy authentication redirect, the resulting HTTP channel is not the same one we created.

The Resource/AsyncResource/RESTRequest code in Sync assumed that it was, so our onComplete handler looked at the wrong channel object.

Patch on the way.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 1: Fix channel management. v1

Part 1. This removes the channel attribute from Resource altogether, and ensures that RESTRequest updates it in the right places.

Part 2 will clean up some old workarounds etc. that're no longer necessary.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 2: remove old shim stuff.

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 570333 [details] [diff] [review]
Part 1: Fix channel management. v1

Epic find!

My criticism is limited to the tests, only. But they're a pretty important part of this fix...


>diff --git a/services/sync/tests/unit/test_proxy_auth_channel_change.js b/services/sync/tests/unit/test_proxy_auth_channel_change.js
>new file mode 100644
>--- /dev/null
>+++ b/services/sync/tests/unit/test_proxy_auth_channel_change.js

I would like to put these into test_resource, test_resource_async and test_restrequest. Mostly because of RESTRequest and it's uplift-ability into Toolkit (which we should continue to pursue).

>+/**
>+ * Fake a PAC to prompt a channel replacement.
>+ */
>+let systemSettings = {
>+  QueryInterface: function (iid) {
>+    if (iid.equals(Components.interfaces.nsISupports) ||
>+        iid.equals(Components.interfaces.nsIFactory) ||
>+        iid.equals(Components.interfaces.nsISystemProxySettings))
>+      return this;
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },

nsISupports is redundant. Also please use XPCOMUtils and Ci to keep the boilerplate at a minimum:

  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory, Ci.nsISystemProxySettings]),

>+  createInstance: function (outer, iid) {
>+    if (outer)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;

Nit: braces.

>+function installFakePAC() {
>+  const CID = Components.ID("{5645d2c1-d6d8-4091-b117-fe7ee4027db7}");
>+  const contractID = "@mozilla.org/system-proxy-settings;1"
>+
>+  Components.manager.nsIComponentRegistrar.registerFactory(
>+    CID,
>+    "Fake system proxy-settings",
>+    contractID, systemSettings);

This wrapping is somewhat inconsistent :)

>+add_test(function test_proxy_auth_redirect_resource() {
>+  let fetched = false;
>+  function original(req, resp) {

Most (all?) other tests that involve httpd.js use request, response. Consistency, please! :)

>+    fetched = true;
>+    let body = "TADA!";
>+    resp.setStatusLine(req.httpVersion, 200, "OK");
>+    resp.bodyOutputStream.write(body, body.length);
>+  }
>+
>+  let server = httpd_setup({
>+    "/original": original,
>+    "/pac1":     pacHandler
>+  });
>+  systemSettings.PACURI = "http://localhost:8080/pac1";

I find it interesting that nsISystemProxySettings defines this property as a string... I had to double check :)


r=me with those addressed

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 1: much uglier version, moving tests into individual files. v2

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 570363 [details] [diff] [review]
Part 1: much uglier version, moving tests into individual files. v2

r=me with IRC nits addressed :)

Comment 8

[Richard Newman [:rnewman]]

I feel like I should saber the top off a bottle of champagne.

Perhaps that should wait until TPS comes back with YEAAAHHH and TBPL turns green…

https://hg.mozilla.org/services/services-central/rev/89ade7d15e04
https://hg.mozilla.org/services/services-central/rev/1ddb0f2baa60

Comment 9

[Boris Zbarsky [:bzbarsky]]

The patch looks fine if the rest.js code does not look at this.channel before onStartRequest is called (e.g. does not try to cancel this.channel in response to something else).  But given that it does seem to do that (e.g. in abort() there), what you probably want to do is to actually register a redirect observer on the channel so you'll be notified when a redirect happens and can update this.channel.  You already register as the notification callbacks, so this should not be too bad.

The only comment I have other than that is that once you get the onStartRequest for some channel you are absolutely guaranteed that you will also get the onStopRequest for that same channel.  So the "update the channel" bit in onStopRequest is not needed.

Oh, one more thing.  Does the _onComplete call in abortRequest need to be updated?  If not, why not?

Comment 10

[Richard Newman [:rnewman]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> The patch looks fine if the rest.js code does not look at this.channel
> before onStartRequest is called (e.g. does not try to cancel this.channel in
> response to something else).

RESTRequest (in rest.js) sets this.channel in dispatch() (line 304), so this.channel is always set to an instance of nsIHttpChannel.

> But given that it does seem to do that (e.g.
> in abort() there), what you probably want to do is to actually register a
> redirect observer on the channel so you'll be notified when a redirect
> happens and can update this.channel.

Is it adequate to simply record the initial channel object (as mentioned above), and update it in onStartRequest? Or should we switch to using notifications for redirects?

Are there any situations other than redirects in which our initial channel can differ from the one supplied on onStopRequest?

Are all internal proxy auth redirects presented to the observer as redirects? (I.e., will we get a redirect notification whenever the channel is swapped out?)

> The only comment I have other than that is that once you get the
> onStartRequest for some channel you are absolutely guaranteed that you will
> also get the onStopRequest for that same channel.  So the "update the
> channel" bit in onStopRequest is not needed.

ORLY?

In which case, I'll file a follow-up bug after getting some clarity on my questions above. Lovely!

> Oh, one more thing.  Does the _onComplete call in abortRequest need to be
> updated?  If not, why not?

It does not, because the first argument to _onComplete will always be a truthy error object, and thus _onComplete will bail out by calling its callback before attempting to examine the channel argument.

(Essentially, providing `error` makes the other arguments optional.)

Thanks so much for the feedback and the insight!

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/89ade7d15e04
https://hg.mozilla.org/mozilla-central/rev/1ddb0f2baa60

Comment 12

[Richard Newman [:rnewman]]

Hey bz,

> In which case, I'll file a follow-up bug after getting some clarity on my
> questions above. Lovely!

Any input on these?

This got merged today, so I'd like to address any follow-ups in a timely fashion... and I'd like to be in a good place to fix this in Aurora and Beta, too, because we get one or more users a day finding us on Twitter to report this bug!

Thanks!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Gah.  I forgot to cc myself, hence didn't see comment 10.  Sorry about that.  :(

> Is it adequate to simply record the initial channel object (as mentioned above), and
> update it in onStartRequest? Or should we switch to using notifications for redirects?

If you want abort() to actually work reliably if called before onStartRequest, you need to observe redirect notifications.

> Are there any situations other than redirects in which our initial channel can differ
> from the one supplied on onStopRequest?

Assuming a properly functioning channel implementation, no.  Or more precisely, all such cases should be pretending to be a redirect.

> Are all internal proxy auth redirects presented to the observer as redirects?

Yes.  So are various other necko-internal things that have the effect of replacing the channel with a new one.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Gah.  I forgot to cc myself, hence didn't see comment 10.  Sorry about that.
> :(

No big deal. Thanks for the clarification!