546606 - Make redirect API async - part 2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bjarne (:bjarne)]

This is the remainder of bug #513086, which is to make the method nsIChannelEventSink::OnChannelRedirect asynchronous.

Comment 1

[Bjarne (:bjarne)]

For now it relies on the patch for bug #513086 for implementing the proper callstack in nsHttpChannel when the call to nsHttpHandler::OnChannelRedirect becomes async.

Two approaches have been suggested :

1) Add a new interface (e.g. nsChannelEventSinkAsync) and make nsHttpHandler/nsIOSevice QI the sink to determine if it can be used (Honza)

2) Change the existing nsChannelEventSink::OnChannelRedirect to be async (bsmedberg)

(1) has the advantage of not touching existing sinks, and only sinks who wish to implement async behaviour need to be changed. The penalty is one more public interface and somewhat more complex code in nsHttpHandler/nsIOSevice.

(2) avoids the penalties of (1). Changes to existing sinks are simple, and there are not too may of them. Penalty is that we change an interface which may be used by 3rd parties, but according to bsmedberg this is not a big issue.

Approach (2) has been taken, but it is not difficult to switch to (1) if this is preferred at some point.

Comment 2

[Honza Bambas (:mayhemer)]

When going the path of (1) shouldn't we also remove the sync method?  I don't see a purpose of it, because the async method may actually return the result synchronously as well, by calling the callback from inside the method, right?  And it's even less work to do.

Comment 3

[Bjarne (:bjarne)]

Attachment: WIP - input requested

This is current thinking. Note that it builds on the patch for bug #513086. A qfold of these patches has been pushed to tryserver...

The idea is to call nsChannelEventSink::OnChannelRedirect() via a delegator (or "helper-class"). The delegator acts as a semaphore doing the book-keeping to ensure we have same number of async calls and callbacks. The delegator is also the callback-object for async sinks.

The callback to nsHttpChannel can happen from the delegator in three ways :

1) If a call to nsChannelEventSink::OnChannelRedirect() returns failure
2) If a veto (error-code) is returned via callback from sink to delegator
3) If all calls to nsChannelEventSink::OnChannelRedirect() are matched by callbacks to the delegator with NS_OK

In current impl, callbacks to nsHttpChannel in case (1) happens from nsAsyncRedirectVerifyHelper::Run() which I believe is ok.

In cases (2) and (3), current impl makes the callback to nsHttpChannel synchronously in the callback to the delegator - I assume this is NOT ok and that it should be done async on main-thread, right?

I'd be interested in comments to the approach in general, but in particular to the changed contract for nsChannelEventSink::OnChannelRedirect() which I (attempt to) explain in nsChannelEvenSink.idl

Comment 4

[Boris Zbarsky [:bzbarsky]]

fwiw, I prefer approach #2 from comment 1.  Seems like it would lead to simpler code, and we don't really want to support sync observers long-term do we?

Comment 5

[Bjarne (:bjarne)]

The supplied patch takes approach #2. The only reason for approach #1 would be to maintain backwards-compatibility for 3rd party code, which (according to bsmedberg) is not particularly useful since the APIs change between major releases in any case. If a sink wants to be sync it just does the callback from within OnChannelRedirect, or simply returns an error-code if that is the decision (no callback to the delegator is needed in this case).

Comment 6

[Bjarne (:bjarne)]

Attachment: WIP2 - input requested

Just realized that it may be more illustrative to only upload the diffs relative to Honzas patch for bug# 513086....  :}

The combined patch seems to pass tryserver with the "normal exceptions" these days. However, none of the sinks are async yet so the test is just half-way there.

Next steps are

1) pick one or two sinks and make their OnChannelRedirect async (suggestions for which sinks to chose would be appreciated!)

2) avoid doing a sync callback to nsHttpChannel from the sink (see question in comment #3 paragraph 7)

Comment 7

[Honza Bambas (:mayhemer)]

For example, in nsCrossSiteListenerProxy the method must not continue after rv = outer->OnChannelRedirect until it knows if the redirect has not been canceled, right?  Then, in that particular case, using the "delegator" is not appropriate.  You must split the nsCrossSiteListenerProxy::OnChannelRedirect after outer->OnChannelRedirect and actually do:

outer->OnChannelRedirect(old, new, flags, *this*)

and let nsCrossSiteListenerProxy implement the "delegator" interface as well.  In method nsCrossSiteListenerProxy::OnChannelRedirectResponse continue with the code of nsCrossSiteListenerProxy::OnChannelRedirect (from after rv = UpdateChannel(aNewChannel) line)

Then, how should nsCrossSiteListenerProxy implement the DelegateOnChannelRedirect method?

The same applies to nsXMLHttpRequest::OnChannelRedirect, imgRequest::OnChannelRedirect and nsIncrementalDownload::OnChannelRedirect.

nsIAsyncVerifyRedirectCallback.onRedirectVerifyCallback and nsIChannelEventSinkDelegator.onChannelRedirectResponse are redundant.

You actually need the "delegator" only to call everything in nsIOService::OnChannelRedirect in parallel as well with the per-channel sink.

So, my suggestion: as nsIOService::OnChannelRedirect is a public method and not an interface one, you can pass the delegator to it and use it the way you do in the patch.  Then, let the delegator implement nsIAsyncVerifyRedirectCallback instead of nsIChannelEventSinkDelegator, then you don't need any new interface.  Personally, I strongly recommend to merge your delegator and the helper to a single class, it has the same lifetime and the same purpose.

BTW, "delegator" seems not to be a word.

Comment 8

[Bjarne (:bjarne)]

(In reply to comment #7)
> For example, in nsCrossSiteListenerProxy the method must not continue after rv
> = outer->OnChannelRedirect until it knows if the redirect has not been
> canceled, right?

Ahh - yes. Some of the sinks have side-effects which should be avoided... good catch!

I'll have to dig into this again. Thanks for input and suggestions!

> BTW, "delegator" seems not to be a word.

Yes - "delegate" is the word I meant to use... :)

Comment 9

[Bjarne (:bjarne)]

Attachment: Version 1.0 - requesting "informal review"

Updated to address Honzas comments. Works nicely when randomly browsing local newspapers for some minutes (a few assertions related to layout, no leaks). Qfold with Honzas patch from bug #513086 is pushed to tryserver (for a more systematic and formal test).

I would also like to make at least one of the sinks truly asynchronous - any good candidates?

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 434235 [details] [diff] [review]
Version 1.0 - requesting "informal review"

We should decide on a form of a callback calling and keep it, I vote for:

OnChannelRedirect(..)
{
  rv = DoStuff();
  
  cb->OnRedirectVerifyCallback(rv);
  
  return NS_OK;
}

OnRedirectVerifyCallback should always succeed.  If it doesn't then the implementation might be broken; we might consider adding an NS_WARNING() log when it fails.  However, failure of OnRedirectVerifyCallback must NOT cause OnChannelRedirect to fail, because we actually called the callback.  'return cb->OnRedirectVerifyCallback(rv);' breaks the contract.

Feel free to oppose me.


In general, try not to add NS_ENSURE_SUCCESS where it was not before.  The programmer might want to avoid error logs, i.e. the failure is not fatal but part of a logic.  And also do not remove it where it was before.


> a/content/base/src/nsCrossSiteListenerProxy.cpp
> nsCrossSiteListenerProxy::OnChannelRedir 	 
> +  if (NS_FAILED(result)) {
> +    mOldChannel->Cancel(result);
> +    mCallback->OnRedirectVerifyCallback(result);
> +  } else {
> +    mCallback->OnRedirectVerifyCallback(UpdateChannel(mNewChannel));

Probably change this to:

  if (NS_SUCCEEDED(result)) {
    result = UpdateChannel(...);
  }
  
  if (NS_FAILED(result)) {
    mOldChannel->Cancel(result);
    NS_WARNING("...");
  }
  
  m* = nsnull; // do it before callback !
  mCallback->OnRedirectVerifyCallback(result);


You are not canceling the old channel when UpdateChannel fails and mCallback->OnRedirectVerifyCallback(UpdateChannel(mNewChannel)); in a single line is quit ugly.

> a/content/base/src/nsCrossSiteListenerProxy.h
> nsCOMPtr<nsIAsyncVerifyRedirectCallback> mCallback;

Shouldn't be named mRedirectCallback?  This is too generic.  The same for old and new channel, generally in the whole patch.

> netwerk/base/public/nsAsyncRedirectVerifyHelper.h
> #include <nsIAsyncVerifyRedirectCallback.h>

The convention is to use "" for includes.

> netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp

You probably don't need class nsAsyncVerifyRedirectCallbackEvent

> nsAsyncRedirectVerifyHelper::OnRedirectVerifyCallback

The code is a bit redundant, if you have ideas how to simplify this method, it would be good.


I would love to have (mochi)tests at the start for at least one of the consumers and then for the first consumer made really asynchronous.

Comment 11

[Honza Bambas (:mayhemer)]

As I read the patch and thinking of it, I probably start to understand you original approach with DelegateOnChannelRedirect method of the Dispatcher.  The contract is quit on-edge to get broken and the whole redirect API would break with it making tracing the culprit very hard.  It might be cool to provide an API (similar to your DelegateOnChannelRedirect method in the prevoius patch) used when callbacks are chained (as in nsCrossSiteListenerProxy etc cases) that would wrap the callback with a tiny helper class that would ensure that the original callback is called exactly ones only and only if the OnChannelRedirect method returns NS_OK and potentially ignored when it fails.  This would keep the core redirect callback counter consistent and it would be potentially simple to trace for a broken implementation.  The tiny class could also keep reference to old and new channels to free the client of additional members and also prevent it from being broken on potential re-entrance of OnChannelRedirect before it gets OnRedirectVerifyCallback.  But this is just a crazy idea for the future.

Comment 12

[Bjarne (:bjarne)]

Attachment: V 1.1 - comments requested

With Honzas patch from bug #513086, comment #41 this one also starts to behave reasonably. A few open questions about xpconnect though :

Some sinks implement nsIAsyncVerifyRedirectCallback, passing themselves as the callback to chained sinks. I had to extend the DOM_CLASSINFO_MAP entry in nsDOMClassInfo.cpp for XMLHttpRequest because some tests use JS implementations of nsIChannelEventSink, and xpconnect doesn't manage to match the OnChannelRedirect() method-signature otherwise. The sinks in question are : nsXMLHttpRequest, imgRequest, nsCrossSiteListenerProxy and nsIncrementalDownload.

1) Should we extend/add DOM_CLASSINFO_MAP entry in nsDOMClassInfo.cpp for all sinks implementing nsIAsyncVerifyRedirectCallback as a precaution, or should we just wait and implement it if if/when needed? Put differently : Is it likely that sinks other than nsXMLHttpRequest will call JS-sinks as part of their OnChannelRedirect call-chain?

2) What about explicit QueryInterface and/or GetInterface implementations for the mentioned classes? Should we maintain these (see e.g. imgRequest::GetInterface()) ?

