513086 - Make redirect API async.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Johnny Stenback (:jst)]

For electrolysis it'd be nice to have the redirect callback be an async callback so that we don't end up needing to do sync IPC from within an OnChannelRedirect() callback. Plugins are also interested in participating in the redirect decision for their own streams, and currently some of the things they want to do are really hard because of the fact that the API is sync.

This change will obviously break APIs, but given the electrolysis work we'll be doing anyways, it's unlikely that the API can remain what it is today, so now would be a good time to change the redirect API.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need this for other things too!

Comment 2

[Honza Bambas (:mayhemer)]

-> me

Comment 3

[Honza Bambas (:mayhemer)]

A question:

nsBaseChannel::Open (the synchronous version) calls Redirect to initialize/open/get the real channel. It also calls gIOService->OnChannelRedirect even it is not creating a true redirect channel but just the "first time" channel. It is probably a bug, isn't it?

see: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBaseChannel.cpp#498 and http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBaseChannel.cpp#98.

IMHO OnChannelRedirect call should be conditioned by openNewChannel?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not following.  If OpenContentStream hands back a channel (which it normally doesn't), then that means that we need to redirect to that channel.  If the redirect is under AsyncOpen, we just have Redirect() call AsyncOpen on the new channel.  If not, we have it not open the new channel and call Open() ourselves in nsBaseChannel::Open.

Now the contract for Open() allows spinning the event loop until Open() returns (see NS_ImplementChannelOpen), so maybe that's what we should do here: do the redirect notification async, spin the event loop until we know whether it's ok to redirect, then call Open() on the post-redirect channel.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: wip1 for consideration only

This is just a work in progress. Please check the approach is correct (I believe we have to change the code more deeply, the patch is complex IMO).

Now I want to write test for code changes I made.

Comments welcome.

Comment 6

[Benjamin Smedberg]

Honza, ping? If this is to land in 1.9.3, it needs to be finished up and go through reviews!

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v0.9.1

First rc, I don't have tests only for nsHttpChannel::DoReplaceWithProxy, will work on it now. All the other code changes are covered by the tests. Going to push this to the try server to see if I broke something uncovered.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v0.9.2

And this contains fix for nsBaseChannel as bz suggests in comment 4.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v0.9.3

...and with fixed mRedirectType (fixed failing test_CrossSiteXHR.html).

Comment 10

[Benjamin Smedberg]

Comment on attachment 415010 [details] [diff] [review]
v0.9.3

I'm not the right sr for this: bz or jst are probably a better choice. But I will note that nsIAsyncVerifyRedirectCallback is completely undocumented, and I can't figure out how client code would actually use it. Don't you need to make changes/additions to nsIChannelEventSink?

Comment 11

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

Can you describe how this async redirect API is going to work with multiple redirect listeners?

Comment 12

[Honza Bambas (:mayhemer)]

To clarify:

This patch doesn't change the API, I mean any method signature. It turns just and only nsHttpHandler::OnChannelRedirect method to have asynchronous behavior. The main goal why the patch lies in bugzilla is to consider the channel code changes.

nsIAsyncVerifyRedirectCallback is implemented just and only by nsHttpChannel and it's method onRedirectVerifyCallback is called AFTER ALL sinks has been synchronously asked to decide. Sinks' implementation is not changed in this patch.

I just talked with Benjamin and he suggested that we probably should have a new method nsIChannelEventSink.onChannelRedirectAsync, having set of arguments as its sync version has but with an additional parameter taking nsIAsyncVerifyRedirectCallback reference. It's better then to force consumers (clients) to QI the old channel to this interface and, BTW, the callback implementation doesn't necessarily have to be the channel if we want all consumers implement this new async API. bz, biesi: opinions?

What is not contained in this patch but has to be done yet: 

nsAsyncRedirectVerifyHelper (actually the sink iterator) will call the new async API (or sync one when new is unimplemented) of all sinks in serial or parallel way (depends) and collect answers. Then finally will send the result to the channel via nsIAsyncVerifyRedirectCallback.

I have to hook also NSAPI calls. Bug 475991 seems to be dedicated to this API change.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm trying to follow comment 12....

The main point here should be that channel implementations, instead of calling redirect observers and getting back an rv immediately should call redirect observers and then get a callback from the observer, right?  Is that what nsIAsyncVerifyRedirectCallback gives us?

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to comment #13)
> I'm trying to follow comment 12....
> 
> The main point here should be that channel implementations, instead of calling
> redirect observers and getting back an rv immediately should call redirect
> observers and then get a callback from the observer, right?  Is that what
> nsIAsyncVerifyRedirectCallback gives us?

Yes, that is exactly what the patch does. nsHttpChannel calls nsHttpHandler::OnChannelRedirect, that immediately returns. nsHttpChannel then expects result of this single call to OnChannelRedirect through nsIAsyncVerifyRedirectCallback::OnRedirectVerifyCallback while the transaction (or cache) pump of the channel is suspended.

Comment 15

[Honza Bambas (:mayhemer)]

So, I have just finished the final patch for nsHttpChannel changes (as described in the previous comment). Going to push it to the try server.

We should probably split this bug to several ones.

Let this one be dedicated to preparation of nsHttpChannel for the asynchronous behavior of the redirect API. It is currently done and wait for the first regular review.

Benjamin, please state clearly what other requirements you have. There is one other that has resulted from our IRC chat: change the API (create a new one) to let any OnChannelRedirect implementation of any client behave asynchronously. This means that the decision will be made asynchronously withing the consumer (client) implementation, i.e. OnChannelRedirect might be left on the caller's thread before the decision has been made and the decision will be returned via callback (the new API we already have in the current patch of this bug).

Comment 16

[Benjamin Smedberg]

Honza, async OnChannelRedirect is the only requirement. It's not clear to me how the changes to nsHttpChannel here relate to that requirement, but that's ok with me as long as biesi/bz/jduell understand.

Comment 17

[Honza Bambas (:mayhemer)]

The http channel changes are necessary because we must not block the networking thread (process) while waiting for the response that might take a long time and may potentially involve other network requests.

Comment 18

[Honza Bambas (:mayhemer)]

BTW, do we want somehow to sanity cases when we don't get any answer from a client in some reasonable time?

Comment 19

[Benjamin Smedberg]

I don't know, but for now let's assume that clients will respond.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v1

Final version for the first regular review. All tests passing, no leaks.

Read comment 12 for description what this patch does and what doesn't.

Comment 21

[Benjamin Smedberg]

Why doesn't this block 475991? We can't implement that without the async redirect API in necko, right?

Comment 22

[Bjarne (:bjarne)]

I've been asked to finish up this by implementing the remaining issue described in the last part of comment #12. After reading comments and Honzas path I have some questions I'd like to clarify before continuing :

1) This bug is originally requesting an async version of the method nsIChannelEventSink::OnChannelRedirect(), right?

2) AFAICS, the current networking code (not Honzas patch) calls nsIChannelEventSink::OnChannelRedirect() from three (pertinent) places

  a) from nsIOService::OnChannelRedirect() on an object identified by NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID (nsScriptSecurityManager, I believe)

  b) from nsIOService::OnChannelRedirect() on a list of objects contained in a category - the first negative response will break the loop

  c) from nsHttpHandler::OnChannelRedirect() on an object described as "per-channel observer"

3) Do we want to allow all these (and possibly others) to be async?

For my general understanding :

4) nsIChannelEventSink::OnChannelRedirect() is essentially a mechanism to allow listeners/clients to veto the decision of doing the channel-redirect, right?

5) All clients must agree for the redirect to happen, right?

6) If a client has received the callback and responded OK, it still has to check whether the redirect actually happened, since there may be a client called later which vetoed, right?

Comment 23

[Benjamin Smedberg]

> 1) This bug is originally requesting an async version of the method
> nsIChannelEventSink::OnChannelRedirect(), right?

