Closed Bug 21236 Opened 20 years ago Closed 20 years ago
[DOGFOOD] HTTP disposes of channel response too early
Fur, I think this broke when you landed the memory cache. Basically, what I'm seeing is that when a HTTP requestor's OnStopRequest() is called, the context that is passed around has already been disposed of, because the HTTP's channel is being prematurely released. I've attached a diff that fixes the problem (for me, anyway). Can you (or maybe Warren) review this? I'd like to see this fixed for M12 if possible, as it breaks some stuff.
Priority: P3 → P2
Summary: HTTP disposes of channel response too early → [DOGFOOD] HTTP disposes of channel response too early
Target Milestone: M12
Do you have an example URL or test case, or is it sporadic ? Your patch seems OK, though it does cause the observer messages to nest incorrectly, i.e. mOpenObserver->OnStartRequest(...) mResponseDataListener->OnStartRequest(...) mOpenObserver->OnStopRequest(...) mResponseDataListener->OnStopRequest(...) Of coure, this may not matter since it sounds like we're getting rid of AsyncOpen and mOpenObserver...
pavlov, you might want to apply this patch and see if it fixes that crash we observed earlier today.
A simple test case is to select "Search | Search the Internet", choose one or more search engines, type in some text to search for, then click on the "Search" button. Without the fix, the spinning icons will never stop, giving the appearance that the search never completes. The real reason is that the search datasource's OnStopRequest() gets called with its "context" parameter being null, so it can't get to its state information for the given connection. With the fix, of course, it works just dandy.
doens't fix the crash :(
FYI, pavlov is talking about a different crash. That's OK, pav - it was just a shotgun approach. You should file a bug for your crash if you have a repeatable test case.
Putting on PDT+ radar.
something else is broken here. Regardless of the ordering of the ResponseCompleted() call, mChannel was ADDREF'd by the response listener itself. It's pointer should be legit until it releases it.
Judd, the problem is that calling mChannel->ResponseCompleted() nulls out HTTP's internal reference to "context".
See nsHTPChannel.cpp, in ResponseComplete(), around line # 1251: mResponseContext = 0; Perhaps a better fix than the one I proposed earlier is to just comment that line out? (Note: its defined via a nsCOMPtr). Just commenting it out fixes the problems I'm seeing, BTW.
Adding Jud to cc list.
I think we have two bugs here. rjc has one, and the other is the crasher. Following the lead from the Summary in this bug. The only way I can see this happening is if the responselistener gets *two* OnStopRequest() callbacks :-/. Rick, can the socket transport *every* fire two to the same listener?
anyone have a test case for the crasher?
Robert, HTTP has some circular references :-/. I think that line is needed to break one to prevent a leak.
Jud, isn't mResponseContext just a nsISupports (i.e. just an opaque value) that the caller passes in when the connection is created, and then called back with on the listeners? Where is the circular reference?
Roger that. My bad. We can definately null that out before an OnStop() is fired to a consumer. Robert's fix (the zero'ing out one) looks good to me... for his bug though. I don't think it has any bearing on the crasher.
Just for clarity, you mean *NOT* null out mResponseContext before OnStopRequest() is called, right? <grin> [My one liner fix was to comment out the line that nulls it out.] Thanks, Jud!
<frown> yea. *not* null it out. Is it the weekend yet? (as if that matters anymore :).
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Bulk move of all Necko (to be deleted component) bugs to new Networking component.
Robert, does your testcase ("Search | Search the Internet") still hold true for this bug? Adding self to cc list.
Chris, yep, it does.
Cant mark as verified because the Search engines do not appear on wednesday's build on windows - 1999121508 Is there any other way to test this?
Probably, but not that I'm aware of, sorry.
Using the search | search the internet test case. Verified 1999122208
You need to log in before you can comment on or make changes to this bug.