Closed Bug 21236 Opened 20 years ago Closed 20 years ago

[DOGFOOD] HTTP disposes of channel response too early

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: mozilla, Assigned: warrensomebody)

Details

(Whiteboard: [PDT+])

Attachments

(1 file)

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
Attached patch Proposed fixSplinter Review
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.
Whiteboard: [PDT+]
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
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.
Status: RESOLVED → VERIFIED
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.