Yes.

> 3) Do we want to allow all these (and possibly others) to be async?

Yes, I think so, although the per-channel observer is the essential one.

> 4) nsIChannelEventSink::OnChannelRedirect() is essentially a mechanism to allow
> listeners/clients to veto the decision of doing the channel-redirect, right?

Notice and possibly veto, yes.

> 5) All clients must agree for the redirect to happen, right?

Yes.

> 6) If a client has received the callback and responded OK, it still has to
> check whether the redirect actually happened, since there may be a client
> called later which vetoed, right?

If somebody vetoed, the client would get an onStopRequest notification with a failure code. I don't think clients care whether the redirect was actually performed after that point.

Comment 24

[Bjarne (:bjarne)]

(In reply to comment #23)
> > 1) This bug is originally requesting an async version of the method
> > nsIChannelEventSink::OnChannelRedirect(), right?
> 
> Yes.

It may be feasible to make nsHttpHandler wrap each existing client to pass response-values asynchronously back to nsHttpChannel via a callback (IMO pretty much what the current patch does). Moreover, this can probably be done without changing existing clients. But am I right in assuming that the goal is really to add a method which clients know is asynchronous, so that they can take advantage of this fact?

In the context of electrolysis : Would clients typically be children or parents? Or would this depend entirely on the type of client?

> > 3) Do we want to allow all these (and possibly others) to be async?
> 
> Yes, I think so, although the per-channel observer is the essential one.

How come? I would intuitively expect the objects in the category to be more important in this respect...?

Comment 25

[Benjamin Smedberg]

There are two primary goals of this bug:
* plugins want to be able to observe and validate redirects. They would like to do so asynchronously
* with multi-process tabs, the child process is basically a necko client. It will want to observe redirects (because that's what the DOM does), and we cannot have the chrome process block waiting for the result of nsIChannelEventSink::OnChannelRedirect.

The fact of the matter is, NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID and the category will both only exist as they do today. Only the necko client (either the plugin host or the ContentProcessParent) will actually *need* this async API.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I've needed an asynchronous redirect API elsewhere too (specifically for CORS implementation). I also think the phishing protection code needs it too, so far it's used a more or less hacky workaround.

Comment 27

[Bjarne (:bjarne)]

As requested by Honza, bug #546606 has been created to address the remaining issue.

Comment 28

[Jason Duell]

Comment on attachment 422027 [details] [diff] [review]
v1

What's the status on this patch?  Honza, is it stale, or still just needs review?  If we can't get biesi/bz to review, can we at least get Bjarne to review for starters?

Comment 29

[Jason Duell]

So Honza says this patch is good.  We could use review--it's been waiting for 2 months.  Biesi?  Boris?

Comment 30

[Honza Bambas (:mayhemer)]

Attachment: v1 merged by Bjarne

Merged to the current m-c.  Thanks Bjarne for this merge.

Comment 31

[Jason Duell]

Comment on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

Let's have bjarne review, and then biesi for sr.  I don't think we're going to get bz any time soon.

Comment 32

[Jason Duell]

Comment on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

This patch adds a lot of separate test JS files, that seem to dupe a lot of boilerplate.  Might be easier to maintain if we could the test logic in fewer files, or at least have them share some included code?

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

I'm stealing this review at jduell's behest.

Comment 34

[Josh Matthews [:jdm]]

Comment on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

> + * The Initial Developer of the Original Code is
> + * Honza Bambas <[e-mail]>

Nit: I believe this should be the Mozilla Foundation, with your entry under
the contributors

> + #ifndef NSASYNCREDIRECTVERIFYHELPER_H__
> + #define NSASYNCREDIRECTVERIFYHELPER_H__

Nit: I believe the modern style is no trailing underscores, using nsInterCaps

> + // E10S: OnRedirectCallback has to be called on the original process
...
> + // E10S: This helper has to be initialized on the other process

Mark these TODO, please.  Possibly file a follow-up for them (and a later reference) ?

> nsAsyncRedirectVerifyHelper* redirectCallbackHelper = 
>      new nsAsyncRedirectVerifyHelper();
...
> +  if (NS_FAILED(rv)) {
> +    mWaitingForRedirectCallback = PR_FALSE;
>      return rv;
> +  }

The helper leaks here.

>      if (NS_SUCCEEDED(mStatus)) {
> +        PushRedirectAsyncFunc(&nsHttpChannel::ContinueHandleAsyncRedirect);
>          rv = ProcessRedirection(mResponseHead->Status());
>          if (NS_FAILED(rv)) {
> +            PopRedirectAsyncFunc(&nsHttpChannel::ContinueHandleAsyncRedirect);
> +            ContinueHandleAsyncRedirect(rv);
> +        }
> +    }
> +    else 
> +        ContinueHandleAsyncRedirect(rv);

Nit: Brace the else, please.

> @@ -1011,32 +1043,22 @@ nsHttpChannel::ProcessResponse()
> +        if (NS_FAILED(rv)) {
> +            PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse);
>              LOG(("ProcessRedirection failed [rv=%x]\n", rv));
> -            if (mTransaction->SSLConnectFailed())
> -                return ProcessFailedSSLConnect(httpStatus);
> -            rv = ProcessNormal();
> +            rv = ContinueProcessResponse(rv);

The "ProcessRedirection failed" message appears again in ContinueProcessResponse.  Let's get rid of one of them.

> +    if (NS_SUCCEEDED(rv)) {
> +        InitCacheEntry();
> +        CloseCacheEntry(PR_FALSE);
> +
> +        if (mCacheForOfflineUse) {
> +            // Store response in the offline cache
> +            InitOfflineCacheEntry();
> +            CloseOfflineCacheEntry();
> +        }
> +    return NS_OK;
> +    }
> +    else {
> +        LOG(("ProcessRedirection failed [rv=%x]\n", rv));
> +        if (mTransaction->SSLConnectFailed())
> +            return ProcessFailedSSLConnect(mRedirectType);
> +        return ProcessNormal();
> +    }
> +}

Nit: fix the indentation please.  Nix the else after return as well.

> +nsresult
> +
>  nsHttpChannel::ProcessNormal()

Nit: Extra newline.

+    if (mFallingBack) {
+        // Do not continue with normal processing, fallback is in
+        // progress now.
+        return NS_OK;
+}

Nit: indent, please.

>      rv = gHttpHandler->OnChannelRedirect(this, newChannel, redirectFlags);
> +    if (NS_FAILED(rv)) {
> +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessFallback);
> +        mRedirectChannel = nsnull;
> +        return rv;
> +    }
> +
> +    rv = WaitForRedirectCallback();
> +    NS_ENSURE_SUCCESS(rv, rv);

If rv != NS_OK here, should we go through the same failure steps as above?

> +    // Indicate we are now wating for redirect asynchronous callback

s/wating/waiting/
How about "Indicate we are now waiting for the asynchronous redirect callback"

>      rv = ioService->NewURI(nsDependentCString(location), originCharset.get(), mURI,
> -                           getter_AddRefs(newURI));
> +                getter_AddRefs(mRedirectURI));

Nit: indentation

> +nsresult
> +nsHttpChannel::ContinueProcessRedirection(nsresult rv)
...
> +
> +    InitCacheEntry();
> +    CloseCacheEntry(PR_FALSE);
> +
> +    if (mCacheForOfflineUse) {
> +        // Store response in the offline cache
> +        InitOfflineCacheEntry();
> +        CloseOfflineCacheEntry();
> +    }
> +

Should this actually be here?  This looks identical to the code that used to be part of ProcessResponse (and is now in ContinueProcessResponse), but I don't see a corresponding block removed from ProcessRedirection.