(In reply to comment #10)
> (From update of attachment 434235 [details] [diff] [review])
> We should decide on a form of a callback calling and keep it, I vote for:
> 
> OnChannelRedirect(..)
> {
>   rv = DoStuff();
> 
>   cb->OnRedirectVerifyCallback(rv);
> 
>   return NS_OK;
> }
> 
> OnRedirectVerifyCallback should always succeed.  If it doesn't then the
> implementation might be broken; we might consider adding an NS_WARNING() log
> when it fails.  However, failure of OnRedirectVerifyCallback must NOT cause
> OnChannelRedirect to fail, because we actually called the callback.  'return
> cb->OnRedirectVerifyCallback(rv);' breaks the contract.

Agreed about the contract and agreed that this is a nice pattern in general. Tried to improve it here...

> In general, try not to add NS_ENSURE_SUCCESS where it was not before.  The
> programmer might want to avoid error logs, i.e. the failure is not fatal but
> part of a logic.  And also do not remove it where it was before.

Yup. Hopefully better now.

> > a/content/base/src/nsCrossSiteListenerProxy.cpp
> > nsCrossSiteListenerProxy::OnChannelRedir 	 
> > +  if (NS_FAILED(result)) {
> > +    mOldChannel->Cancel(result);
> > +    mCallback->OnRedirectVerifyCallback(result);
> > +  } else {
> > +    mCallback->OnRedirectVerifyCallback(UpdateChannel(mNewChannel));
> 
> Probably change this to:
> 
>   if (NS_SUCCEEDED(result)) {
>     result = UpdateChannel(...);
>   }
> 
>   if (NS_FAILED(result)) {
>     mOldChannel->Cancel(result);
>     NS_WARNING("...");
>   }
> 
>   m* = nsnull; // do it before callback !
>   mCallback->OnRedirectVerifyCallback(result);
> 
> 
> You are not canceling the old channel when UpdateChannel fails and
> mCallback->OnRedirectVerifyCallback(UpdateChannel(mNewChannel)); in a single
> line is quit ugly.

Nice catch - fixed. :)

> > a/content/base/src/nsCrossSiteListenerProxy.h
> > nsCOMPtr<nsIAsyncVerifyRedirectCallback> mCallback;
> 
> Shouldn't be named mRedirectCallback?  This is too generic.  The same for old
> and new channel, generally in the whole patch.

Yup. Better now, I hope.

> The convention is to use "" for includes.

Ok.

> > netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp
> 
> You probably don't need class nsAsyncVerifyRedirectCallbackEvent

Problem of requesting feedback on WIP... :)  It was a placeholder - it hopefully does some useful stuff now.

> > nsAsyncRedirectVerifyHelper::OnRedirectVerifyCallback
> 
> The code is a bit redundant, if you have ideas how to simplify this method, it
> would be good.

Not sure what you mean - suggestions and/or more concrete issues are welcome.

> I would love to have (mochi)tests at the start for at least one of the
> consumers and then for the first consumer made really asynchronous.

Me too...  :)

I'll see what I can do. Passed this to tryserver in the meantime...

Comment 13

[Bjarne (:bjarne)]

It suddenly struck me that it might be simpler and clearer if we let OnChannelRedirect() return some special value (NS_ASYNC_PROCESSING or similar) when it wants to process a request asynchronously. Returning NS_OK would then mean the same as it currently does ("no objections to redirect"), allowing us to simplify changes to existing sinks.

Input?

Comment 14

[Honza Bambas (:mayhemer)]

Hmm... I'd say it makes the API even more complicated putting more effort to the programmer.  What benefit would that bring?

Comment 15

[Bjarne (:bjarne)]

