Closed
Bug 21236
Opened 25 years ago
Closed 25 years ago
[DOGFOOD] HTTP disposes of channel response too early
Categories
(Core :: Networking, defect, P2)
Core
Networking
Tracking
()
VERIFIED
FIXED
M12
People
(Reporter: mozilla, Assigned: warrensomebody)
Details
(Whiteboard: [PDT+])
Attachments
(1 file)
746 bytes,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•25 years ago
|
Priority: P3 → P2
Summary: HTTP disposes of channel response too early → [DOGFOOD] HTTP disposes of channel response too early
Target Milestone: M12
Reporter | ||
Comment 1•25 years ago
|
||
Comment 2•25 years ago
|
||
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...
Comment 3•25 years ago
|
||
pavlov, you might want to apply this patch and see if it fixes that crash we observed earlier today.
Reporter | ||
Comment 4•25 years ago
|
||
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.
Comment 5•25 years ago
|
||
doens't fix the crash :(
Comment 6•25 years ago
|
||
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.
Comment 8•25 years ago
|
||
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.
Reporter | ||
Comment 9•25 years ago
|
||
Judd, the problem is that calling mChannel->ResponseCompleted() nulls out HTTP's internal reference to "context".
Reporter | ||
Comment 10•25 years ago
|
||
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.
Reporter | ||
Comment 11•25 years ago
|
||
Adding Jud to cc list.
Comment 12•25 years ago
|
||
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?
Comment 13•25 years ago
|
||
anyone have a test case for the crasher?
Comment 14•25 years ago
|
||
Robert, HTTP has some circular references :-/. I think that line is needed to break one to prevent a leak.
Reporter | ||
Comment 15•25 years ago
|
||
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?
Comment 16•25 years ago
|
||
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.
Reporter | ||
Comment 17•25 years ago
|
||
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!
Comment 18•25 years ago
|
||
<frown> yea. *not* null it out. Is it the weekend yet? (as if that matters anymore :).
Reporter | ||
Updated•25 years ago
|
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 19•25 years ago
|
||
Fixed.
Comment 20•25 years ago
|
||
Bulk move of all Necko (to be deleted component) bugs to new Networking component.
Comment 21•25 years ago
|
||
Robert, does your testcase ("Search | Search the Internet") still hold true for this bug? Adding self to cc list.
Reporter | ||
Comment 22•25 years ago
|
||
Chris, yep, it does.
Comment 23•25 years ago
|
||
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?
Reporter | ||
Comment 24•25 years ago
|
||
Probably, but not that I'm aware of, sorry.
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Comment 25•25 years ago
|
||
Using the search | search the internet test case. Verified 1999122208
Comment hidden (collapsed) |
You need to log in
before you can comment on or make changes to this bug.
Description
•