>      // on proxy errors, try to failover
>      if (mConnectionInfo->ProxyInfo() &&
> +       (mStatus == NS_ERROR_PROXY_CONNECTION_REFUSED ||
> +        mStatus == NS_ERROR_UNKNOWN_PROXY_HOST ||
> +        mStatus == NS_ERROR_NET_TIMEOUT)) {
> +        
> +        PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
>          if (NS_SUCCEEDED(ProxyFailover()))
>              return NS_OK;
> +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
> +    }
> +
> +    return ContinueOnStartRequest2(NS_OK);
> +}
> +
> +nsresult
> +nsHttpChannel::ContinueOnStartRequest1(nsresult result)
> +{
> +    // Success indicates we passed ProxyFailover, in that case we must not continue
> +    // with this code chain.
> +    if (NS_SUCCEEDED(result))
> +        return NS_OK;
> +
> +    return ContinueOnStartRequest2(result);
> +}

I find this ContinueOnStartRequestX section to be a bit of a mind-bender, so I may be off-base here.  Is ContinueOnStartRequest1 actually necessary?  As far as I can tell, there only a few scenarios:

1) We push ContinueOnStartRequest1, ProxyFailover() succeeds, we call ContinueOnStartRequest1 at a later point, it exits quickly.
2) We push ContinueOnStartRequest1, ProxyFailover() fails, we pop ContinueOnStartRequest1 and go to ContinueOnStartRequest2.
3) We push ContinueOnStartRequest1, ProxyFailover() succeeds, some other callbacks are called before we call ContinueOnStartRequest1, which ends up receiving a failing |result| as a parameter and proceeds to call ContinueOnStartRequest2.

3) is the case I'm having trouble picturing, so I'll rely on your judgement as to whether it's possible.  If it's not, it looks to me like we can remove ContinueOnStartRequest1, as it doesn't actually end up doing anything.

> +nsresult
> +nsHttpChannel::ContinueOnStartRequest2(nsresult result)
> +{
>      // on other request errors, try to fall back
> +
> +    if (NS_FAILED(mStatus)) {

Nit: nix the extra newline here

> +    NS_ASSERTION(mWatingForRedirectCallback, "Someone forgot to call WaitForRedirectCallback() ?!");

Nit: 80 chars?  I'm not sure what the house style is here.

> +    for (PRInt32 i = mRedirectFuncStack.Length()-1; i >= 0; --i) {

Nit: spaces around operators, please.
However, this will almost certainly generate signedness warnings.  Please make |i| a PRUint32 and use the idiom:

  for (PRUint32 i = mRedirectFuncStack.Length(); i > 0; ) {
    --i;
  }

> +        mRedirectFuncStack.RemoveElementAt(mRedirectFuncStack.Length()-1);

Nit: unsquish the operator.

> +        // Check if we are not again in the wait-for-redirect state, some
> +        // other function was pushed to the stack at this moment, so break
> +        // the iteration and wait for the callback again.

How about: "If a new function has been pushed to the stack and placed us in the waiting state, we need to break the chain and wait for the callback again."

> +        // Must always resume pumps to:
> +        // 1. let OnStopRequest be triggered
> +        // 2. in case we still wait for this callback we have just suspended
> +        // both pums and we must balance the suspension count.

s/pums/pumps/
How about: "We must resume the pumps to let OnStopRequest be triggered, or in case we're waiting for a suspended callback".
Also: "The suspension count must be balanced with the pumps."

> +    NS_ASSERTION(func == mRedirectFuncStack[mRedirectFuncStack.Length()-1],

Nit: operator squeeze.

> +    mRedirectFuncStack.TruncateLength(mRedirectFuncStack.Length()-1);

Here also.

> --- a/netwerk/protocol/http/src/nsHttpHandler.cpp	
> +++ a/netwerk/protocol/http/src/nsHttpHandler.cpp

> +    PRUint32                          mWatingForRedirectCallback: 1;

s/mWatingForRedirectCallback/mWaitingForRedirectCallback/

> +    // E10S: This helper has to be initialized on the other process

Nit: Another TODO and possible followup bug addition.

> --- a/netwerk/test/unit/head_channels.js	
> +++ a/netwerk/test/unit/head_channels.js

> -
> +  

Nit: Extraneous whitespace change.

> +          do_throw("Could not get contentLength while not expecting failure");

Nit: The double negatives hinder comprehension, in my opinion. How about removing the "while not expecting failure" bit?

> +ChannelEventSink.prototype = {
> +  QueryInterface: function(iid) {
> +    dump("--- ChannelEventSink.QueryInterface ---\n");
> +    if (iid.equals(Components.interfaces.nsIInterfaceRequestor) ||
> +        iid.equals(Components.interfaces.nsISupports))
> +      return this;
> +    dump("--- ChannelEventSink.QueryInterface threw... ---\n");
> +    throw Components.results.NS_ERROR_NO_INTERFACE;
> +  },
> +  
> +  getInterface: function(iid) {
> +    dump("--- ChannelEventSink.getInterface ---\n");
> +    if (iid.equals(Components.interfaces.nsIChannelEventSink))
> +      return this;
> +    dump("--- ChannelEventSink.getInterface threw... ---\n");
> +    throw Components.results.NS_ERROR_NO_INTERFACE;
> +  },
> +  
> +  onChannelRedirect: function(oldChannel, newChannel, flags) {
> +    dump("--- ChannelEventSink.onChannelRedirect ---\n");
> +    if (this._flags & ES_ABORT_REDIRECT) {
> +      dump("--- ChannelEventSink.onChannelRedirect aborting! ---\n");
> +      throw Components.results.NS_BINDING_ABORTED;
> +    }
> +    dump("--- ChannelEventSink.onChannelRedirect passed ---\n");
> +  }  
> +};

Nit: make all of the Components.* Ci or Cr as appropriate?  Also, can we lose the dump() statements, or do they serve a useful purpose?

> --- a/netwerk/test/unit/test_fallback_no-cache-entry_canceled.js
> +++ a/netwerk/test/unit/test_fallback_no-cache-entry_canceled.js

> +var httpserver = null;
> +var cacheupdateobserver = null;

Nit: mixedCase names please?

> +  cacheupdateobserver = {observe: function() {

Ditto.

I agree with jduell; I'd really like to see some of the common test boilerplate collected in one location.  The above naming nits apply to every JS test.

> --- a/netwerk/test/unit/test_proxy-failover_canceled.js
> +++ a/netwerk/test/unit/test_proxy-failover_canceled.js

> +  var os = Cc["@mozilla.org/observer-service;1"].
> +           getService(Ci.nsIObserverService);
> +  os.addObserver(modifyrequestobserver, "http-on-modify-request", false);
> +  
> +  

Nit: nix the extra newline.

r=me with nits scratched and issues/questions addressed.

Comment 35

[Bjarne (:bjarne)]

Attachment: V1.1 unbitrotted and addressing comments by reviewer...

(In reply to comment #34)
> (From update of attachment 435277 [details] [diff] [review])
> > + * The Initial Developer of the Original Code is
> > + * Honza Bambas <[e-mail]>
> 
> Nit: I believe this should be the Mozilla Foundation, with your entry under
> the contributors

Ok.

> 
> > + #ifndef NSASYNCREDIRECTVERIFYHELPER_H__
> > + #define NSASYNCREDIRECTVERIFYHELPER_H__
> 
> Nit: I believe the modern style is no trailing underscores, using nsInterCaps

Assuming I understand you correctly : fixed.

> > + // E10S: OnRedirectCallback has to be called on the original process
> ...
> > + // E10S: This helper has to be initialized on the other process
> 
> Mark these TODO, please.  Possibly file a follow-up for them (and a later
> reference) ?

Marked TODO. Will be followed up in part 2 (bug #546606)

> > nsAsyncRedirectVerifyHelper* redirectCallbackHelper = 
> >      new nsAsyncRedirectVerifyHelper();
> ...
> > +  if (NS_FAILED(rv)) {
> > +    mWaitingForRedirectCallback = PR_FALSE;
> >      return rv;
> > +  }
> 
> The helper leaks here.

Yup. Deleting the helper in if-block.

Btw, wouldn't it also leak in nsHttpHandler::OnChannelRedirect()? Added code to handle it here also...

> >      if (NS_SUCCEEDED(mStatus)) {
> > +        PushRedirectAsyncFunc(&nsHttpChannel::ContinueHandleAsyncRedirect);
> >          rv = ProcessRedirection(mResponseHead->Status());
> >          if (NS_FAILED(rv)) {
> > +            PopRedirectAsyncFunc(&nsHttpChannel::ContinueHandleAsyncRedirect);
> > +            ContinueHandleAsyncRedirect(rv);
> > +        }
> > +    }
> > +    else 
> > +        ContinueHandleAsyncRedirect(rv);
> 
> Nit: Brace the else, please.

Ok.
> > @@ -1011,32 +1043,22 @@ nsHttpChannel::ProcessResponse()
> > +        if (NS_FAILED(rv)) {
> > +            PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse);
> >              LOG(("ProcessRedirection failed [rv=%x]\n", rv));
> > -            if (mTransaction->SSLConnectFailed())
> > -                return ProcessFailedSSLConnect(httpStatus);
> > -            rv = ProcessNormal();
> > +            rv = ContinueProcessResponse(rv);
> 
> The "ProcessRedirection failed" message appears again in
> ContinueProcessResponse.  Let's get rid of one of them.

Ok.

> > +    if (NS_SUCCEEDED(rv)) {
> > +        InitCacheEntry();
> > +        CloseCacheEntry(PR_FALSE);
> > +
> > +        if (mCacheForOfflineUse) {
> > +            // Store response in the offline cache
> > +            InitOfflineCacheEntry();
> > +            CloseOfflineCacheEntry();
> > +        }
> > +    return NS_OK;
> > +    }
> > +    else {
> > +        LOG(("ProcessRedirection failed [rv=%x]\n", rv));
> > +        if (mTransaction->SSLConnectFailed())
> > +            return ProcessFailedSSLConnect(mRedirectType);
> > +        return ProcessNormal();
> > +    }
> > +}
> 
> Nit: fix the indentation please.  Nix the else after return as well.

Ok.

> > +nsresult
> > +
> >  nsHttpChannel::ProcessNormal()
> 
> Nit: Extra newline.

Ok.

> +    if (mFallingBack) {
> +        // Do not continue with normal processing, fallback is in
> +        // progress now.
> +        return NS_OK;
> +}
> 
> Nit: indent, please.
> >      rv = gHttpHandler->OnChannelRedirect(this, newChannel, redirectFlags);
> > +    if (NS_FAILED(rv)) {
> > +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessFallback);
> > +        mRedirectChannel = nsnull;
> > +        return rv;
> > +    }
> > +
> > +    rv = WaitForRedirectCallback();
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> If rv != NS_OK here, should we go through the same failure steps as above?

Resetting mRedirectChannel and returning rv if (rv != NS_OK)

> > +    // Indicate we are now wating for redirect asynchronous callback
> 
> s/wating/waiting/
> How about "Indicate we are now waiting for the asynchronous redirect callback"

Ok.

> >      rv = ioService->NewURI(nsDependentCString(location), originCharset.get(), mURI,
> > -                           getter_AddRefs(newURI));
> > +                getter_AddRefs(mRedirectURI));
> 
> Nit: indentation

Ok.

> > +nsresult
> > +nsHttpChannel::ContinueProcessRedirection(nsresult rv)
> ...
> > +
> > +    InitCacheEntry();
> > +    CloseCacheEntry(PR_FALSE);
> > +
> > +    if (mCacheForOfflineUse) {
> > +        // Store response in the offline cache
> > +        InitOfflineCacheEntry();
> > +        CloseOfflineCacheEntry();
> > +    }
> > +
> 
> Should this actually be here?  This looks identical to the code that used to be
> part of ProcessResponse (and is now in ContinueProcessResponse), but I don't
> see a corresponding block removed from ProcessRedirection.
> 

I agree. Code removed from ContinueProcesRedirection. Honza : Any comments..?

> >      // on proxy errors, try to failover
> >      if (mConnectionInfo->ProxyInfo() &&
> > +       (mStatus == NS_ERROR_PROXY_CONNECTION_REFUSED ||
> > +        mStatus == NS_ERROR_UNKNOWN_PROXY_HOST ||
> > +        mStatus == NS_ERROR_NET_TIMEOUT)) {
> > +        
> > +        PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
> >          if (NS_SUCCEEDED(ProxyFailover()))
> >              return NS_OK;
> > +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
> > +    }
> > +
> > +    return ContinueOnStartRequest2(NS_OK);
> > +}
> > +
> > +nsresult
> > +nsHttpChannel::ContinueOnStartRequest1(nsresult result)
> > +{
> > +    // Success indicates we passed ProxyFailover, in that case we must not continue
> > +    // with this code chain.
> > +    if (NS_SUCCEEDED(result))
> > +        return NS_OK;
> > +
> > +    return ContinueOnStartRequest2(result);
> > +}
> 
> I find this ContinueOnStartRequestX section to be a bit of a mind-bender, so I
> may be off-base here.  Is ContinueOnStartRequest1 actually necessary?  As far
> as I can tell, there only a few scenarios:
> 
> 1) We push ContinueOnStartRequest1, ProxyFailover() succeeds, we call
> ContinueOnStartRequest1 at a later point, it exits quickly.
> 2) We push ContinueOnStartRequest1, ProxyFailover() fails, we pop
> ContinueOnStartRequest1 and go to ContinueOnStartRequest2.
> 3) We push ContinueOnStartRequest1, ProxyFailover() succeeds, some other
> callbacks are called before we call ContinueOnStartRequest1, which ends up
> receiving a failing |result| as a parameter and proceeds to call
> ContinueOnStartRequest2.
> 
> 3) is the case I'm having trouble picturing, so I'll rely on your judgement as
> to whether it's possible.  If it's not, it looks to me like we can remove
> ContinueOnStartRequest1, as it doesn't actually end up doing anything.
> 

