Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: jst, Assigned: mayhemer)

Tracking

({dev-doc-complete})

unspecified
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, fennec2.0a1+)

Details

Attachments

(3 attachments, 20 obsolete attachments)

4.65 KB, text/plain
Details
117.45 KB, patch
Details | Diff | Splinter Review
1.13 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
We need this for other things too!

Updated

9 years ago
Blocks: 516730
(Assignee)

Comment 2

9 years ago
-> me
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Comment 3

9 years ago
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?
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.
(Assignee)

Comment 5

9 years ago
Created attachment 407759 [details] [diff] [review]
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

9 years ago
Honza, ping? If this is to land in 1.9.3, it needs to be finished up and go through reviews!
(Assignee)

Comment 7

9 years ago
Created attachment 414729 [details] [diff] [review]
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.
Attachment #407759 - Attachment is obsolete: true
Attachment #414729 - Flags: review?(cbiesinger)
Attachment #414729 - Flags: review?(benjamin)
(Assignee)

Comment 8

9 years ago
Created attachment 414767 [details] [diff] [review]
v0.9.2

And this contains fix for nsBaseChannel as bz suggests in comment 4.
Attachment #414729 - Attachment is obsolete: true
Attachment #414767 - Flags: review?(cbiesinger)
Attachment #414767 - Flags: review?(benjamin)
Attachment #414729 - Flags: review?(cbiesinger)
Attachment #414729 - Flags: review?(benjamin)
(Assignee)

Comment 9

9 years ago
Created attachment 415010 [details] [diff] [review]
v0.9.3

...and with fixed mRedirectType (fixed failing test_CrossSiteXHR.html).
Attachment #414767 - Attachment is obsolete: true
Attachment #415010 - Flags: review?(cbiesinger)
Attachment #415010 - Flags: review?(benjamin)
Attachment #414767 - Flags: review?(cbiesinger)
Attachment #414767 - Flags: review?(benjamin)

Comment 10

9 years ago
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?
Attachment #415010 - Flags: review?(benjamin) → review-
Can you describe how this async redirect API is going to work with multiple redirect listeners?
Attachment #415010 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

9 years ago
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.
See Also: → bug 475991
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?
(Assignee)

Comment 14

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
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

9 years ago
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.
(Assignee)

Comment 17

9 years ago
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.
(Assignee)

Comment 18

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

Comment 19

9 years ago
I don't know, but for now let's assume that clients will respond.
(Assignee)

Comment 20

9 years ago
Created attachment 422027 [details] [diff] [review]
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.
Attachment #415010 - Attachment is obsolete: true
Attachment #422027 - Flags: review?(cbiesinger)
Attachment #422027 - Flags: review?(bzbarsky)
Attachment #415010 - Flags: review?(cbiesinger)
Attachment #415010 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Blocks: 475991

Updated

9 years ago
No longer blocks: 475991

Comment 21

9 years ago
Why doesn't this block 475991? We can't implement that without the async redirect API in necko, right?
(Assignee)

Updated

9 years ago
Blocks: 475991

Comment 22

9 years ago
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

9 years ago
> 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

9 years ago
(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

9 years ago
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.
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

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

Updated

9 years ago
Depends on: 546606
(Assignee)

Updated

9 years ago
Blocks: 546606
No longer depends on: 546606
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?
So Honza says this patch is good.  We could use review--it's been waiting for 2 months.  Biesi?  Boris?
(Assignee)

Comment 30

9 years ago
Created attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

Merged to the current m-c.  Thanks Bjarne for this merge.
Attachment #422027 - Attachment is obsolete: true
Attachment #435277 - Flags: review?(cbiesinger)
Attachment #435277 - Flags: review?(bzbarsky)
Attachment #422027 - Flags: review?(cbiesinger)
Attachment #422027 - Flags: review?(bzbarsky)
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.
Attachment #435277 - Flags: superreview?(cbiesinger)
Attachment #435277 - Flags: review?(cbiesinger)
Attachment #435277 - Flags: review?(bzbarsky)
Attachment #435277 - Flags: review?(bjarne)
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 on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

I'm stealing this review at jduell's behest.
Attachment #435277 - Flags: review?(bjarne) → review?(josh)

Updated

9 years ago
Attachment #435277 - Flags: review?(josh) → review+
Comment on attachment 435277 [details] [diff] [review]
v1 merged by Bjarne

> + * The Initial Developer of the Original Code is
> + * Honza Bambas <honzab@firemni.cz>

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

9 years ago
Created attachment 441016 [details] [diff] [review]
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 <honzab@firemni.cz>
> 
> 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.
Attachment #435277 - Attachment is obsolete: true
Attachment #435277 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 36

9 years ago
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 ;)
(Assignee)

Comment 38

9 years ago
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 ;)
(Assignee)

Comment 39

9 years ago
Created attachment 441620 [details] [diff] [review]
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.
Attachment #441016 - Attachment is obsolete: true
Attachment #441620 - Flags: review?(cbiesinger)
(Assignee)

Updated

9 years ago
Attachment #441620 - Flags: review?(cbiesinger) → superreview?(cbiesinger)
(Assignee)

Comment 40

9 years ago
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.
Attachment #441620 - Attachment is obsolete: true
Attachment #441620 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 41

9 years ago
Created attachment 442132 [details] [diff] [review]
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.
Attachment #442132 - Flags: superreview?(cbiesinger)
Attachment #442132 - Flags: superreview?(cbiesinger) → superreview-
(Assignee)

