Closed Bug 690170 (responsestatus) Opened 9 years ago Closed 9 years ago

[meta] Fix channel management to avoid nsIHTTPChannel.responseStatus => NS_ERROR_NOT_AVAILABLE in Sync HTTP requests.

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Keywords: meta, Whiteboard: [fixed in services][qa-])

Attachments

(2 files, 1 obsolete file)

This is seeming to be a little widespread, so here's a tracking bug.
Alias: responsestatus
Summary: Tracking bug: NS_ERROR_NOT_AVAILABLE in nsIHttpChannel.responseStatus and nsIHttpChannel.visitResponseHeaders → [meta] NS_ERROR_NOT_AVAILABLE in nsIHttpChannel.responseStatus and nsIHttpChannel.visitResponseHeaders
We've now seen this on all three platforms. Bug 693531 has trace logging.
Depends on: 669176
Depends on: 669589
Depends on: 694676
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.
Summary: [meta] NS_ERROR_NOT_AVAILABLE in nsIHttpChannel.responseStatus and nsIHttpChannel.visitResponseHeaders → [meta] Fix channel management to avoid nsIHTTPChannel.responseStatus => NS_ERROR_NOT_AVAILABLE in Sync HTTP requests.
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #570333 - Flags: review?(philipp)
Attachment #570333 - Flags: feedback?(bzbarsky)
Attachment #570337 - Flags: review?(philipp)
Attachment #570337 - Flags: review?(philipp) → review+
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
Attachment #570333 - Flags: review?(philipp) → review+
Attachment #570333 - Attachment is obsolete: true
Attachment #570333 - Flags: feedback?(bzbarsky)
Attachment #570363 - Flags: review?(philipp)
Comment on attachment 570363 [details] [diff] [review]
Part 1: much uglier version, moving tests into individual files. v2

r=me with IRC nits addressed :)
Attachment #570363 - Flags: review?(philipp) → review+
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
Whiteboard: [fixed in services][qa-]
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?
(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!
https://hg.mozilla.org/mozilla-central/rev/89ade7d15e04
https://hg.mozilla.org/mozilla-central/rev/1ddb0f2baa60
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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!
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.
(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!
Duplicate of this bug: 698901
Depends on: 705279
Depends on: 718043
Blocks: 704539
Duplicate of this bug: 698305
Duplicate of this bug: 714471
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.