AFAICS, a failing |result| param to ContinueOnStartRequest1() could for example
come from the nsIChannelEventSink if it vetos the redirect. So, I think it must
be kept. Honza...?
> > +nsresult
> > +nsHttpChannel::ContinueOnStartRequest2(nsresult result)
> > +{
> >      // on other request errors, try to fall back
> > +
> > +    if (NS_FAILED(mStatus)) {
> 
> Nit: nix the extra newline here

Ok.

> > +    NS_ASSERTION(mWatingForRedirectCallback, "Someone forgot to call WaitForRedirectCallback() ?!");
> 
> Nit: 80 chars?  I'm not sure what the house style is here.

Fixed.

> > +    for (PRInt32 i = mRedirectFuncStack.Length()-1; i >= 0; --i) {
> 
> Nit: spaces around operators, please.
> However, this will almost certainly generate signedness warnings.  Please make
> |i| a PRUint32 and use the idiom:
> 
>   for (PRUint32 i = mRedirectFuncStack.Length(); i > 0; ) {
>     --i;
>   }

Very well... fixed.

> 
> > +        mRedirectFuncStack.RemoveElementAt(mRedirectFuncStack.Length()-1);
> 
> Nit: unsquish the operator.

Assuming I understand the terminology : fixed (spaces around the hyphen)

> 
> > +        // Check if we are not again in the wait-for-redirect state, some
> > +        // other function was pushed to the stack at this moment, so break
> > +        // the iteration and wait for the callback again.
> 
> How about: "If a new function has been pushed to the stack and placed us in the
> waiting state, we need to break the chain and wait for the callback again."

Replaced.

> > +        // Must always resume pumps to:
> > +        // 1. let OnStopRequest be triggered
> > +        // 2. in case we still wait for this callback we have just suspended
> > +        // both pums and we must balance the suspension count.
> 
> s/pums/pumps/
> How about: "We must resume the pumps to let OnStopRequest be triggered, or in
> case we're waiting for a suspended callback".
> Also: "The suspension count must be balanced with the pumps."

Rephrased. Honza : Ok?

> 
> > +    NS_ASSERTION(func == mRedirectFuncStack[mRedirectFuncStack.Length()-1],
> 
> Nit: operator squeeze.

Assuming I understand the terminology : fixed (spaces around the hyphen)

> 
> > +    mRedirectFuncStack.TruncateLength(mRedirectFuncStack.Length()-1);
> 
> Here also.

Assuming I understand the terminology : fixed (spaces around the hyphen)

> 
> > --- a/netwerk/protocol/http/src/nsHttpHandler.cpp   
> > +++ a/netwerk/protocol/http/src/nsHttpHandler.cpp
> 
> > +    PRUint32                          mWatingForRedirectCallback: 1;
> 
> s/mWatingForRedirectCallback/mWaitingForRedirectCallback/

Ok.

> 
> > +    // E10S: This helper has to be initialized on the other process
> 
> Nit: Another TODO and possible followup bug addition.

Marked TODO. Will be followed up in part 2 (bug #546606)

> > --- a/netwerk/test/unit/head_channels.js    
> > +++ a/netwerk/test/unit/head_channels.js
> 
> > -
> > +  
> 
> Nit: Extraneous whitespace change.

Ok.
> 
> > +          do_throw("Could not get contentLength while not expecting failure");
> 
> Nit: The double negatives hinder comprehension, in my opinion. How about
> removing the "while not expecting failure" bit?

Rephrased. Honza : Ok?

> > +ChannelEventSink.prototype = {
> > +  QueryInterface: function(iid) {
> > +    dump("--- ChannelEventSink.QueryInterface ---\n");
> > +    if (iid.equals(Components.interfaces.nsIInterfaceRequestor) ||
> > +        iid.equals(Components.interfaces.nsISupports))
> > +      return this;
> > +    dump("--- ChannelEventSink.QueryInterface threw... ---\n");
> > +    throw Components.results.NS_ERROR_NO_INTERFACE;
> > +  },
> > +  
> > +  getInterface: function(iid) {
> > +    dump("--- ChannelEventSink.getInterface ---\n");
> > +    if (iid.equals(Components.interfaces.nsIChannelEventSink))
> > +      return this;
> > +    dump("--- ChannelEventSink.getInterface threw... ---\n");
> > +    throw Components.results.NS_ERROR_NO_INTERFACE;
> > +  },
> > +  
> > +  onChannelRedirect: function(oldChannel, newChannel, flags) {
> > +    dump("--- ChannelEventSink.onChannelRedirect ---\n");
> > +    if (this._flags & ES_ABORT_REDIRECT) {
> > +      dump("--- ChannelEventSink.onChannelRedirect aborting! ---\n");
> > +      throw Components.results.NS_BINDING_ABORTED;
> > +    }
> > +    dump("--- ChannelEventSink.onChannelRedirect passed ---\n");
> > +  }  
> > +};
> 
> Nit: make all of the Components.* Ci or Cr as appropriate?  Also, can we lose
> the dump() statements, or do they serve a useful purpose?

Using Ci and Cr. Dump-statements removed.

> > --- a/netwerk/test/unit/test_fallback_no-cache-entry_canceled.js
> > +++ a/netwerk/test/unit/test_fallback_no-cache-entry_canceled.js
> 
> > +var httpserver = null;
> > +var cacheupdateobserver = null;
> 
> Nit: mixedCase names please?

Ok.

> > +  cacheupdateobserver = {observe: function() {
> 
> Ditto.

Ok.

> I agree with jduell; I'd really like to see some of the common test boilerplate
> collected in one location.  The above naming nits apply to every JS test.

I'm working on it...

> > --- a/netwerk/test/unit/test_proxy-failover_canceled.js
> > +++ a/netwerk/test/unit/test_proxy-failover_canceled.js
> 
> > +  var os = Cc["@mozilla.org/observer-service;1"].
> > +           getService(Ci.nsIObserverService);
> > +  os.addObserver(modifyrequestobserver, "http-on-modify-request", false);
> > +  
> > +  
> 
> Nit: nix the extra newline.

Ok.

Comment 36

[Honza Bambas (:mayhemer)]

I'll attach a new patch soon.  Currently I pushed both (mine and Bjarne's) patches to the try server to see the leaks.  I remember just a single leak present in an older version of the patch that I have fixed.  It used to be ok...

(In reply to comment #35)
> > > nsAsyncRedirectVerifyHelper* redirectCallbackHelper = 
> > >      new nsAsyncRedirectVerifyHelper();
> > ...
> > > +  if (NS_FAILED(rv)) {
> > > +    mWaitingForRedirectCallback = PR_FALSE;
> > >      return rv;
> > > +  }
> > 
> > The helper leaks here.
> 
> Yup. Deleting the helper in if-block.
> 

Better use nsRefPtr?

> > If rv != NS_OK here, should we go through the same failure steps as above?
> 
> Resetting mRedirectChannel and returning rv if (rv != NS_OK)
> 

You also have to pop the method.

> > Should this actually be here?  This looks identical to the code that used to be
> > part of ProcessResponse (and is now in ContinueProcessResponse), but I don't
> > see a corresponding block removed from ProcessRedirection.
> > 
> 
> I agree. Code removed from ContinueProcesRedirection. Honza : Any comments..?
> 

Agree, tests passes w/o it, and I don't remember any reason it should be there and there is apparantly none. Ok to remove it.

> > I find this ContinueOnStartRequestX section to be a bit of a mind-bender, so I
> > may be off-base here.  Is ContinueOnStartRequest1 actually necessary?  As far
> > as I can tell, there only a few scenarios:
>
> AFAICS, a failing |result| param to ContinueOnStartRequest1() could for example
> come from the nsIChannelEventSink if it vetos the redirect. So, I think it must
> be kept. Honza...?

Yes, this has to be kept as is.  I'll try to add a meaningful comment.

> > > +    for (PRInt32 i = mRedirectFuncStack.Length()-1; i >= 0; --i) {
> > 
> > Nit: spaces around operators, please.
> > However, this will almost certainly generate signedness warnings.  Please make
> > |i| a PRUint32 and use the idiom:
> > 
> >   for (PRUint32 i = mRedirectFuncStack.Length(); i > 0; ) {
> >     --i;
> >   }
> 

The patch has originally been build for nsVoidArray that used PRInt32, thanks for this catch.

> > s/pums/pumps/
> > How about: "We must resume the pumps to let OnStopRequest be triggered, or in
> > case we're waiting for a suspended callback".
> > Also: "The suspension count must be balanced with the pumps."
> 
> Rephrased. Honza : Ok?
> 

Your comment is perfect, Bjarne.

> > > +          do_throw("Could not get contentLength while not expecting failure");
> > 
> > Nit: The double negatives hinder comprehension, in my opinion. How about
> > removing the "while not expecting failure" bit?
> 
> Rephrased. Honza : Ok?
> 

Agree, you forget to add this rephrase to your patch ;)

Comment 37

[Honza Bambas (:mayhemer)]

Confirming that both patches are leaking:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272052196.1272062692.15381.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272052196.1272062321.14257.gz#err14
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272052228.1272059786.6245.gz#err9

Comment 38

[Honza Bambas (:mayhemer)]

Leaks reproducible with content/base/test/test_CrossSiteXHR.html: 10 nsHttpChannel instances leaking, going to look at this tomorrow (Saturday).  If anyone would be going to analyze this, please make a note to this bug ;)

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: v1.2

ATM waiting for the try server result.  Locally seems that are no leaks.

The cause was that I was not releasing mRedirectChannel member after the callback result was failure.

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 441620 [details] [diff] [review]
v1.2

This patch is not passing try server.  I wait for another cycle with an adjusted patch.  If ok, I will submit it.

Comment 41

[Honza Bambas (:mayhemer)]

Attachment: v1.3

Passed try server build except some random failure of browser_privatebrowsing_beforeunload.js test that is uninvolved in the redirection code and that I am unable to reproduce locally.

Comment 42

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

Attachment: review comments

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 443950 [details]
review comments

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>+    // Set mIsPending to PR_FALSE here. At this moment we are done from
>+    // the point of view of our consumer and we have to report our self
>+    // as not-pending.
>+    mIsPending = PR_FALSE;
>
>Why are you doing this in DoNotifyListener now?

I have added a test that covers this code now (there was non before) and is expecting (by the contract) that the request is not pending while calling OnStopRequets of its consumer.  The new test (that MUST succeed even w/o this whole patch, i.e. in either sync or async version of the redirect API) is failing w/o this change.

>+nsHttpChannel::ContinueProcessNormal(nsresult rv)
>+{
>+    if (NS_FAILED(rv)) {
>+        // Fill the failure status here, we have failed to fall back, thus we
>+        // have to report our status as failed.
>+        mStatus = rv;
>
>Why wasn't the mStatus=rv line necessary before?

It was, but there was not any test that would uncover it.  I have added a test that covers this piece of code and it doesn't work w/o this mStatus = rv.  It expects the request to fail but it doesn't report itself that way.  The test fails w/o this change even w/o the whole patch.

>+nsHttpChannel::ContinueHandleAsyncReplaceWithProxy(nsresult status)
>+{
>+    if (mLoadGroup && NS_SUCCEEDED(status)) {
>+        mLoadGroup->RemoveRequest(this, nsnull, mStatus);
>+    }
>+    else if (NS_FAILED(status)) {
>+       AsyncAbort(status);
>+    }
>+    return status;
>
>You should return NS_OK here, right?

Good catch, if it is because we don't want the function stack logic call Cancel directly (after we invoked AsyncAbort), however, test didn't find any problems with this.

>+        if (!NS_SecurityCompareURIs(mURI, mRedirectURI, PR_FALSE)) {
>+            PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessRedirectionAfterFallback);
>+            rv = ProcessFallback();
>+            if (NS_ERROR_IN_PROGRESS == rv)
>                 return NS_OK;
>
>Don't you want to do something if ProcessFallback returns an error?

Hmmm... The original code throws the rv away if it is a failure (we just don't fallback).  If we should do something, what I don't think, then it is a different bug.

>+    if (!mWaitingForRedirectCallback) {
>+        // We are not waiting for the callback but handles didn't return
>+        // a failure state.  At this moment we must release reference to
>+        // the redirect target channel, otherwise we may leak.
>+        mRedirectChannel = nsnull;
>+    }
>
>Why are you saying "but handles didn't return a failure state."? First, I don't
>see how that is even true at this point, and second, wouldn't you want to
>release the channel anyway? Maybe you also want to mention that mRedirectChannel
>is only required for the redirect function.

I see, comment doesn't make sense, quote removed. But, I have to leave mRedirectChannel intact when we are waiting for the redirect callback again. Maybe I don't fully understand your question 'wouldn't you want to release the channel anyway?'

>
>+    if (mTransactionPump)
>+        mTransactionPump->Resume();
>+    else if (mCachePump)
>+        mCachePump->Resume();
>
>Are you sure that only one pump can be non-null here? I think the combination of
>PAC/proxy fallback and resuming a partial cache entry makes it possible for both
>to be valid here.

Thanks for this catch. Another question is if during the time we wail for the redirect callback could one of the transactions become null or in contrary non-null (in the former case it would not be resumed and it the letter case it would be resumed more times then suspended). Then I would have to remember which transaction had been suspended by WaitForRedirectCallback specifically.

Comment 44

[Honza Bambas (:mayhemer)]

Attachment: v1.4

Fixed the failing onbeforeunload test (privately approved by mak).  Interdiff follows.

Comment 45

[Honza Bambas (:mayhemer)]

Attachment: 1.3 -> 1.4 diff

Comment 46

[Honza Bambas (:mayhemer)]

And a note: I removed call to GetCallback(nsIChannelEventSink)->OnChannelRedirect from nsBaseChannel::Redirect (ContinueRedirect).  It is, and always was, duplicated by calling on sink gained by call to NS_QueryNotificationCallbacks(oldChannel) in the HttpHandler.  It does GetInterface(nsIChannelEventSink) first on mCallbacks and then on mLoadGroup's mCallbacks if not found.  None of the channels inheriting nsBaseChannel overrides its GetInterface.

Comment 47

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

(In reply to comment #43)
> I see, comment doesn't make sense, quote removed. But, I have to leave
> mRedirectChannel intact when we are waiting for the redirect callback again.
> Maybe I don't fully understand your question 'wouldn't you want to release the
> channel anyway?'

Sorry, the 'wouldn't you want to release the channel anyway?' was just part of the explanation why the comment is wrong. Irrelevant now that it's removed.

> Thanks for this catch. Another question is if during the time we wail for the
> redirect callback could one of the transactions become null or in contrary
> non-null (in the former case it would not be resumed and it the letter case it
> would be resumed more times then suspended). Then I would have to remember
> which transaction had been suspended by WaitForRedirectCallback specifically.

I don't think that's possible... they should only become null during pump callbacks, and only non-null during channel setup/fallback. So that should be safe unless I'm missing something now.

Comment 48

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

Comment on attachment 445609 [details] [diff] [review]
v1.4

+++ b/browser/components/places/tests/browser/browser_library_middleclick.js

This looks like an unrelated change to me.

+++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp

The synchronous case here seems fairly complicated for no good reason. Why don't you refactor this code a bit:

Add a function CallObservers() which calls the ioservice and the channel observer and returns rv.

nsAsyncRedirectVerifyHelper::Run can then basically be:
  Callback(CallObservers());
  return NS_OK;

Then ::Init can basically be:
  if (synchronous) {
    callback->OnRedirectVerifyCallback(CallObservers());
    return NS_OK;
  }

No need to pump events here. Pumping events inside of asyncOpen doesn't sound like a terribly good idea to me in any case.

+++ b/netwerk/base/src/nsBaseChannel.cpp
+  nsresult rv = redirectCallbackHelper->Init(this, newChannel, redirectFlags, checkRedirectSynchronously);

80 chars per line, please

+  // XXX Is our http channel implementation going to derive from nsBaseChannel?
+  //     If not, this code can be removed.

Hopefully it will, some day :/

+      rv = Redirect(newChannel,
                              nsIChannelEventSink::REDIRECT_TEMPORARY,
                              PR_TRUE);

please fix this indentation

-          doNotify = PR_FALSE;
+      if (NS_SUCCEEDED(rv))
+          return;

Can you add a comment here "OnRedirectVerifyCallback will be called asynchronously"

--
Sorry, I didn't manage to finish before the meeting. Will continue later today.

Comment 49

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

Comment on attachment 445609 [details] [diff] [review]
v1.4

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+        if (waitingForRedirectCallback)
+            // The transaction has been suspended by ProcessFallback.
             return NS_OK;

Please put {} around multi-line if bodies, even when they're just one statement.

Comment 50

[Honza Bambas (:mayhemer)]

(In reply to comment #48)
> (From update of attachment 445609 [details] [diff] [review])
> +++ b/browser/components/places/tests/browser/browser_library_middleclick.js
> 
> This looks like an unrelated change to me.

This test sometimes leaves an openned tab that breaks another test running
later.  The redirect API change makes that happen more offten.

The flaw is in browser_library_middleclick.js test, should I fix it in a
different bug?

> 
> +++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp
> 
> The synchronous case here seems fairly complicated for no good reason. Why
> don't you refactor this code a bit:
> 

I understand, but we will need to pump events after we make the API itself
asynchronous (the second part of this bug, bug 546606).  Maybe bjarne working
on that bug should include this change in his patch rather?

This will never be called from AsyncOpen!  It is only called from
nsBaseChannel::Open where a redirect confirmation is needed and must be
synchronous from nature of Open().  Maybe add a comment not to use syncing when
called from AsyncOpen?

It seems to me simpler to add this code here then to mess with nsBaseChannel
and I am sure there were a reason to put it here (I don't recall what ATM).


I'll fix the remaining nits.

Comment 51

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

(In reply to comment #50)
> (In reply to comment #48)
> > (From update of attachment 445609 [details] [diff] [review] [details])
> > +++ b/browser/components/places/tests/browser/browser_library_middleclick.js
> > 
> > This looks like an unrelated change to me.
> 
> This test sometimes leaves an openned tab that breaks another test running
> later.  The redirect API change makes that happen more offten.
> 
> The flaw is in browser_library_middleclick.js test, should I fix it in a
> different bug?

I'd prefer if you did it in a different bug, reviewed by a browser peer, yes.

> I understand, but we will need to pump events after we make the API itself
> asynchronous (the second part of this bug, bug 546606).  Maybe bjarne working
> on that bug should include this change in his patch rather?

Hmm, yeah, ok. Seems OK to leave it in here then.

> This will never be called from AsyncOpen!

Sorry, you are, of course, correct.

Comment 52

[Honza Bambas (:mayhemer)]

(In reply to comment #51)
> I'd prefer if you did it in a different bug
Reported bug 571522.

Comment 53

[Honza Bambas (:mayhemer)]

Attachment: v1.5

(In reply to comment #48)
> +      rv = Redirect(newChannel,
>                               nsIChannelEventSink::REDIRECT_TEMPORARY,
>                               PR_TRUE);
> 
> please fix this indentation

The bugzilla diff just displays this wrong.

Comment 54

[Honza Bambas (:mayhemer)]

Attachment: v1.4 -> 1.5 interdiff

Comment 55

[Honza Bambas (:mayhemer)]

> The bugzilla diff just displays this wrong.
No, it was wrong but I was looking into an already merged local patch where this was already fixed, my apologies to bugzilla.

Comment 56

[Jason Duell]

Comment on attachment 450834 [details] [diff] [review]
v1.5

This patch has bitrotted--please update when you have the chance.

Comment 57

[Honza Bambas (:mayhemer)]

Attachment: v1.5 unbitrotted

Comment 58

[Benjamin Smedberg]

This is an extension-visible API change which needs to block beta2.

Comment 59

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Initial developer is the Mozilla Foundation: <http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html>

Comment 60

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

Comment on attachment 453774 [details] [diff] [review]
v1.5 unbitrotted

diff --git a/netwerk/base/public/nsAsyncRedirectVerifyHelper.h b/netwerk/base/public/nsAsyncRedirectVerifyHelper.h
+     * decision is passed through this inreface back to the oldChannel.

inreface -> interface

+++ b/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp
+    nsCOMPtr<nsIAsyncVerifyRedirectCallback> callback(do_QueryInterface(mOldChan));
+    if (callback)
+        callback->OnRedirectVerifyCallback(result);

I think you should add an NS_ASSERTION for the case where this QI fails. That should never happen, right?

Comment 61

[Honza Bambas (:mayhemer)]

So, this patch seems to cause following tests to fail:
- chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js | Found an unexpected tab at the end of test run: about:blank
- chrome://mochikit/content/browser/browser/base/content/test/browser_NetworkPrioritizer.js | Window has expected number of tabs - Got 3, expected 2

Looks like a tab is again left open in some test previous to these.  Will look at that tomorrow.

Comment 62

[Honza Bambas (:mayhemer)]

For reference:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279143439.1279144438.11569.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279144745.1279146263.19275.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279144206.1279145035.14235.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279145306.1279147288.24501.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279150990.1279151604.11344.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279146810.1279148499.30164.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279145646.1279147899.27180.gz

Comment 63

[Honza Bambas (:mayhemer)]

browser_privatebrowsing_openlocation.js failure:

- apply the patch
- run test browser_privatebrowsing_newwindow_stopcmd.js
- it switches to PB mode and opens a new browser window
- after it loads, the test closes that new browser window and switches back to non-private browsing mode
- only the original window is open and active, there are no tabs, only the [+] "tab" to add a new tab
- but, the content of this "null" tab gets http://www.mozilla.org/projects/firefox/prerelease.html URI to load; it's filled in the URL bar (that I would expect not to be visible at this situation)
- this URI is a redirect from http://www.mozilla.org/projects/firefox/4.0b2pre/firstrun/ that any new browser window loads as a default page
- browser_privatebrowsing_openlocation.js "before run check" finds a tab that should not be there... => failure
- curious is that there is one more test between browser_privatebrowsing_newwindow_stopcmd.js and browser_privatebrowsing_openlocation.js



I don't know the cause, but this seems to be serious.

I suspect failure of browser_NetworkPrioritizer.js is caused by the same issue as it is run as the first test just after the browser window opens but it's more a guess.

Comment 64

[Honza Bambas (:mayhemer)]

Some notes:
- not 100% reproducible
- I see this assertion: ###!!! ASSERTION: Wrong Document Channel: 'request == mDocumentRequest', file d:/mozilla/mozilla-central/uriloader/base/nsDocLoader.cpp, line 1594;  seems to indicate the cause

Comment 65

[Honza Bambas (:mayhemer)]

OK, nsDocLoader gets OnChannelRedirect after it has already been canceled as well as the old channel itself (doc loader's mDocumentRequest is null).  That's why the assertion fails.

However, we continue loading the redirect result request - the new channel, this is the behavior (w/ and w/o my patch).  Only problem is that doc loader could not get canceled w/o redirect API being asynchronous => canceling the doc loader would cancel the new channel.

So, the new channel continues to load and takes the original listener of the old channel that consumes the content.  I don't understand why the content ends up in an empty unrelated tab, is it a default target for the content?

To solve this we may:
1. let doc loader veto the redirect when the doc loader has been canceled, or
2. let the channel check its context (loadgroup?) it has not been canceled and if so, cancel the redirect internally as it would be vetoed

I more like the second option, also after we make the API fully async this will be more accurate.  Question: could the loadgroup or doc loader be re-started again with a different request before we get the redirect result?

Comment 66

[Boris Zbarsky [:bzbarsky]]

#2 sounds better to me, since it preserves more of the existing API guarantees...

> could the loadgroup or doc loader be re-started again with a different request
> before we get the redirect result?

Probably yes.  In fact, in many cases the Cancel() on the docloader immediately precedes AsyncOpen on a new document channel.

Comment 67

[Honza Bambas (:mayhemer)]

Attachment: v1.6 interdiff

This is an addition to fix the failing tests.  Passed try server with full green, local testing shows the same.

BZ, could you please check this change?  It's quit simple, actually what's described in comment 65 and agreed in comment 66, thank you.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Why does it matter what the loadgroup is doing?  Shouldn't canceling the channel cancel the redirect no matter what?

Comment 69

[Honza Bambas (:mayhemer)]

Attachment: v1.6 interdiff 2

I was only trying to preserve most of the previous behavior.  However, there is not any sink that would cancel the channel and along with it would return NS_OK.  So, just set the redirect result to a failure when the channel has been canceled seems to be OK.

What was a larger problem: the assertion from comment 64 was still there with the previous patch.  Doc loader was getting OnChannelRedirect for a channel then was no more related to that doc loader and posted notifications to higher levels about it.  That was wrong.

So, I check the status of the channel in nsDocLoader::OnChannelRedirect and when it is a failure, I completely ignore notification from that channel.  This fixes the test failures from comment 61.

Pushing to try server now, will report on results.

Comment 70

[Honza Bambas (:mayhemer)]

Comment on attachment 459035 [details] [diff] [review]
v1.6 interdiff 2

This patch (with v1.5 under it) passed try server run.

Comment 71

[Honza Bambas (:mayhemer)]

Attachment: v1.6 full patch

Comment 72

[Honza Bambas (:mayhemer)]

Attachment: v1.5->1.6 interdiff 3

BZ suggested on IRC to prevent calls to (Async)OnChanneRedirect when the old channel gets canceled from whatever reason.

This is safer solution then just let sinks, like docloader, check for this explicitly and is more backward compatible.

Comment 73

[Honza Bambas (:mayhemer)]

Ok, plans changing.  This breaks two of my xpcshell tests for this stuff, other tests are passing OK.  Going to check what happened.

Comment 74

[Honza Bambas (:mayhemer)]

New patch pushed to try server... Waiting for results.

Comment 75

[Honza Bambas (:mayhemer)]

Attachment: v1.5->1.6 interdiff 4

This is it!  Passes all tests.

Comment 76

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 460917 [details] [diff] [review]
v1.5->1.6 interdiff 4

>+      oldChannelInternal->GetCanceled(&canceled);
>+      if (canceled) {
>+          Callback(NS_BINDING_ABORTED);

Do we not want to cancel the redirection if the old channel just has an error status, no matter whether it was explicitly canceled?  I guess this is the safe thing for now....  but worth thinking about.

>+++ b/netwerk/protocol/http/HttpBaseChannel.cpp
> HttpBaseChannel::HttpBaseChannel()
>   : mStatus(NS_OK)
>+  , mCanceled(PR_FALSE)

That line needs to come after the line that initializes mRedirectionLimit.

r=me with that.

Comment 77

[Honza Bambas (:mayhemer)]

(In reply to comment #76)
> Do we not want to cancel the redirection if the old channel just has an error
> status, no matter whether it was explicitly canceled?  I guess this is the safe
> thing for now....  but worth thinking about.
> 

The interdiff version 2 was using that approach.  But in case we have a connection failure we still have to inform sinks and proceed with redirection.

I was getting these failures:

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/test_fallback_request-error_passing.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Could not get contentLength - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Error in onStartRequest: 2147500036 - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Failed to load URL: 80004004 - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Error in onStopRequest: 2147500036 - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/test_proxy-failover_passing.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Could not get contentLength - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Error in onStartRequest: 2147500036 - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Failed to load URL: 80004004 - See following stack:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver-fedora-opt-u-xpcshell/build/xpcshell/tests/test_necko/unit/head_channels.js | Error in onStopRequest: 2147500036 - See following stack:

The first one: in case we fail to load a request because of a network failure  (mStatus is a failure), we must proceed with fallback (for a resource marked that way in offline application cache).  Fallback is handled as a redirect.

The second one: in case we fail to connect a proxy (mStatus is a failure), we must proceed with other proxy configured for a host.  This is proxy fail-over and is also handled as a redirect.

I'll change the rest and land this stuff.

Comment 78

[Honza Bambas (:mayhemer)]

Attachment: v1.6 as landed [Check-in comment 78]

http://hg.mozilla.org/mozilla-central/rev/ed95964188a8

Comment 79

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

This is causing painting issues in the mozillazine forums and on the http-ftp page here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/

The page layout gets messed up and shows on the left side [dir] or [txt] 

And in the forum the layout is word wrapped in strange ways, and the icons are not being painted properly.  

 Should this be reopened  and backed out ? or file new bug.

Comment 80

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

This is affecting several sites with repaint issues, leaving image not drawn and layout errors.

CNN, MSNBC Neowin.net missing images , Activewin.com - missing images.

Comment 81

[Gary [:streetwolf52]]

(In reply to comment #80)
> This is affecting several sites with repaint issues, leaving image not drawn
> and layout errors.
> 
> CNN, MSNBC Neowin.net missing images , Activewin.com - missing images.

Using http://hg.mozilla.org/mozilla-central/rev/5f857be14db9 and no problems with icons at the sites mentioned.

Comment 82

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

(In reply to comment #81)
> (In reply to comment #80)
> > This is affecting several sites with repaint issues, leaving image not drawn
> > and layout errors.
> > 
> > CNN, MSNBC Neowin.net missing images , Activewin.com - missing images.
> 
> Using http://hg.mozilla.org/mozilla-central/rev/5f857be14db9 and no problems
> with icons at the sites mentioned

I still see the problem even with that build.

Comment 83

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

This seems to occur when re-loading a page, or after a hard refresh with 
Ctrl+F5, and just F5 to reload, images disappear.

Comment 84

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

Repo on a clean profile as well.  Reopening - I think this should be backed out pending further investigation.

Comment 85

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

Seems that setting the pref: network.http.use-cache to 'false' prevents the paint issues on refresh of the page.

Using this link http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/

Its very easy to see it change with every refresh, its acting like every-other-time the page is correct.  Turning off the http-cache setting stops the 
every-other-time syndrome.

Comment 86

[Honza Bambas (:mayhemer)]

I'm on it.

Comment 87

[Honza Bambas (:mayhemer)]

Attachment: update to regression 1 - v1

I identified the problem with image redraw: when image content is being loaded from cache we use DoNotifyListeners() that sets mPending = PR_FALSE before we trigger OnStartRequest().  Before the patch v1.6 has landed this flag was still PR_TRUE but was breaking some checks in test core code.

The image library code doesn't like seeing pending flag set to false in OnStartRequest.

What this patch changes:
- removes set of mPending = PR_FALSE before we fire mListener->OnStartRequest()
- add set of mPending = PR_FALSE between fire of mListener->OnStartRequest() and mListener->OnStopRequest()

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 461223 [details] [diff] [review]
update to regression 1 - v1

Ah, yes.  That was just wrong.

Do you need to set mPending = false if !mListener too?

Comment 89

[Honza Bambas (:mayhemer)]

(In reply to comment #88)
> Do you need to set mPending = false if !mListener too?

I was thinking about that, but if we do not call any listener, then there is actually no consumer that would read that flag anyway.  So I think it is not needed.

Comment 90

[Boris Zbarsky [:bzbarsky]]

I'd still rather set on both branches, just so we have the invariant that the boolean is false once this function returns.

Comment 91

[Honza Bambas (:mayhemer)]

I'll update patch to do that before landing it.

Comment 92

[Honza Bambas (:mayhemer)]

Attachment: Regression from comment 79 fix as landed [Check-in comment 92]

http://hg.mozilla.org/mozilla-central/rev/4d0e3037210d

Comment 93

[Honza Bambas (:mayhemer)]

Let's report new regressions as separate bugs.

Comment 94

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

Marking Verified Fixed using cset:
http://hg.mozilla.org/mozilla-central/rev/4d0e3037210d

which contains the regression fix.

thanks for the quick fix.

Comment 95

[Honza Bambas (:mayhemer)]

Not sure this bug can be marked as "VERIFIED".  This bug is not fixing any broken feature.  I had to open a new bug for the regression, that affected also publicly used nightly builds...

Comment 96

[Boris Zbarsky [:bzbarsky]]

We need to document the new constraints on implementations here...

Comment 97

[Eric Shepherd [:sheppy]]

What are the new constraints that need documenting? Is there anything beyond what's said here?
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIChannelEventSink#asyncOnChannelRedirect()
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAsyncVerifyRedirectCallback

Comment 98

[Eric Shepherd [:sheppy]]

Marking as doc complete. If there's anything more to say beyond what I poitn to in comment 97, let me know.

Comment 99

[Honza Bambas (:mayhemer)]

(In reply to comment #98)
> Marking as doc complete. If there's anything more to say beyond what I poitn to
> in comment 97, let me know.

In [1] maybe rather describe the interface as:

"Implement this interface to receive a callback with result of asynchronous redirect verify/veto decision"


[1] https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAsyncVerifyRedirectCallback