143311 - nsStreamListenerTee should completely drop mSink during OnStopRequest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

nsStreamListenerTee should completely drop mSink during OnStopRequest...

this code:

  NS_IMETHODIMP
  nsStreamListenerTee::OnStopRequest(nsIRequest *request,
                                     nsISupports *context,
                                     nsresult status)
  {
    NS_ENSURE_TRUE(mListener, NS_ERROR_NOT_INITIALIZED);
    mInputTee = 0;
    mSink = 0;
    return mListener->OnStopRequest(request, context, status);
  }

does not do enough... it needs to also ensure that mInputTee also drops its
reference to mSink.

dougt noticed this bug when he tried to fetch a HTTP URL via javascript
initiated from a timer callback... apparently, javascript garbage collection
only runs when we load a page.  as a result, the reference that dougt's code
held to mInputTee (for reading the data in his OnDataAvailable) was never
getting released (garbage collected).  sure, this should probably be solved in
the JS GC code, but we clearly have a bug in necko as well, since it is
perfectly valid to hold a reference to the input stream given out in
OnDataAvailable.

patch in hand...

Comment 1

[Darin Fisher]

Attachment: v1 patch

this patch changes nsIInputStreamTee to make it possible to drop the sink.

gordon: i've also added the error handling code we talked about adding to
TeeSegment.  if mSink->Write fails, we simply drop mSink, and pretend that
nothing bad happened.

Comment 2

[Doug Turner (:dougt)]

I solved this by explictly calling close() on the scriptable input stream
wrapping the inputstream passed by oda.  

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 82941 [details] [diff] [review]
v1 patch

this works great for me!  r=dougt

Comment 4

[gordon]

Comment on attachment 82941 [details] [diff] [review]
v1 patch

r=gordon as well.  Thanks for adding the error handling code for
mSink->Write().

Comment 5

[rpotts (gone)]

Comment on attachment 82941 [details] [diff] [review]
v1 patch

sr=[e-mail]

Comment 6

[Darin Fisher]

fixed-on-trunk

Comment 7

[Jacob Kjome]

Will this stop XMLHttpRequest from continuing even after a page reload?

You can see this behavior at:

http://www.visi.com/~hoju/humor.html

Click the "Check Links" button and it will begin looping through all the links
on the page and make HEAD requests to the links and then reports their status by
inserting the returned HTTP status code right after the link.

What I am seeing is that if I start this process, do a shift+reload of the page,
the script continues on in a loop from where it last left off (without ever
pressing the "Check Links" button again).  In the status bar, it tells you which
link it is currently processing.  If it was "checking link 5" right before I did
the reload, after the page reloads, it starts back up again at "checking link 6".  

I would have expected that the whole script would have started over since the
scope of the script should have been lost upon reload of the page.  Am I wrong
here?  Does the fix for this bug fix this issue?  If this is a different bug, is
it a known bug or should it be reported as a new one?

BTW, you will need to add the following to your prefs.js for this to work:

user_pref("signed.applets.codebase_principal_support", true); 

Then, when asked whether to allow this script to run, check "remember decision"
and click "Ok".

Jake

Comment 8

[Darin Fisher]

jake: that sounds like an unrelated issue to me... but try it out for yourself
now that this patch has landed on the trunk.