Closed Bug 943149 Opened 6 years ago Closed 6 years ago

nsHttpChannel::ResolveProxy -> nsProtocolProxyService::AsyncResolve2 may break AsyncOpen contract

Categories

(Core :: Networking: HTTP, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: mayhemer, Assigned: mcmanus)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

During shutdown, proxy setting at 'none':

	xul.dll!mozilla::net::HttpBaseChannel::DoNotifyListener() 
	xul.dll!mozilla::net::nsHttpChannel::OnProxyAvailable() 
	xul.dll!nsAsyncResolveRequest::DoCallback() 
	xul.dll!nsAsyncResolveRequest::Run() 
	xul.dll!nsProtocolProxyService::AsyncResolveInternal() 
	xul.dll!nsProtocolProxyService::AsyncResolve2() 
        xul.dll!mozilla::net::nsHttpChannel::ResolveProxy() 
	xul.dll!mozilla::net::nsHttpChannel::AsyncOpen() 

Marking critical since this might lead to unexpected behavior.
I bet we can fix it by having nsHttpChannel::OnProxyAvailable post an event when it needs to run the NS_FAILED {cancel, donotifylistener()} path..

I think that would fix the contract problem without introducing a scheduler round trip to the normal fast path.

I post a patch tues unless honza wants to beat me to it (I'm going to bed now)
Patrick, feel free to take this bug, you this particular piece of code better then me.  I can review then.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 8338661 [details] [diff] [review]
nsHttpChannel::ResolveProxy may break AsyncOpen contract

Review of attachment 8338661 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4674,5 @@
> +        nsRefPtr<nsRunnableMethod<HttpBaseChannel> > event =
> +            NS_NewRunnableMethod(this, &nsHttpChannel::DoNotifyListener);
> +        rv = NS_DispatchToCurrentThread(event);
> +    }
> +    return rv;

I'm just not sure you want to return a failure when dispatch fails.  What the proxy service can do with it?  Maybe just log an NS_WARNING when dispatch fails?
Attachment #8338661 - Flags: review?(honzab.moz) → review+
 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed81454baf9

re comment 4: I added the NS_WARNING, but I kept the "return failed-rv" because in at least one use case this is on the stack all the way back to the caller of asyncOpen(), so they can be aware that onstart/onstop won't be called.. though I suspect if we're in such a bad place that events can't be dispatched it isn't going to turn out well anyhow :)
https://hg.mozilla.org/mozilla-central/rev/3ed81454baf9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Honza, can you please confirm this no longer reproduces for you in the latest Aurora builds?
Flags: needinfo?(honzab.moz)
Is this urgent?  I'm quite occupied right now.
(In reply to Honza Bambas (:mayhemer) from comment #8)
> Is this urgent?  I'm quite occupied right now.

I don't think it's urgent but it would be good to have it verified before we release in 6 weeks.
Whiteboard: [qa-]
Never have hit this again.
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.