Closed Bug 813869 Opened 10 years ago Closed 7 years ago

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

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1271987

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

Attached patch v1Splinter Review
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 :)
Attachment #683895 - Flags: review?(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
Attachment #683895 - Flags: review?(mcmanus)
(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.
Attached file 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.
Assignee: nobody → jduell.mcbugs
Attachment #683895 - Flags: review?(mcmanus)
Attachment #683895 - Flags: review?(mcmanus) → review+
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 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
Attachment #683895 - Flags: approval-mozilla-beta?
Attachment #683895 - Flags: approval-mozilla-aurora?
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.
Attachment #683895 - Flags: approval-mozilla-beta?
Attachment #683895 - Flags: approval-mozilla-beta-
Attachment #683895 - Flags: approval-mozilla-aurora?
Attachment #683895 - Flags: approval-mozilla-aurora-
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.
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-active]
At this point this patch should be done as part of bug 1271987, as it hits the some pieces of code.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jduell.mcbugs)
Resolution: --- → DUPLICATE
Duplicate of bug: 1271987
You need to log in before you can comment on or make changes to this bug.