(In reply to comment #14)
> Passed this to tryserver in the meantime...

Tryserver is unhappy with the browser_privatebrowsing_beforeunload.js as mentioned in bug #513086, comment #41. Seems to be ok otherwise.

(In reply to comment #14)
> Hmm... I'd say it makes the API even more complicated putting more effort to
> the programmer.  What benefit would that bring?

We would avoid touching the code of most existing sinks. The exceptions are sinks who call other sinks, but these must be changed significantly in any case.

It would actually make the patch a lot simpler, reduce the potential for introducing regressions and it would force sinks wanting to be asynchronous to explicitly tell us so. Return-values of OnChannelRedirect() would be

NS_OK : sink doesn't object to the redirect

NS_ASYNC_PROCESSING (or similar) : sink wants to process request asynchronously and guarantees that a callback will be performed

Anything else : redirect is vetoed by the sink

Note that synchronous sinks will just work as before. I can come up with a patch following this approach in a while...

Comment 16

[Benjamin Smedberg]

I think we should force the API consumers to change: I think it will just introduce more confusion to have an API which can be used synchronously or asynchronously, and it requires mucking with Components.returnCode in JavaScript, which is ugly.

Comment 17

[Bjarne (:bjarne)]

Seriously? We don't believe API consumers will grasp the idea that returning one particular value means "give decision in a callback later", otherwise the returned value is the decision (like before)?

Comment 18

[Honza Bambas (:mayhemer)]

Something deep back in my brain tells me that what you suggest is not a good approach.  IMO it makes the API only more complicated and rises chance for regressions and confusions in the future.  The contract is complicated even now.  What you are trying seems to be just simplifying your patch.

Comment 19

[Benjamin Smedberg]

Alternate success code are difficult to use in script (where you're not "returning" those codes), and confusing for people in C++.

Comment 20

[Bjarne (:bjarne)]

(In reply to comment #18)
> Something deep back in my brain tells me that what you suggest is not a good
> approach. 

I have full respect for such hunches - believe me! However, life will be simpler for sinks with the suggested approach (based on experience of re-writing 20-25 sinks for this patch).

> What you are trying seems to be just simplifying your patch.

That will be a side-effect, yes. Isn't that nice...?  :)

The issue with the current approach is really that it redefines the meaning of returning NS_OK from the sink. Thus, we force innocent, simple sinks which make their OK-decision fast and synchronously to make callbacks. IMO, we should keep the current meaning of returning NS_OK as well as general error-values, but somehow let a sink inform us that it will report its decision asynchronously. My suggestion is to use a special return-value for this, another suggestion at some point was to use a new interface to signal this (see comment #1, approach 1).

(In reply to comment #16)
> I think we should force the API consumers to change

Signature of OnChannelRedirect() has changed, hence all users of the API will have to revise and maybe reconsider their implementations. I don't see, however, why we should force them to rewrite their code in all cases by redefining the meaning of NS_OK.

(In reply to comment #19)
> Alternate success code are difficult to use in script (where you're not
> "returning" those codes), and confusing for people in C++.

I'm not going to insist on the new approach. But I know (from experience of re-writing a number of sinks as part of the patch) that it will be simpler for sinks. Whether it will be simpler for the caller I don't know, but that can be figured out by implementing the approach.

Now, if the decision is to reject the suggested approach I think we should purify the API in "the opposite direction" by requiring sinks to always do the callback even for vetos. A sink returning anything else than NS_OK should be ignored by the caller and the caller expects callbacks from all the others.

IMO, the current patch implements an undesirable hybrid : We should either make it "pure and simple" like mentioned above or we should make it "rich and complex" like suggested in comment #13.

Comment 21

[Bjarne (:bjarne)]

FYI, I've implemented and am currently testing the "pure and simple" API from above.

Comment 22

[Johnny Stenback (:jst)]

(In reply to comment #12)
> Some sinks implement nsIAsyncVerifyRedirectCallback, passing themselves as the
> callback to chained sinks. I had to extend the DOM_CLASSINFO_MAP entry in
> nsDOMClassInfo.cpp for XMLHttpRequest because some tests use JS implementations
> of nsIChannelEventSink, and xpconnect doesn't manage to match the
> OnChannelRedirect() method-signature otherwise. The sinks in question are :
> nsXMLHttpRequest, imgRequest, nsCrossSiteListenerProxy and
> nsIncrementalDownload.

I'm afraid I don't understand how adding anything to XHR's DOM_CLASSINFO_MAP entry helps here, but either way, I really don't think that's the right answer here. Seems like if JS is trying to call OnChannelRedirect() (or onChannelRedirect() in JS) and that method isn't exposed on whatever it's called on, then there's a missing QI. Maybe the type changed someplace, and what used to be automatically exposed to JS as an nsIChannelEventSink is now exposed as an nsIAsyncVerifyRedirectCallback? In that case a simple QI in JS should be all that's needed to get to the method you want.

> 1) Should we extend/add DOM_CLASSINFO_MAP entry in nsDOMClassInfo.cpp for all
> sinks implementing nsIAsyncVerifyRedirectCallback as a precaution, or should we
> just wait and implement it if if/when needed? Put differently : Is it likely
> that sinks other than nsXMLHttpRequest will call JS-sinks as part of their
> OnChannelRedirect call-chain?

I think the answer here is no, we shouldn't need to touch classinfo stuff here at all.

> 2) What about explicit QueryInterface and/or GetInterface implementations for
> the mentioned classes? Should we maintain these (see e.g.
> imgRequest::GetInterface()) ?

I think yes.

Let me know if I'm misunderstanding something obvious here...

Comment 23

[Bjarne (:bjarne)]

Mmmmm...  I'll try to explain my understanding of this.

After some time in the debugger I figured out that the call to the chained sink in nsXMLHttpRequest::OnChannelRedirect fails in a certain case (nsXMLHttpRequest.cpp:3057 in the patch) 

rv = sink->OnChannelRedirect(aOldChannel, aNewChannel, aFlags, this);

|sink| in the failing case is a JS-implementation of nsIChannelEventSink (BadCertHandler from CertUtils.jsm, if memory serves me right). I.e. this is C++ calling a JS-object.

Some more debugging reveals that xpcwrappedjsclass::CallMethod() fails when preparing the 4th parameter for (the new version of) nsIChannelEventSink::OnChannelRedirect (4th parameter is the new interface nsIAsyncVerifyRedirectCallback). sink->OnChannelRedirect() is in fact never called and |rv| is set to NS_ERROR_FAILURE.

The way I understand it, xpcwrappedjsclass::CallMethod() fails when trying to verify that |this| implements nsIAsyncVerifyRedirectCallback. After failing this, it skips calling the requested method and returns the default value of NS_ERROR_FAILURE. |this| is an instance of nsXMLHttpRequest, and the way I made it work was to add nsIAsyncVerifyRedirectCallback to XHR's DOM_CLASSINFO_MAP. I'd be happy to do it in a more recommended way, if any.

Now, there are four sinks which implements nsIAsyncVerifyRedirectCallback : Q1 above is about whether we should apply the same mechanism for all these or rather wait and see if it becomes a problem.

Q2 above got a clear answer...  I'll get on with it on Monday. :)

Comment 24

[Bjarne (:bjarne)]

Wrapping up the work from Q2 above seems to eliminate the need to change XHR's DOM_CLASSINFO_MAP. Wonder why that didn't work for me before, but ok...

Need some local testing before sending to tryserver.

Comment 25

[Johnny Stenback (:jst)]

(In reply to comment #24)
> Wrapping up the work from Q2 above seems to eliminate the need to change XHR's
> DOM_CLASSINFO_MAP. Wonder why that didn't work for me before, but ok...

Sounds good :)

> Need some local testing before sending to tryserver.

Comment 26

[Bjarne (:bjarne)]

Attachment: V1.1 unbitrotted w/ some bugfixes

This version passes basic local testing. Pushed to tryserver and review requested.

As always, comments are highly appreciated. In particular, comments to the new description of nsIChannelEventSink might be useful since it implements the new approach to the API as described in comment #20, last two paragraphs.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 446819 [details] [diff] [review]
V1.1 unbitrotted w/ some bugfixes

My first comment is, please rename OnChannelRedirect to AsyncOnChannelRedirect, to make it clearer that a response is expected asynchronously. It might also make it more obvious to JS implementations that they have to change their code.

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Hmm, although... I see that you do allow synchronous callbacks. Maybe better to leave it as-is then...

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to comment #28)
> Hmm, although... I see that you do allow synchronous callbacks. Maybe better to
> leave it as-is then...

We had this discussion already.  It is simpler to make the API straightforwardly simple: always use callback to give an answer.  Read comment 13 and follow ups.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 446819 [details] [diff] [review]
V1.1 unbitrotted w/ some bugfixes

Two general comments:
- please add a test that makes sure that async cancelling of a redirect works
- it is quite unfortunate that JS implementations of nsIChannelEventSink will silently do the wrong thing with this patch :/

+++ b/netwerk/base/public/nsIChannelEventSink.idl	Sat May 22 00:03:11 2010 +0200
+     * There is a certain freedom in implementing this method :

please delete the space before the colon, also below ("Repeat :")

+++ b/content/base/src/nsCSPService.cpp	Sat May 22 00:03:11 2010 +0200
                               nsIChannel *newChannel,
-                              PRUint32   flags)
+                              PRUint32   flags,
+                              nsIAsyncVerifyRedirectCallback *callback)

now that the arguments aren't aligned anymore anyway, just use one space before 'flags'. Same in nsContentUtils.cpp, nsCrossSiteListenerProxy.cpp and nsSyncLoadService.cpp

+++ b/content/base/src/nsContentUtils.cpp	Sat May 22 00:03:11 2010 +0200
+  if (!(oldPrincipal && newURI && newOriginalURI)) {
+      NS_WARNING(("nsSameOriginChecker::OnChannelRedirect : bad state"));

please use two-space indentation, like the rest of the file

also, no space before colon

+++ b/content/base/src/nsCrossSiteListenerProxy.cpp	Sat May 22 00:03:11 2010 +0200
+      aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI); // is this is necessary..?

I suspect it isn't, but ask the author of that code?

+          NS_WARNING("nsCrossSiteListenerProxy::OnRedirectVerifyCallback : "
+                     "UpdateChannel() returned failure");

please put {} around this code

+++ b/content/base/src/nsXMLHttpRequest.cpp	Sat May 22 00:03:11 2010 +0200
+        NS_WARNING("nsXMLHttpRequest::OnChannelRedirect : "

no space before colon, please

-    }
+      rv = mChannelEventSink->OnChannelRedirect(aOldChannel, aNewChannel,

Please stay with two-space indentation

+++ b/content/html/content/src/nsHTMLMediaElement.cpp	Sat May 22 00:03:11 2010 +0200
NS_IMETHODIMP nsHTMLMediaElement::MediaLoadListener::OnChannelRedirect(nsIChannel* aOldChannel,
                                                                        nsIChannel* aNewChannel,
-                                                                       PRUint32 aFlags)
+                                                                       PRUint32 aFlags,
+                                                                       nsIAsyncVerifyRedirectCallback *cb)

please be consistent with the placement of the *

+++ b/content/media/nsMediaStream.cpp	Sat May 22 00:03:11 2010 +0200

same

+++ b/docshell/base/nsDocShell.cpp	Sat May 22 00:03:11 2010 +0200
+    NS_WARNING(("nsPingListener::OnChannelRedirect : bad state"));

no space before colon

+      NS_WARNING("nsClassifierCallback::OnChannelRedirect : "

same

+++ b/netwerk/base/public/nsAsyncRedirectVerifyHelper.h	Sat May 22 00:03:11 2010 +0200

Can you please add comments to the functions you're adding? It's not always obvious from the name what they are doing.

+    virtual ~nsAsyncRedirectVerifyHelper();

you could make this nonvirtual (and private) so save some codesize and to get slightly better performance

+    nsAsyncRedirectAutoCallback(nsIAsyncVerifyRedirectCallback* aCallback)
+    : mCallback(aCallback)
+  {
+      mResult = NS_OK;

why not initialize mResult in the initializer list as well?

+++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp	Sat May 22 00:03:11 2010 +0200
+        LOG(("nsAsyncVerifyRedirectCallbackEvent::Run() "
+             "callback to %p with result %u",

usually %x is used for nsresults

+    : mCallbackThread(nsnull),

No need to explicitly initialize nsCOMPtrs

+    if (mCallbackThread)
+        mCallbackThread = nsnull;

No need to do that in a destructor

+    if (NS_FAILED(result)) {
+
+        // We chose to store the first failure-value (as opposed to the last)

No need for this empty line

+         "result=%u expectedCBs=%u mResult=%u",

again, the results should use %x

+        // return. Otherwise the callback will be invoked from InitCallback()
+        // because mResult is set to failure

But the callback will be called even if mResult is success...

+    if (NS_FAILED(rv)) --mExpectedCallbacks;

please put the -- on a new line

+nsAsyncRedirectVerifyHelper::GetInterface(const nsIID & aIID, void **aResult)

What do you need this for?

+        gIOService->OnChannelRedirect(mOldChan, mNewChan, mFlags,
+                                                this);

Incorrect indentation on this second line

+++ b/netwerk/base/src/nsIncrementalDownload.cpp	Sat May 22 00:03:11 2010 +0200
+      NS_WARNING(("nsIncrementalDownload::OnChannelRedirect : "

no space before colon (several times in this function)

+                  "oldChannel not an nsIGttpChannel"));

Gttp -> Http

+  if (NS_FAILED(rv)) {
+      cb->OnRedirectVerifyCallback(rv);
+      return NS_OK;

two-space indentation (also in other places in this function)

+++ b/rdf/base/src/nsRDFXMLDataSource.cpp	Sat May 22 00:03:11 2010 +0200
+  if (NS_FAILED(rv)) {
+      NS_WARNING(("RDFXMLDataSourceImpl::OnChannelRedirect : "

two-space indentation, no space before comma

+++ b/uriloader/base/nsDocLoader.cpp	Sat May 22 00:03:11 2010 +0200
                                              nsIChannel *aNewChannel,
-                                             PRUint32    aFlags)
+                                             PRUint32    aFlags,
+                                             nsIAsyncVerifyRedirectCallback *cb)

please either align all parameters, or use one space consistently

+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp	Sat May 22 00:03:11 2010 +0200
+            NS_WARNING(("nsOfflineCacheUpdateItem::OnChannelRedirect : "

no space before colon

+                NS_WARNING(("nsOfflineCacheUpdateItem::OnChannelRedirect : "

same

     NS_ENSURE_STATE(httpChannel);

You forgot to remove this line

+        NS_WARNING(("nsOfflineCacheUpdateItem::OnChannelRedirect : "

same

+++ b/uriloader/prefetch/nsPrefetchService.cpp	Sat May 22 00:03:11 2010 +0200
+        NS_WARNING(("nsPrefetchNode::OnChannelRedirect : bad state"));

no space before colon

Comment 31

[Honza Bambas (:mayhemer)]

Bjarne, any updates on this bug?

Comment 32

[Bjarne (:bjarne)]

Not really  :(  I'll unbitrot and address review-comments when I have bug #549767 under control. However, the worrying part is IMO this

(In reply to comment #30)
> - it is quite unfortunate that JS implementations of nsIChannelEventSink will
> silently do the wrong thing with this patch :/

It's worrying because it's true and bound to introduce problems, but it is the currently chosen way to change the API.

Comment 33

[Honza Bambas (:mayhemer)]

Won't this issue go away, when we rename the method?

Comment 34

[Bjarne (:bjarne)]

I guess you mean comment #27? My interpretation of comment #28 is that we shouldn't bother...  or do I miss something?

Comment 35

[Honza Bambas (:mayhemer)]

So, to summarize, we agreed on following way of the new API behavior:

- leave the interface, just change the IID
- rename the method from OnChannelRedirect to AsyncOnChannelRedirect
- when this method implementation throws any exception, we consider the redirect as vetoed, i.e. we block the redirect channel load; this safe precaution and makes implementation easier, when the result can be determined synchronously
- when this method returns NS_OK (i.e. doesn't throw), we expect exactly one call of OnRedirectVerifyCallback back with the result; this callback can be called from within the AsyncOnChannelRedirect method or asynchronously after the method returns

Comment 36

[Bjarne (:bjarne)]

Attachment: New API-semantics + comments from reviewer

Unbitrotted, changed method name and reverted to earlier version of semantics as described in comment #35. Reviewers comments from comment #30 have been taken care of (afaics).

I get a few errors when running local tests and I'm currently working my way through these. Hopefully they are due to me wrongly translating the logic in some sinks - any (formal or informal) reviews of these would be really helpful (there are 30+ of them). In particular, I would appreciate if someone takes a closer look at the logic of imgRequest::OnChannelRedirect() and picks up any mistakes I may have made there.

Requesting formal review even with local errors due to the rush to get this done...

Comment 37

[Johnny Stenback (:jst)]

Bjarne, any luck with the remaining errors you were seeing locally with this change?

Comment 38

[Bjarne (:bjarne)]

I believe some of them may be due to an issue in Part 1 which I'm discussing with Honza. Also searching for other causes without luck so far.

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: update: makes test_bug435425.html work

I would match rather see nsChannelCanceller left used and play with DontCancel() where necessary, here in this method.

Comment 40

[Bjarne (:bjarne)]

You're a gem, Honza! :) This is right on spot and explains explicit calls to |oldChannel->Cancel()| in several sinks which has puzzled me all the time while working on this.

To summarize Honzas findings: In order to cancel loading of the old channel (which received the 30x response), its Cancel-method *must* be called from the sink. Otherwise it will continue loading as if it received a 200, even when it receives error-response from the sinks. My patch fails to call Cancel() in (at least) one place, hence certain tests fail.

Question: Doesn't this break our carefully designed API? IIRC, we state that if any sink vetos the redirect it will be canceled. We don't state that a vetoing sink must cancel the old channel itself.

See code in nsHttpChannel::ProcessResponse() at http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#905 and note that ProcessNormal() is called (in non-ssl cases) regardless of the return-value from ProcessRedirection(). Moreover, see http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2877 and observe that if any sink returns failure, this failure is just returned from this method.

Comment 41

[Bjarne (:bjarne)]

Comment on attachment 454833 [details] [diff] [review]
New API-semantics + comments from reviewer

Found a (stupid) source for leaks - canceling request for review.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Question: Doesn't this break our carefully designed API? IIRC, we state that if
> any sink vetos the redirect it will be canceled. We don't state that a vetoing
> sink must cancel the old channel itself.

Uh...  Why should we state that?  If the redirect is canceled, we should render the response-body of the 3xx response, which may contain information that the user cares about, no?

Comment 43

[Bjarne (:bjarne)]

Yeah - the implication of that question did seem a little fantastic...  :)

But it means that sinks decide the fate of the old channel, right? If no sinks touch the old channel, observers will see it as successfully loaded? (Maybe this should be stated somewhere?)

Comment 44

[Boris Zbarsky [:bzbarsky]]

> But it means that sinks decide the fate of the old channel, right?

Yes, as do various other necko observers....

> If no sinks touch the old channel, observers will see it as successfully loaded?

Yes.

Comment 45

[Bjarne (:bjarne)]

I'll add some words about this in the API-doc for nsIChannelEventSink, since IMO this behavior is not at all obvious.

Still get some leaks with the patch... :(

Comment 46

[Bjarne (:bjarne)]

The leaks (in xpcshell-tests) I've been tracing also occur with only the patch for Part 1 applied. Part 1 has bitrotted, btw.

Comment 47

[Honza Bambas (:mayhemer)]

Bjarne, do you know concretely which xpc-shell test leaks?  I was runnig just the necko set and also try server didn't show anything, maybe something has changed over the time...

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 454833 [details] [diff] [review]
New API-semantics + comments from reviewer

+++ b/content/base/src/nsCrossSiteListenerProxy.cpp	Tue Jun 29 13:55:42 2010 +0200
+  if (outer)
+    return outer->AsyncOnChannelRedirect(aOldChannel, aNewChannel,
+                                         aFlags, this);

Please put {} around the if-body

+++ b/content/base/src/nsXMLHttpRequest.cpp	Tue Jun 29 13:55:42 2010 +0200
-    if ((mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {
-       return NS_ERROR_DOM_BAD_URI;
-    }
+    if ((mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT))
+      return NS_ERROR_DOM_BAD_URI;

why? afaik it's content style to always put {} around if-bodies

+++ b/content/base/test/test_bug435425.html	Tue Jun 29 13:55:42 2010 +0200

Why the commented-out tests? If you're commenting out code, please always add a comment why you're doing that.

+++ b/content/html/content/src/nsHTMLMediaElement.cpp	Tue Jun 29 13:55:42 2010 +0200
+  // TODO is this really correct?? Ignoring the return-value here...?

hm... so, that function returns an error is the new channel is not HTTP. that's probably not so great (?), but the other errors probably shouldn't be ignored.

+++ b/modules/libpr0n/src/imgLoader.cpp	Tue Jun 29 13:55:42 2010 +0200
   if (!target)
-    return NS_OK;
-  return target->OnChannelRedirect(oldChannel, newChannel, flags);
+      cb->OnRedirectVerifyCallback(NS_OK);
+
+  // Delegate to |target| if set, reusing |cb|
+  return target->AsyncOnChannelRedirect(oldChannel, newChannel, flags, cb);

there's a return missing after cb->..., as it is you'll crash if target is null.

+++ b/modules/libpr0n/src/imgRequest.cpp	Tue Jun 29 13:55:42 2010 +0200
 imgRequest::GetInterface(const nsIID & aIID, void **aResult)
 {
-  if (!mPrevChannelSink || aIID.Equals(NS_GET_IID(nsIChannelEventSink)))
+  if (!mPrevChannelSink ||
+      aIID.Equals(NS_GET_IID(nsIChannelEventSink)) ||
+      aIID.Equals(NS_GET_IID(nsIAsyncVerifyRedirectCallback)))

No point in adding the callback here.

+++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp	Tue Jun 29 13:55:42 2010 +0200
+static PRLogModuleInfo *hLog = PR_NewLogModule("nsRedirect");

why 'h'? s/g seem more common (static/global)

+    if (!mCallbackInitated)

Could you rename this to mCallbackInitiated?

+nsAsyncRedirectVerifyHelper::GetInterface(const nsIID & aIID, void **aResult)

what's that for?

+++ b/netwerk/base/src/nsIOService.cpp	Tue Jun 29 13:55:42 2010 +0200 nsIOService::OnChannelRedirect(nsIChannel* oldChan, nsIChannel* newChan,

now that this is async, please rename it to AsyncOnChannelRedirect

+++ b/netwerk/base/src/nsIncrementalDownload.cpp	Tue Jun 29 13:55:42 2010 +0200
+nsIncrementalDownload::AsyncOnChannelRedirect(nsIChannel *oldChannel,
                                          nsIChannel *newChannel,
-                                         PRUint32 flags)
+                                         PRUint32 flags,
+                                         nsIAsyncVerifyRedirectCallback *cb)

fix the indentation, please

Comment 49

[Bjarne (:bjarne)]

Attachment: Unbitrotted and addressed comments from review

(In reply to comment #48)
> +  if (outer)
> +    return outer->AsyncOnChannelRedirect(aOldChannel, aNewChannel,
> +                                         aFlags, this);
> 
> Please put {} around the if-body

Right...

> +++ b/content/base/src/nsXMLHttpRequest.cpp    Tue Jun 29 13:55:42 2010 +0200
> -    if ((mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {
> -       return NS_ERROR_DOM_BAD_URI;
> -    }
> +    if ((mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT))
> +      return NS_ERROR_DOM_BAD_URI;
> 

Sorry...  fixed.

> +++ b/content/base/test/test_bug435425.html    Tue Jun 29 13:55:42 2010 +0200
> 
> Why the commented-out tests? If you're commenting out code, please always add a
> comment why you're doing that.

Hunting down an error and forgot to re-enable tests. Fixed.

> +++ b/content/html/content/src/nsHTMLMediaElement.cpp    Tue Jun 29 13:55:42
> 2010 +0200
> +  // TODO is this really correct?? Ignoring the return-value here...?
> 
> hm... so, that function returns an error is the new channel is not HTTP. that's
> probably not so great (?), but the other errors probably shouldn't be ignored.

I'm open to suggestions for how to deal with this. It's not my code - I'm just pointing it out.

> +++ b/modules/libpr0n/src/imgLoader.cpp    Tue Jun 29 13:55:42 2010 +0200
>    if (!target)
> -    return NS_OK;
> -  return target->OnChannelRedirect(oldChannel, newChannel, flags);
> +      cb->OnRedirectVerifyCallback(NS_OK);
> +
> +  // Delegate to |target| if set, reusing |cb|
> +  return target->AsyncOnChannelRedirect(oldChannel, newChannel, flags, cb);
> 
> there's a return missing after cb->..., as it is you'll crash if target is
> null.

Ahh... thanks! That could be bad...

> +++ b/modules/libpr0n/src/imgRequest.cpp    Tue Jun 29 13:55:42 2010 +0200
>  imgRequest::GetInterface(const nsIID & aIID, void **aResult)
>  {
> -  if (!mPrevChannelSink || aIID.Equals(NS_GET_IID(nsIChannelEventSink)))
> +  if (!mPrevChannelSink ||
> +      aIID.Equals(NS_GET_IID(nsIChannelEventSink)) ||
> +      aIID.Equals(NS_GET_IID(nsIAsyncVerifyRedirectCallback)))
> 
> No point in adding the callback here.

See last part of comment #22 and comment #23, but I'd be happy to improve my understanding of this if you can contribute to that.

> 
> +++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp    Tue Jun 29 13:55:42
> 2010 +0200
> +static PRLogModuleInfo *hLog = PR_NewLogModule("nsRedirect");
> 
> why 'h'? s/g seem more common (static/global)

Hmm... can only blame keyboard-layout and fast typing...  :)  (fixed)

> +    if (!mCallbackInitated)
> 
> Could you rename this to mCallbackInitiated?

Sure...

> +nsAsyncRedirectVerifyHelper::GetInterface(const nsIID & aIID, void **aResult)
> 
> what's that for?

Still comment #22 and comment #23, but I'd be happy to improve my understanding.

> +++ b/netwerk/base/src/nsIOService.cpp    Tue Jun 29 13:55:42 2010 +0200
> nsIOService::OnChannelRedirect(nsIChannel* oldChan, nsIChannel* newChan,
> 
> now that this is async, please rename it to AsyncOnChannelRedirect

Ok.

> +++ b/netwerk/base/src/nsIncrementalDownload.cpp    Tue Jun 29 13:55:42 2010
> +0200
> +nsIncrementalDownload::AsyncOnChannelRedirect(nsIChannel *oldChannel,
>                                           nsIChannel *newChannel,
> -                                         PRUint32 flags)
> +                                         PRUint32 flags,
> +                                         nsIAsyncVerifyRedirectCallback *cb)
> 
> fix the indentation, please

Ok.

Comment 50

[Bjarne (:bjarne)]

This patch (together with a locally unbitrotted part-1) was pushed to tryserver but gives reproducible failures which seem very relevant. Looking into those...

Comment 51

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #49)
> I'm open to suggestions for how to deal with this. It's not my code - I'm just
> pointing it out.

Yeah, maybe just file a bug on it so that the author of that code can fix it the right way.

> See last part of comment #22 and comment #23, but I'd be happy to improve my
> understanding of this if you can contribute to that.

GetInterface doesn't need the callback interface (QueryInterface does, of course). These GetInterface impls are just for allowing necko to get the right event sink implementation; but the callback doesn't need to be available there.

> > +nsAsyncRedirectVerifyHelper::GetInterface(const nsIID & aIID, void **aResult)
> > 
> > what's that for?
> 
> Still comment #22 and comment #23, but I'd be happy to improve my
> understanding.

Same answer, except that I don't even think you're actually passing this anywhere that uses the helper as an nsIInterfaceRequestor


Will mark the r+ in a bit, gotta run to a bus now.

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 457284 [details] [diff] [review]
Unbitrotted and addressed comments from review

r=biesi but please do remove the *Callback from the various GetInterface functions.

Comment 53

[Honza Bambas (:mayhemer)]

So, this patch seems to cause following test to fail:

- chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_updatessl.js | Combination of: Should have seen no failure for a http* to http* redirect - Got -2, expected 0; fairly regular

For reference (just a subset):

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279136640.1279137377.17846.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279144042.1279144676.12598.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279139165.1279141386.32551.gz

Bjarne, please drop a note you are about to look at this, otherwise I'll try to check it myself.

Comment 54

[Bjarne (:bjarne)]

(In reply to comment #53)
> Bjarne, please drop a note you are about to look at this, otherwise I'll try to
> check it myself.

This is the failing test I'm tracking down, yes.

Comment 55

[Bjarne (:bjarne)]

> > See last part of comment #22 and comment #23, but I'd be happy to improve my
> > understanding of this if you can contribute to that.
> 
> GetInterface doesn't need the callback interface (QueryInterface does, of
> course). These GetInterface impls are just for allowing necko to get the right
> event sink implementation; but the callback doesn't need to be available there.

I'm really looking for the right mechanism to allow calling back from JS. Or in other words, how to let JS know that a certain wrapped native object implements (in this case) nsIAsyncVerifyRedirectCallback and therefore allow invoking the callback-method. From comments #22 and #23 I got the impression that properly implementing GetInterface resolves this (and that maintaining these is "a good thing" in any case).

Any insight is appreciated...  :)

Comment 56

[Bjarne (:bjarne)]

(In reply to comment #54)
> (In reply to comment #53)
> > Bjarne, please drop a note you are about to look at this, otherwise I'll try to
> > check it myself.
> 
> This is the failing test I'm tracking down, yes.

Or rather "tracking down the cause of..."  :]

I found the problem. The core issue is (again) the one described in previous comment, namely how to allow JS to call back into a native object. Solution seems to be as described in comment #12 third paragraph, meaning that the two questions in the same comment becomes relevant again.

This all seemed to be resolved as per comment #22 comment #23 and comment #24 but now I'm not certain about the proper way to do this. Further insight is highly appreciated!

I'll push this to tryservers and get back about the results.

Comment 57

[Bjarne (:bjarne)]

(In reply to comment #55)
> Any insight is appreciated...  :)

Although the issue appears to be fixed I'll try to be a little more specific so that a clear explanation may be provided by someone who know this stuff...  the problematic code-lines look like this

[1]      sink->AsyncOnChannelRedirect(param1, param2, param3, this);

where sink is implemented in JS. The sink receives the call like this

      AsyncOnChannelRedirect(p1, p2, p3, callback)

and will want to do

[2]      callback->OnRedirectverifyCallback(result)

In order to make [1] work, normal inheritance should work, right? Possibly by maintaining QueryInterface() if it is explicitly implemented?

And in order to make [2] work, my solution is to add to nsDOMClassInfo. Is this the right mechanism?

Comment 58

[Johnny Stenback (:jst)]

I *suspect* what's going on here is that XPConnect is blocking us from wrapping nsXMLHttpRequest as an nsIAsyncVerifyRedirectCallback, and it's doing that because we do in fact tell it not to expose interfaces to JS that are not explicitly permitted in its class info (which is done by setting the flag nsIXPCScriptable::CLASSINFO_INTERFACES_ONLY).

Assuming that is what's happening here (I tried to verify, but the patches for these bugs don't apply cleanly to trunk any more, again), then there's two ways to fix this, one is to add the interface nsIAsyncVerifyRedirectCallback to the list of exposed interfaces, the other way is to make nsXMLHttpRequest use a separate object as its nsIAsyncVerifyRedirectCallback implementation. And I'm afraid we're going to have to go with the latter. If we simply just add nsIAsyncVerifyRedirectCallback to the list of exposed interfaces on nsXMLHttpRequest then that means that more methods that don't belong on the web get exposed to the web, and untrusted code can arbitrarily call nsXMLHttpRequest::OnRedirectVerifyCallback(), which isn't a good idea. If we add a separate class and pass an instance of that class as the callback then we avoid both of those issues all together.

Let me know if that makes sense, or let me know what revisions the relevant patches apply cleanly against so that I can have a closer look here.

Comment 59

[Bjarne (:bjarne)]

Thanks Johnny! Makes a lot of sense. But for my curiosity: what makes it safer to use a separate class as the callback (the same code must be executed)? Can it be made "chrome-only" or something?

Comment 60

[Johnny Stenback (:jst)]

Attachment: Suggested fix for the XHR issue.

Bjarne, here's a patch that separates the async verify redirect callback from nsXMLHttpRequest, which fixes the failing test (got updated patches from Honza so I was able to test, thanks Honza!). The reason this is safer is that a) there's no way this separate object is ever exposed to content script (and thus not the OnRedirectVerifyCallback() method either), and b) even if it was this separate object has no class info or scriptable helpers at all, which means that it's only ever callable by chrome code.

Comment 61

[Bjarne (:bjarne)]

Looks great - thanks again! :)  I'll update my stuff tomorrow (getting late around here) and push to tryservers.

Now, rephrasing question 1 from comment #12: Does it make sense to do this now also for the other sinks (3, I think) which also implement nsIAsyncVerifyRedirectCallback? And what about nsAsyncRedirectVerifyHelper - I guess we can have a JS sink as per-channel observer, or in the category/as the global sink of nsIOService (all these will receive an instance of nsAsyncRedirectVerifyHelper for callback)?

Comment 62

[Johnny Stenback (:jst)]

(In reply to comment #61)
> Looks great - thanks again! :)  I'll update my stuff tomorrow (getting late
> around here) and push to tryservers.

Great!

> Now, rephrasing question 1 from comment #12: Does it make sense to do this now
> also for the other sinks (3, I think) which also implement
> nsIAsyncVerifyRedirectCallback?

If there are other cases where we're dealing with objects that are exposed to untrusted scripts on the web then yes, I think we should do something similar for them as well.

Comment 63

[Bjarne (:bjarne)]

Comment on attachment 457642 [details] [diff] [review]
Suggested fix for the XHR issue.

Very nice and useful - r=me (fwiw)

Comment 64

[Bjarne (:bjarne)]

(In reply to comment #51)
> (In reply to comment #49)
> > I'm open to suggestions for how to deal with this. It's not my code - I'm just
> > pointing it out.
> 
> Yeah, maybe just file a bug on it so that the author of that code can fix it
> the right way.

Bug #579329 filed to cover this.

Comment 65

[Bjarne (:bjarne)]

Attachment: Updated with comments from last review and separate class to implement nsIAsyncRedirectVerifyCallback

I've taken the XHR fix from above and generalized it so that it can be used by other sinks than nsXMLHttpRequest. Renamed to AsyncVerifyRedirectCallbackForwarder because original name was similar to existing helper-class. Requesting review by jst since changes from the version which was r+'ed by biesi essentially involves the use of this new class. Feel free to re-assign review to whoever appropriate....

Passes simple local testing - pushed to tryserver...

Comment 66

[Johnny Stenback (:jst)]

Comment on attachment 457852 [details] [diff] [review]
Updated with comments from last review and separate class to implement nsIAsyncRedirectVerifyCallback

 NS_IMETHODIMP
-nsCrossSiteListenerProxy::OnChannelRedirect(nsIChannel *aOldChannel,
+nsCrossSiteListenerProxy::AsyncOnChannelRedirect(
+                                            nsIChannel *aOldChannel,
                                             nsIChannel *aNewChannel,
-                                            PRUint32    aFlags)
+                                            PRUint32 aFlags,
+                                            nsIAsyncVerifyRedirectCallback *cb)

Fix up argument indentation here please...

>+    nsRefPtr<AsyncVerifyRedirectCallbackForwarder> fwd =
>+      new AsyncVerifyRedirectCallbackForwarder(this);

... and I can't see why we need the forwarder here? Seems to me that this is a completely internal class where it's perfectly safe to pass the instance itself as the async verify redirect callback.

 NS_IMETHODIMP
-nsObjectLoadingContent::OnChannelRedirect(nsIChannel *aOldChannel,
+nsObjectLoadingContent::AsyncOnChannelRedirect(
+                                          nsIChannel *aOldChannel,
                                           nsIChannel *aNewChannel,
-                                          PRUint32    aFlags)
+                                          PRUint32    aFlags,
+                                          nsIAsyncVerifyRedirectCallback *cb)

Fix argument indentation here too.

- In nsXMLHttpRequest.cpp:

-nsACProxyListener::OnChannelRedirect(nsIChannel *aOldChannel,
+nsACProxyListener::AsyncOnChannelRedirect(
+                                     nsIChannel *aOldChannel,
                                      nsIChannel *aNewChannel,
-                                     PRUint32 aFlags)
+                                     PRUint32 aFlags,
+                                     nsIAsyncVerifyRedirectCallback *callback)

... and here.

-nsXMLHttpRequest::OnChannelRedirect(nsIChannel *aOldChannel,
+nsXMLHttpRequest::AsyncOnChannelRedirect(
+                                    nsIChannel *aOldChannel,
                                     nsIChannel *aNewChannel,
-                                    PRUint32    aFlags)
+                                    PRUint32    aFlags,
+                                    nsIAsyncVerifyRedirectCallback *callback)

same thing.

-imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags)
+imgRequest::AsyncOnChannelRedirect(nsIChannel *oldChannel,
+                                  nsIChannel *newChannel, PRUint32 flags,
+                                  nsIAsyncVerifyRedirectCallback *callback)

Fix the off-by-one indentation here.

- Also in imgRequest::AsyncOnChannelRedirect():

+    nsRefPtr<AsyncVerifyRedirectCallbackForwarder> fwd =
+      new AsyncVerifyRedirectCallbackForwarder(this);

I can't see why we'd need the forwarder here.

- In netwerk/base/public/nsAsyncRedirectVerifyHelper.h:

+class nsAsyncRedirectAutoCallback
+{
+public:
+    nsAsyncRedirectAutoCallback(nsIAsyncVerifyRedirectCallback* aCallback)
+    : mCallback(aCallback)
+  {
+      mResult = NS_OK;
+  }

Constructor name is indented too far.


+/*
+ * Simple helper class that just forwards the redirect callback back to the
+ * nsInternalAsyncVerifyRedirectCallback. 
+ */
+class AsyncVerifyRedirectCallbackForwarder : public nsIAsyncVerifyRedirectCallback

This class can't live here, at least not yet. Declaring this class here and using it outside of the necko library won't work in non-libxul builds (which we won't support for much longer, but long enough that we'll need to deal with it here). But I think that's ok, we can simply move this back into the nsXMLHttpRequest since I really think that's the only case where we need this, if not, it can live in nsContentUtils if it's really needed in other classes in the layout library. But again, I don't think we need it anywhere other than in nsXMLHttpRequest.

+  AsyncVerifyRedirectCallbackForwarder(nsInternalAsyncVerifyRedirectCallback *cb)

nsInternalAsyncVerifyRedirectCallback is identical to nsIAsyncVerifyRedirectCallback, I say we just use that here and not add nsInternalAsyncVerifyRedirectCallback to the IDL file at all. As for nsXMLHttpRequest, it can inherit from that interface, it just can't (or need to) claim to support it in its QueryInterface method, which will leave the OnRedirectVerifyCallback() unreachable from script.

+private:
+  nsRefPtr<nsInternalAsyncVerifyRedirectCallback> mCallback;

nsIAsyncVerifyRedirectCallback here too.

- In nsIncrementalDownload::OnChannelRedirect():

+    nsRefPtr<AsyncVerifyRedirectCallbackForwarder> fwd =
+      new AsyncVerifyRedirectCallbackForwarder(this);
+
+    rv = sink->AsyncOnChannelRedirect(oldChannel, newChannel, flags, fwd);

No need for a forwarder here AFAICT.

-nsURIChecker::OnChannelRedirect(nsIChannel *aOldChannel,
+nsURIChecker::AsyncOnChannelRedirect(nsIChannel *aOldChannel,
                                 nsIChannel *aNewChannel,
-                                PRUint32    aFlags)
+                                PRUint32    aFlags,
+                                nsIAsyncVerifyRedirectCallback *callback)

Fix argument indentation.

-RDFXMLDataSourceImpl::OnChannelRedirect(nsIChannel *aOldChannel,
+RDFXMLDataSourceImpl::AsyncOnChannelRedirect(nsIChannel *aOldChannel,
                                         nsIChannel *aNewChannel,
-                                        PRUint32 aFlags)
+                                        PRUint32 aFlags,
+                                        nsIAsyncVerifyRedirectCallback *cb)

Here too...

-FetchNetworkIconStep::OnChannelRedirect(nsIChannel* oldChannel,
+FetchNetworkIconStep::AsyncOnChannelRedirect(nsIChannel* oldChannel,
                                         nsIChannel* newChannel,
-                                        PRUint32 flags)
+                                        PRUint32 flags,
+                                        nsIAsyncVerifyRedirectCallback *cb)

And here.

-nsXPInstallManager::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags)
+nsXPInstallManager::AsyncOnChannelRedirect(nsIChannel *oldChannel,
+                                      nsIChannel *newChannel,
+                                      PRUint32 flags,
+                                      nsIAsyncVerifyRedirectCallback *callback)

And here.

With that fixed I think this is ready to go. r- until then.

Comment 67

[Johnny Stenback (:jst)]

Attachment: Review comments fixed.

Update to reflect my own review comments per discussion with Bjarne (who is out for a few days). This has r=jst and sr=biesi, and is ready to land with the fix for bug 513086.

Comment 68

[Honza Bambas (:mayhemer)]

To all: I'm just updating this patch according to bug 513086 comment 72.  There soon (tomorrow morning) will be a small update patch as was for that bug.

Comment 69

[Bjarne (:bjarne)]

Attachment: Unbitrotted and updated

Updated with nits from comment #68 plus various communication with Honza. Pushed to tryserver.

Comment 70

[Honza Bambas (:mayhemer)]

(In reply to comment #69)
> Pushed to tryserver.

Passed.

Comment 71

[Bjarne (:bjarne)]

Attachment: Updated with offline comments

Honza spotted a mistake and suggested a few improvements in an offline review. Pushed to tryserver again...

Comment 72

[Johnny Stenback (:jst)]

With the latest patch from this bug I crashed once on startup with a stack like this:

#2  <signal handler called>
#3  0x00007f73753050f6 in nsHttpChannel::ContinueProcessResponse (this=
    0x7f7362793000, rv=2147500035)
    at ../../../../mozilla/netwerk/protocol/http/nsHttpChannel.cpp:1008
#4  0x00007f73752ffeb9 in nsHttpChannel::OnRedirectVerifyCallback (this=
    0x7f7362793000, result=2147500035)
    at ../../../../mozilla/netwerk/protocol/http/nsHttpChannel.cpp:4482
#5  0x00007f737529aa3c in nsAsyncVerifyRedirectCallbackEvent::Run (
    this=<value optimized out>)
    at ../../../../mozilla/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp:73
#6  0x00007f7375c3cbac in nsThread::ProcessNextEvent (this=0x7f73730491f0, 
    mayWait=0, result=0x7fffb364bd3c)
    at ../../../mozilla/xpcom/threads/nsThread.cpp:547

The reason for the crash is that mTransaction is null. I don't know if this patch is to blame or the patch in bug 513086 is to blame (where nsHttpChannel::ContinueProcessResponse() was introduced), and I've so far been unable to reproduce this crash so it's hard to say. I can open a new bug on this if we think this issue is unrelated to this patch.

Comment 73

[Bjarne (:bjarne)]

I don't have any reasonable input to the previous comment, and with the state of m-c today it's darn hard to say anything definite about the latest tryserver results... :(

This one

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280809703.1280810443.27281.gz

*may* be related to this patch and it also occurs on more than one platform. I'll try to reproduce it locally while waiting for the tree to cool down...

Comment 74

[Bjarne (:bjarne)]

Not able to reproduce locally, however m-c seems to have cooled down now so I'm pushing the patch to tryserver again.

Comment 75

[Bjarne (:bjarne)]

Passes tryserver (all oranges are starred) with the exception of a timeout in

/tests/dom/tests/mochitest/dom-level1-core/test_PIsetdatanomodificationallowederrEE.html

on Linux and Linux64 (mochitest-2). This doesn't make much sense to me....  anyone who has seen this?

If a reasonable explanation for the above can be given I'd say that this is ready for landing. Advice for how to proceed is in any case appreciated...

Comment 76

[Benjamin Smedberg]

Does it fail locally when you run that test?

Comment 77

[Bjarne (:bjarne)]

The test seems to have only two TODOs (which is why it is pretty weird)...

Comment 78

[(no longer active)]

(In reply to comment #75)
> Passes tryserver (all oranges are starred) with the exception of a timeout in
> 
> /tests/dom/tests/mochitest/dom-level1-core/test_PIsetdatanomodificationallowederrEE.html
> 
> on Linux and Linux64 (mochitest-2). This doesn't make much sense to me.... 
> anyone who has seen this?

This is intermittent.  I filed bug 584368.

Comment 79

[Bjarne (:bjarne)]

In that case: this patch results in 10 oranges from tryserver, all starred. It has been r+ and sr+'ed so I'm requesting check-in. If this violates procedure, feel free to correct me, anyone.

Comment 80

[Honza Bambas (:mayhemer)]

It would be good to get reviewed, or at least seen, diff between 'Review comments fixed' patch and 'Updated with offline comments' patch (the last one).  There were test failures that were fixed, we should check those difference.

Comment 81

[(no longer active)]

You need to request approval2.0 on this patch in order to be able to land it.

Comment 82

[Bjarne (:bjarne)]

Comment on attachment 462193 [details] [diff] [review]
Updated with offline comments

Ok - thanks for corrections guys. Requesting r, sr and approval2.0

Comment 83

[Johnny Stenback (:jst)]

Comment on attachment 462193 [details] [diff] [review]
Updated with offline comments

This is a beta blocker, doesn't need explicit approval.

Comment 84

[Johnny Stenback (:jst)]

Attachment: Changes since the last reviewed patch.

For the record, this shows the changes from the last reviewed patch to Bjarne's last patch. Given how straight forward these changes are I think this is good to land, I'll stamp the patch.

Comment 85

[Johnny Stenback (:jst)]

Comment on attachment 462193 [details] [diff] [review]
Updated with offline comments

Biesi, I don't mean to step on your toes here, but I don't think it's necessary for you to look through this.

Comment 86

[Johnny Stenback (:jst)]

Comment on attachment 462983 [details] [diff] [review]
Changes since the last reviewed patch.

For the records, the #include changes in this diff shouldn't have been included, I just didn't bother merging those changes when creating the interdiff, I focused on the functional changes...

Comment 87

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/11170037df20

Comment 88

[Matheus Kerschbaum]

I should note that the build with patches from this bug doesn't work on any website, at all. As soon as I startup the home page is blank and I can't connect to any site.

Comment 89

[Johnny Stenback (:jst)]

Matheus, do you have any extensions installed? This change breaks API compatibility with some extensions that care about redirects (such as Adblock etc).

Comment 90

[Matheus Kerschbaum]

(In reply to comment #89)
> Matheus, do you have any extensions installed? This change breaks API
> compatibility with some extensions that care about redirects (such as Adblock
> etc).

Odd, I remember trying this build in safe-mode and still failing to load sites, but now it worked. Anyway it was actually Noscript that busted websites connection here, I'm still using Adblock just fine.

Comment 91

[imradyurrad]

This seems to have broken something. Most pages now give me "Moved Temporarily
The document has moved here. "

Happens on Google and Yahoo. Clicking on youtube links, the page doesn't load.

Comment 92

[Johnny Stenback (:jst)]

Taz, do you have any extensions installed? Try starting firefox with a clean profile, or in safe mode. See the previous few comments...

Comment 93

[imradyurrad]

@Johnny, it seems to have fixed itself somehow. I don't think it was caused by an addon since I only have Adblock installed.

Comment 94

[Johnny Stenback (:jst)]

Adblock *is* affected by this change. But exactly to what extent I don't know. Image redirects seem to be problematic at least, probably other cases as well.

Comment 95

[IU]

For me, the following extensions are affected by this:
- Charset Switcher 4.0.20100720pre
- Adblock Plus 1.3a.20100726
- NoScript 2.0

If you have any of those installed, visit http://abcnews.go.com/ and you will see the "The document has moved here" page.

Disabling all of those addons solve the problem.

Comment 96

[IU]

Also, I get a whole bunch of the following in Error Console:

Error: 'JavaScript component does not have a method named: "asyncOnChannelRedirect"' when calling method: [nsIChannelEventSink::asyncOnChannelRedirect] = NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED

Comment 97

[Johnny Stenback (:jst)]

Yup, all from incompatible extensions.

Comment 98

[Bjarne (:bjarne)]

To make it clear: This change deliberately (see e.g. comment #16) breaks the redirect API (ref bug-title). Any extension/add-on using the old version of this API is *bound* to malfunction. Upgrading to use the new API is quite simple though, so the situation should be quickly resolved.

Comment 99

[alex_mayorga]

(In reply to comment #95)
> - Adblock Plus 1.3a.20100726
Filed https://www.mozdev.org/bugs/show_bug.cgi?id=23092 FWIW

Comment 100

[Giorgio Maone [:ma1]]

Fixed in NoScript 2.0.1rc4 (which is an implementor but also a client of this API) FWIW
http://noscript.net/getit#devel

Comment 101

[Gary King]

too extensionpath(os different)\extensions\{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}\modules\ContentPolicy.jsm in line 395
under

// nsIChannelEventSink interface implementation

add this

asyncOnChannelRedirect: function(oldChan, newChan, flags, redirectCallback) {
this.onChannelRedirect(oldChan, newChan, flags);
redirectCallback.onRedirectVerifyCallback(0);
},

so it looks like

// nsIChannelEventSink interface implementation
//
asyncOnChannelRedirect: function(oldChan, newChan, flags, redirectCallback) {
this.onChannelRedirect(oldChan, newChan, flags);
redirectCallback.onRedirectVerifyCallback(0);
},
onChannelRedirect: function(oldChannel, newChannel, flags)
{

This is not fixed as far as Adblock Plus is concerned. The above code will fix the problem.

Comment 102

[pal-moz]

Beta 3 Candidate Build 3
Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3) Gecko/20100805 Firefox/4.0b3 ID:20100805192522


is this included in Beta 3 ?

Comment 103

[Johnny Stenback (:jst)]

No, this is not in Firefox 4 beta 3, but it will be in beta 4.

Comment 104

[Jim Jeffery not reading bug-mail 1/2/11]

Did this by chance cause; https://bugzilla.mozilla.org/show_bug.cgi?id=585214

Comment 105

[Bjarne (:bjarne)]

(In reply to comment #104)
> Did this by chance cause; https://bugzilla.mozilla.org/show_bug.cgi?id=585214

Perhaps. I'll take a look...

Comment 106

[Wladimir Palant]

Adblock Plus implementation has been fixed as well so 1.3a.20100817 development build tomorrow will be fine. I'll also release Adblock Plus 1.2.2 from branch soon to fix the problem for people using the stable version.

Comment 107

[Gary King]

This bug is not fixed I am still getting redirects and 301 errors.

Comment 108

[Wladimir Palant]

Gary, the fix won't magically propagate itself back into the current development build that was created tonight. The next development build will be generated in about 12 hours, that one will work.

Comment 109

[Gary King]

To Comment 108.

Thanks Wladimir, I guess I misread comment 106.

Comment 110

[Eric Shepherd [:sheppy]]

Documentation updated (added, really, none of the interfaces involved had docs yet at all):

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIChannelEventSink
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAsyncVerifyRedirectCallback

And mentioned here:

https://developer.mozilla.org/en/Firefox_4_for_developers#Interface_changes