Comment 43

9 years ago
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.
(Assignee)

Comment 44

9 years ago
Created attachment 445609 [details] [diff] [review]
v1.4

Fixed the failing onbeforeunload test (privately approved by mak).  Interdiff follows.
Attachment #442132 - Attachment is obsolete: true
Attachment #445609 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 45

9 years ago
Created attachment 445610 [details] [diff] [review]
1.3 -> 1.4 diff
(Assignee)

Comment 46

9 years ago
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.
(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 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 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.
Attachment #445609 - Flags: superreview?(cbiesinger) → superreview-
(Assignee)

Comment 50

8 years ago
(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.
(Assignee)

Updated

8 years ago
Depends on: 571522
(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.
(Assignee)

Comment 52

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

Comment 53

8 years ago
Created attachment 450834 [details] [diff] [review]
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.
Attachment #445609 - Attachment is obsolete: true
Attachment #445610 - Attachment is obsolete: true
Attachment #450834 - Flags: review?(cbiesinger)
(Assignee)

Comment 54

8 years ago
Created attachment 450835 [details] [diff] [review]
v1.4 -> 1.5 interdiff
(Assignee)

Comment 55

8 years ago
> 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 on attachment 450834 [details] [diff] [review]
v1.5

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

Comment 57

8 years ago
Created attachment 453774 [details] [diff] [review]
v1.5 unbitrotted
Attachment #450834 - Attachment is obsolete: true
Attachment #453774 - Flags: review?(cbiesinger)
Attachment #450834 - Flags: review?(cbiesinger)

Comment 58

8 years ago
This is an extension-visible API change which needs to block beta2.
blocking2.0: --- → beta2+
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?
Attachment #453774 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 61

8 years ago
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.
(Assignee)

Comment 63

8 years ago
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.
(Assignee)

Comment 64

8 years ago
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
(Assignee)

Comment 65

8 years ago
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?
#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.
(Assignee)

Comment 67

8 years ago
Created attachment 458396 [details] [diff] [review]
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.
Attachment #450835 - Attachment is obsolete: true
Attachment #453774 - Attachment is obsolete: true
Attachment #458396 - Flags: review?(bzbarsky)
Why does it matter what the loadgroup is doing?  Shouldn't canceling the channel cancel the redirect no matter what?
(Reporter)

Updated

8 years ago
blocking2.0: beta2+ → betaN+
(Assignee)

Comment 69

8 years ago
Created attachment 459035 [details] [diff] [review]
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.
Attachment #458396 - Attachment is obsolete: true
Attachment #459035 - Flags: review?(bzbarsky)
Attachment #458396 - Flags: review?(bzbarsky)
(Assignee)

Comment 70

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

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

Comment 71

8 years ago
Created attachment 460815 [details] [diff] [review]
v1.6 full patch
Attachment #459035 - Attachment is obsolete: true
Attachment #459035 - Flags: review?(bzbarsky)
(Assignee)

Comment 72

8 years ago
Created attachment 460816 [details] [diff] [review]
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.
Attachment #460816 - Flags: review?(bzbarsky)
(Assignee)

Comment 73

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

Updated

8 years ago
Attachment #460815 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #460816 - Attachment is obsolete: true
Attachment #460816 - Flags: review?(bzbarsky)
(Assignee)

Comment 74

8 years ago
New patch pushed to try server... Waiting for results.
(Assignee)

Comment 75

8 years ago
Created attachment 460917 [details] [diff] [review]
v1.5->1.6 interdiff 4

This is it!  Passes all tests.
Attachment #460917 - Flags: review?(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.
Attachment #460917 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 77

8 years ago
(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.
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
tracking-fennec: --- → 2.0a1+
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.
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

8 years ago
(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.
(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.
This seems to occur when re-loading a page, or after a hard refresh with 
Ctrl+F5, and just F5 to reload, images disappear.
Repo on a clean profile as well.  Reopening - I think this should be backed out pending further investigation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 86

8 years ago
I'm on it.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 87

8 years ago
Created attachment 461223 [details] [diff] [review]
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()
Attachment #461223 - Flags: review?(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?
Attachment #461223 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 89

8 years ago
(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.
I'd still rather set on both branches, just so we have the invariant that the boolean is false once this function returns.
(Assignee)

Comment 91

8 years ago
I'll update patch to do that before landing it.
(Assignee)

Comment 92

8 years ago
Created attachment 461244 [details] [diff] [review]
Regression from comment 79 fix as landed [Check-in comment 92]

http://hg.mozilla.org/mozilla-central/rev/4d0e3037210d
Attachment #461223 - Attachment is obsolete: true
(Assignee)

Comment 93

8 years ago
Let's report new regressions as separate bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Marking Verified Fixed using cset:
http://hg.mozilla.org/mozilla-central/rev/4d0e3037210d

which contains the regression fix.

thanks for the quick fix.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 95

8 years ago
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...
(Assignee)

Updated

8 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 8 years ago8 years ago
We need to document the new constraints on implementations here...
Keywords: dev-doc-needed
(Assignee)

Updated

8 years ago
Depends on: 587177
Marking as doc complete. If there's anything more to say beyond what I poitn to in comment 97, let me know.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 99

8 years ago
(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
Depends on: 710776
You need to log in before you can comment on or make changes to this bug.