813869 - missing DoNotifyListener, invalid late error return in nsHttpChannel::AsyncOpen

Status: RESOLVED DUPLICATE of bug 1271987

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: v1

I noticed a couple of issues with recent proxy changes to nsHttpChannel:AsyncOpen

1) we don't call OnStart/Stop if we call BeginConnect() synchronously and it fails.

2) we are returning values other than NS_OK after adding channel to loadGroup and/or notifying observers, which I'm pretty sure is not OK.

This patch makes some effort to avoid cases where we both Cancel() a channel and call DoNotifyListeners, on the idea that we only ever need to do one or the other (if connect() succeeded, we should cancel).  That said, AFAICT it's ok to do both.

One other thing that I haven't had time to look into but someone may know.  I'm 99% sure that it's actually a valid use case to call "Cancel(NS_OK)": I remember seeing that use case, but can't remember when/where it's used.  Anyway, if for some reason http-on-modify-request observers can do that (and I don't see why not), this patch will need some more work, as right now we'd wind up never calling OnStop/Start in that case (possibly first wandering into nsChannelClassifier, getting suspended and resumed but with neither a cachepump or transactionpump to ever call OnStart/Stop: perhaps Resume needs an assert that one or the other is non-null?).

How quickly making little fixes to our HTTP code leads one into the weeds :)

Comment 1

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #0)
> Created attachment 683895 [details] [diff] [review]
> v1
> 
> I noticed a couple of issues with recent proxy changes to
> nsHttpChannel:AsyncOpen
> 
> 1) we don't call OnStart/Stop if we call BeginConnect() synchronously and it
> fails.
> 

that's on purpose

nsichannel says " If asyncOpen returns successfully, the channel promises to call at least onStartRequest and onStopRequest."

in the case of beginconnect() being called synchronously that effects the asyncopen() return code.. and if it fails it doesn't call OSR

Comment 2

[Josh Matthews [:jdm]]

(In reply to Jason Duell (:jduell) from comment #0)
> One other thing that I haven't had time to look into but someone may know. 
> I'm 99% sure that it's actually a valid use case to call "Cancel(NS_OK)": I
> remember seeing that use case, but can't remember when/where it's used. 
> Anyway, if for some reason http-on-modify-request observers can do that (and
> I don't see why not), this patch will need some more work, as right now we'd
> wind up never calling OnStop/Start in that case (possibly first wandering
> into nsChannelClassifier, getting suspended and resumed but with neither a
> cachepump or transactionpump to ever call OnStart/Stop: perhaps Resume needs
> an assert that one or the other is non-null?).

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2035

I looked into this one time when I saw the warning in the console, and it's necessary to use a success code here or else the XHR fails. There's some blob test suite in our code that fails when that occurs.

Comment 3

[Jason Duell]

Attachment: IRC chat with bz

So I think we do want this patch--we also have an (implicit, but those are important too: we should probably update the docs and make it explicit) contract with observers, which do things like keep references to the channels they observe, insert themselves into the listener chain, and then (implicitly) expect to get OnStop so they know when to release their ref to the channel.  bz agrees (IRC chat attached).  The existing code can possibly call http-on-modify-request observers here already, and in bug 800799 I'll be adding http-on-starting-request observers too.

Comment 4

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

cancel() with a success code isn't really supported. If XHR needs to do that, it should use an internal error code and treat that as if it were success...

Comment 5

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

See documentation at http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRequest.idl#61

Comment 6

[Jason Duell]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd841806e43b

This is not a strong must-have for aurora/beta IMO (I'd be interested to hear Patrick's opinion), but it does fix a bug that started on FF 18. Consequences probably aren't that harsh (small amount of memory leakage), and no known cases in the wild.  But I'd have a warm fuzzy feeling if I knew the hole was closed.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 769764
User impact if declined: in some cases addons won't ever get OnStart/Stop notifications from channels, if they failed during BeginConnect/Connect.  Only channels affected are FTP and HTTP if async proxy resolution fails (I don't know how often that happens).  Most likely outcome is addons will never let go of channel (memory leak).  
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch: none

Comment 7

[Ed Morley [:emorley]]

Backed out for breaking m1, m2, reftest:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fd841806e43b

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec0f2aade39

Comment 8

[Alex Keybl [:akeybl]]

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

Not necessary, already broke some tests - I don't think there's good reason to fix this in FF18.

Comment 9

[Patrick McManus [:mcmanus]]

Jason, this is another r+'d but unlanded patch. Do you think this is still a concern? If so it should get assigned and landed - if not wontfix.

Comment 10

[Jason Duell]

At this point this patch should be done as part of bug 1271987, as it hits the some pieces of code.