nsStreamListenerTee should completely drop mSink during OnStopRequest

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed trunk])

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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...
(Assignee)

Comment 1

16 years ago
Created attachment 82941 [details] [diff] [review]
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

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

Comment 3

16 years ago
Comment on attachment 82941 [details] [diff] [review]
v1 patch

this works great for me!  r=dougt
Attachment #82941 - Flags: review+

Comment 4

16 years ago
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

16 years ago
Comment on attachment 82941 [details] [diff] [review]
v1 patch

sr=rpotts@netscape.com
Attachment #82941 - Flags: superreview+
(Assignee)

Comment 6

16 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: [fixed trunk]

Comment 7

16 years ago
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
(Assignee)

Comment 8

16 years ago
jake: that sounds like an unrelated issue to me... but try it out for yourself
now that this patch has landed on the trunk.
You need to log in before you can comment on or make changes to this bug.