Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 10 years ago

I'd like to add the ability to clone nsIInputStream objects.  This would require stream classes to opt-in by implementing this interface:

  interface nsICloneableInputStream : nsIInputStream
  {
    nsIInputStream clone();
  };

The clone() method returns a copy of the stream in the most efficient manner possible.  This might mean different operations depending on the concrete implementation of the stream.

For example,

 * nsFileInputStream would simply dup() its file descriptors like when serializing across IPC.
 * nsStringStream would simply do an nsCString-style copy construction.
 * nsPipe would create a new nsPipeInputStream that reads from the same underlying pipe buffer.  The buffer is maintained until all cloned input streams are closed.

Motivation:

In order to implement Fetch we need to be able to clone Request and Response body streams. [1]  These body streams can be provided a number of ways resulting in different nsIInputStream implementations being used.  (Essentially the list above.)

Currently it seems the only infrastructure we have for performing a clone-type operation is to use nsIInputStreamTee.  Unfortunately this almost always does the sub-optimal thing in terms of memory/CPU performance by copying the entire stream into a new buffer.  Also, it requires that one side of the stream be fully read in order for the other side to have access to all the data.  If you have a slow reader on the primary stream then you may delay a fast reader on the other side.

If instead we allow the underlying stream implementations decide how to produce a copy then we can avoid these problems.  This is important for Fetch and ServiceWorkers in order to support potentially huge Response bodies in an efficient way.

I originally posted about this on dev-platform here:

  https://groups.google.com/forum/#!topic/mozilla.dev.platform/6T7sO74GErk

I didn't get any objections (or other feedback), so I plan to proceed.

I put this bug in xpcom since xpcom/io seems like a reasonable place for the cloneable interface to live.

[1] https://fetch.spec.whatwg.org/#dom-response-clone

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 1 • 10 years ago

This could be useful for DevTools transport code too.  

The bug title says "in C++ code", but could this new method also be available for chrome JS as well?

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 10 years ago

Sure.  I was just being conservative about not exposing it where its not needed.

	Seth Fowler [:seth] [:s2h]
	Comment 3 • 10 years ago

I'm excited! We'd definitely have use for this in ImageLib once it becomes available.

	Alex Vincent [:WeirdAl]
	Comment 4 • 10 years ago

Peanut gallery comment:  have you looked at nsStorageStream?  You can call .getInputStream(0) and effectively have a copy of a storage stream ready to go.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 10 years ago

Thanks Alex, I was not aware of nsStorageStream.  That's probably a good fallback solution and maybe internally to nsIPipe, but I think we should avoid copying if we can.  For file streams, for example, this would create a potentially large in-memory copy of the file.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 10 years ago

Just started looking at this in earnest.  Here's a start with one of the easy streams.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 9 • 10 years ago

Completely untested, but I think something like this will work for nsPipe input streams.

I'm going to add some gtests now to make sure I didn't completely break things.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 10 years ago

Updated pipe clone implementation.  Now with gtests that validate the clone behavior pretty exhaustively.  They still don't cover all of the pipe API surface, though.

Nathan, any initial thoughts on these changes?

Also a try to this point:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0a3bc5501f33

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 13 • 10 years ago

Fix non-unified build:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6dfc361d97cf

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 10 years ago

Looks like I introduced a ref-count cycle that causes pipes to leak.  I'll look at fixing that later today.

	Nathan Froyd [:froydnj]
	Comment 16 • 10 years ago

Comment on attachment 8538720 [details] [diff] [review]
P4 Make nsPipeInputStream cloneable. (v2)

Review of attachment 8538720 [details] [diff] [review]:
-----------------------------------------------------------------

I *think* splitting out the nsPipeReadState changes might have made this easier to comprehend.

Having a better interface to nsSegmentedBuffer would have also helped, I think. :(

I didn't look at the tests very closely, but I *think* they do the right thing and touch all the interesting cases.  I'll look at those more closely in the next iteration.

Something about this design of the pipe having to know about all its input streams doesn't sit well with me.

::: xpcom/io/nsPipe3.cpp
@@ +432,5 @@
>  nsPipe::GetInputStream(nsIAsyncInputStream** aInputStream)
>  {
> +  MOZ_ASSERT(mInputList.Length() > 0);
> +  nsRefPtr<nsPipeInputStream> ref = mInputList[0];
> +  ref.forget(aInputStream);

I applaud you for making this use nsRefPtr/forget rather than raw NS_ADDREF.

How much do we care that this can return two different input streams at different times, and input streams at different places besides?  It seems surprising to me that somebody can close the result of GetInputStream, but if the input stream has been cloned, then the next person calling GetInputStream gets an unclosed input stream.

@@ +661,5 @@
> +      if (mInputList.Length() > 1) {
> +        if (mInputList[i]->OnInputException(aReason, events)) {
> +          mon.Notify();
> +        }
> +        mInputList.RemoveElementAt(i);

Is it important that mInputList is maintained in order  (I don't see any reason why, but I could be missing something)?  Otherwise, you could swap this element and the last element, and pop the last one off the end.

@@ +1218,5 @@
>    return NS_ERROR_UNEXPECTED;    // keep compiler happy
>  }
>  
> +NS_IMETHODIMP
> +nsPipeInputStream::GetCloneable(bool* aCloneableOut)

To make clients of this call more readable, you may want to mark the attribute as [infallible]; then you can just say:

  if (stream->GetCloneable()) {
    ...
  }

instead of doing the nsresult dance.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 17 • 10 years ago

This should fix the memory leak and issue with returning a different stream from GetInputStream().

I still need to fix recognizing that individual streams have been closed in Read(), etc.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 19 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=864748312995

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 20 • 10 years ago

Add [infallible] to nsICloneableInputStream cloneable attribute.  This doesn't change the signature of any of the implementing methods in later patches, of course, though.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 21 • 10 years ago

This patch fixes things so the many input streams each track their own status in addition to the overall pipe status.  This lets individual streams get closed while the rest of the pipe continues to function.

Green try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8745ace9ec2d

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 22 • 10 years ago

So it appears cloning file streams is not so straight forward and may not be possible.

I previously thought I could simply do a dup() like our IPC code does.  A dup(), however, shares the file position between the two file descriptors.  This obviously doesn't work for Clone() as we want the streams to be able to read independently without effecting each other.

The only option appears to be to let the nsFileInputStream re-open the file on Clone().

Is this something we want to allow in the child process?  This would mean serializing the file path across IPC in addition to the fd.

Jed tells me that open() will continue to function in the child process in the future by proxying to an unsandboxed process.

Ben, what do you think?  If the sandbox will support child process open(), should we allow the child to re-open the file on Clone here?

I'm unsure at the moment and I'm going to shelve the file cloning support for now.  I'll work to get other streams cloning instead.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 23 • 10 years ago

I could also support cloning file streams only in the parent process, but that seems to have limited utility.  (At least, it doesn't help my use case.)

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 24 • 10 years ago

This provides an NS_CloneInputStream() factory method.  It will attempt to use the nsICloneableInputStream if available.

If that's not possible, then a fallback is provided using nsPipe, NS_AsyncCopy, and StreamTransportService.  The caller must opt into this fallback by provided an extra "replacement stream out" argument.  The streams produced by this fallback approach are then directly cloneable without any additional memory impact.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 25 • 10 years ago

(In reply to Ben Kelly [:bkelly] from comment #22)

I don't think we ever want to give the child process file paths if we can help it.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 32 • 10 years ago

I'm punting file stream Clone() support to bug 1127531.  We could make it cloneable only in the parent process.  This might benefit things like imglib, but I don't need it for the ServiceWorker Cache right now.

If you want parent process file stream Clone(), please chime in on bug 1127531.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 33 • 10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2d87a4fbd5

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 34 • 10 years ago

I forgot to hg qref some additional comments I added.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 35 • 10 years ago

An intermittent that needs to be fixed:

  Assertion failure: mInputList.Length() > 0, at /builds/slave/try-l64-d-00000000000000000000/build/src/xpcom/io/nsPipe3.cpp:677

In this try log:

  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bkelly@mozilla.com-4269619404a9/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-e10s-4-bm52-tests1-linux64-build400.txt.gz

	Nathan Froyd [:froydnj]
	Comment 36 • 10 years ago

Comment on attachment 8556684 [details] [diff] [review]
P1 Add the nsICloneableInputStream interface. (v1)

Review of attachment 8556684 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsICloneableInputStream.idl
@@ +14,5 @@
> +
> +  // Produce a copy of the current stream in the most efficient way possible.
> +  // In this case "copy" means that both the original and cloned streams
> +  // should produce the same bytes for all future reads.  Bytes that have
> +  // already been consumed from the original stream are not copies to the

Nit: "...are not copied to the..."

	Nathan Froyd [:froydnj]
	Comment 37 • 10 years ago

Comment on attachment 8556683 [details] [diff] [review]
P0 Break out helper routines to support gtests for more stream types. (v2)

Review of attachment 8556683 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/moz.build
@@ +7,5 @@
>  UNIFIED_SOURCES += [
>      'TestCRT.cpp',
>      'TestEncoding.cpp',
>      'TestExpirationTracker.cpp',
> +    'TestHelpers.cpp',

Bikeshed: WDYT about calling this StreamTestHelpers, in an effort to indicate that it's not a test file itself?

	Nathan Froyd [:froydnj]
	Comment 38 • 10 years ago

Comment on attachment 8556683 [details] [diff] [review]
P0 Break out helper routines to support gtests for more stream types. (v2)

Review of attachment 8556683 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/TestHelpers.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please add a one-line summary comment:

---
...license block...

/* summary comment */

... includes
---

so MXR (and DXR?) display a helpful summary of the file in the directory listing.

@@ +15,5 @@
> +
> +namespace testing {
> +
> +void
> +CreateData(uint32_t aNumBytes, nsTArray<char>& aDataOut);

Forgot to say: some brief comments on each of these functions would be super-helpful for later contributors.

	Nathan Froyd [:froydnj]
	Comment 39 • 10 years ago

Comment on attachment 8556689 [details] [diff] [review]
P3 Make nsStorageStream's input streams cloneable. (v1)

Review of attachment 8556689 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/TestStorageStream.cpp
@@ +67,3 @@
>  
>    // now, write 3 more full 4k segments + 11 bytes, starting at 8192
>    // total written equals 20491 bytes

I wonder about the magic-numberness of this...

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 40 • 10 years ago

Make sure the cloned nsStringInputStream correctly owns the string (directly or via sharing).  The nsDependentCSubstring does not do this, so use Assign() instead.

New try, with patches from bug 1073231:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f32d96bd7908

	Nathan Froyd [:froydnj]
	Comment 41 • 10 years ago

Comment on attachment 8556694 [details] [diff] [review]
P5 Provide NS_CloneInputStream() factory method in nsStreamUtils.h. (v1)

Review of attachment 8556694 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/TestCloneInputStream.cpp
@@ +38,5 @@
> +  nsCOMPtr<nsIInputStream> base;
> +  nsresult rv = NS_NewCStringInputStream(getter_AddRefs(base), inputString);
> +  ASSERT_TRUE(NS_SUCCEEDED(rv));
> +
> +  // Take advantage of the fact nsBufferedInputStream is not cloneable right

Nit: "Take advantage of nsBufferedInputStream being non-cloneable right..."

@@ +66,5 @@
> +  nsCOMPtr<nsIInputStream> base;
> +  nsresult rv = NS_NewCStringInputStream(getter_AddRefs(base), inputString);
> +  ASSERT_TRUE(NS_SUCCEEDED(rv));
> +
> +  // Take advantage of the fact nsBufferedInputStream is not cloneable right

Same thing here.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 42 • 10 years ago

Comment on attachment 8556694 [details] [diff] [review]
P5 Provide NS_CloneInputStream() factory method in nsStreamUtils.h. (v1)

Review of attachment 8556694 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsStreamUtils.h
@@ +283,5 @@
> + * @return NS_OK on successful clone.  Error otherwise.
> + */
> +extern nsresult
> +NS_CloneInputStream(nsIInputStream* aSource, nsIInputStream** aCloneOut,
> +                    nsIInputStream** aReplacementOut = nullptr);

Just a nit: Gecko convention seems to be that the outparam is either the first param (NS_NewURI) or the last one (NS_StartCORSPreflight). Would you mind making the outparam the first one?

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 43 • 10 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #42)
> Comment on attachment 8556694 [details] [diff] [review]
> P5 Provide NS_CloneInputStream() factory method in nsStreamUtils.h. (v1)
> 
> Review of attachment 8556694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/io/nsStreamUtils.h
> @@ +283,5 @@
> > + * @return NS_OK on successful clone.  Error otherwise.
> > + */
> > +extern nsresult
> > +NS_CloneInputStream(nsIInputStream* aSource, nsIInputStream** aCloneOut,
> > +                    nsIInputStream** aReplacementOut = nullptr);
> 
> Just a nit: Gecko convention seems to be that the outparam is either the
> first param (NS_NewURI) or the last one (NS_StartCORSPreflight). Would you
> mind making the outparam the first one?

Well, there are two out params and they are both at the end.  The second one is optional.  I would prefer not to have a first-arg out param and an optional last-arg out param.  That seems like the worst of both worlds.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 44 • 10 years ago

So, I was not able to reproduce that intermittent I mentioned in comment 35:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7e647ff454

Looking at the code, though, I think I see how it could happen.  If a thread is blocked in Wait() when an input stream exception occurs, then the Wait() can unblock and re-enter the exception handling code.  This patch tries to gracefully handle that condition.

I also added some locking in nsPipeInputStream::Clone(), as that was touching the mInputList.

New try:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6016c9c8c8b8

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 45 • 10 years ago

New pipe patch after applying the interdiff.

	Nathan Froyd [:froydnj]
	Comment 46 • 10 years ago

Ben, friendly reminder to provide that interdiff we talked about last week.  Thanks!

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 47 • 10 years ago

Here is the interdiff.  I had to rebase the old patch first, so its not a perfect interdiff, but its pretty close.

	Nathan Froyd [:froydnj]
	Comment 48 • 10 years ago

Comment on attachment 8561440 [details] [diff] [review]
bug1100398_p4_interdiff_v2_to_v7.patch

Review of attachment 8561440 [details] [diff] [review]:
-----------------------------------------------------------------

I have questions about some of these things.

::: xpcom/io/nsPipe3.cpp
@@ +107,5 @@
>  };
>  
>  //-----------------------------------------------------------------------------
>  
> +// an input end of a pipe (maintained as a list of refs within the pipe)

hooray for updating comments.

@@ +116,5 @@
>    , public nsICloneableInputStream
>    , public nsIClassInfo
>  {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

We're declaring our own refcnt now because we can have multiples of these per-pipe, correct?

I'm not sure I want to know why this class needs to be threadsafe.  I wonder if anything breaks if we make it not threadsafe...

@@ +185,3 @@
>    int64_t                        mLogicalOffset;
>    bool                           mBlocking;
> +  nsresult                       mStatus;

What exactly is this the status of?  (I see that it replaces mClosed, but...)  At the very least, this could use a nicer name; a descriptive comment would be excellent.

Let's also move this before mBlocking, in an attempt to make member variables pack slightly better.

@@ +305,1 @@
>    uint32_t CountSegmentReferences(int32_t aSegment) const;

This method needs either:

- a MOZ_ASSERT(mReentrantMonitor.AssertCurrentThreadIn()) in it; or
- a |const ReentrantMonitorAutoLock& aProofOfLock| argument.

I think the former is more Gecko-idiomatic.

@@ +322,5 @@
> +
> +  // But hold a strong ref to our original input stream.  For backward
> +  // compatibility we need to be able to consistently return this same
> +  // object from GetInputStream().
> +  nsRefPtr<nsPipeInputStream> mOriginalInput;

Is mOriginalInput kept in mInputList or not?  Would be good to clarify the relationship here.

@@ +410,5 @@
> +  MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");
> +  nsrefcnt count = --mRefCnt;
> +  NS_LOG_RELEASE(this, count, "nsPipe");
> +  if (count == 0) {
> +    delete (this);

Nit: parentheses not required.

@@ +414,5 @@
> +    delete (this);
> +    return 0;
> +  }
> +  if (mOriginalInput && count == 1) {
> +    mOriginalInput = nullptr;

This seems sketchy to me.  If the refcnt of the pipe is one, we're going to release the original input stream?  Why would we do that?

That means code like:

nsRefPtr<nsPipe> p(new nsPipe());
return p->QueryInterface(...whatever the args are...);

will end up nulling out mOriginalInput, which doesn't seem correct.

Assuming this is the desired thing, there should be an explanatory comment here.

	Nathan Froyd [:froydnj]
	Comment 49 • 10 years ago

Comment on attachment 8558654 [details] [diff] [review]
P4 Make nsPipeInputStream cloneable. (v7, through interdiff 001)

Review of attachment 8558654 [details] [diff] [review]:
-----------------------------------------------------------------

Just going to cancel this review, since I responded in the interdiff.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 50 • 10 years ago

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #48)
> Comment on attachment 8561440 [details] [diff] [review]
> bug1100398_p4_interdiff_v2_to_v7.patch
> @@ +116,5 @@
> >    , public nsICloneableInputStream
> >    , public nsIClassInfo
> >  {
> >  public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> We're declaring our own refcnt now because we can have multiples of these
> per-pipe, correct?
> 
> I'm not sure I want to know why this class needs to be threadsafe.  I wonder
> if anything breaks if we make it not threadsafe...

I believe it needs to be threadsafe if you want to use the nsPipeInputStream with IO threads like STS.

> @@ +185,3 @@
> >    int64_t                        mLogicalOffset;
> >    bool                           mBlocking;
> > +  nsresult                       mStatus;
> 
> What exactly is this the status of?  (I see that it replaces mClosed,
> but...)  At the very least, this could use a nicer name; a descriptive
> comment would be excellent.

Individual input streams can now be closed with different statuses without closing the overall pipe.  So I need to track this status per input stream.

> @@ +322,5 @@
> > +
> > +  // But hold a strong ref to our original input stream.  For backward
> > +  // compatibility we need to be able to consistently return this same
> > +  // object from GetInputStream().
> > +  nsRefPtr<nsPipeInputStream> mOriginalInput;
> 
> Is mOriginalInput kept in mInputList or not?  Would be good to clarify the
> relationship here.

It is kept in mInputList.

> @@ +414,5 @@
> > +    delete (this);
> > +    return 0;
> > +  }
> > +  if (mOriginalInput && count == 1) {
> > +    mOriginalInput = nullptr;
> 
> This seems sketchy to me.  If the refcnt of the pipe is one, we're going to
> release the original input stream?  Why would we do that?

Because there is a ref-cycle between the nsPipe object and mOriginalInput stream.  The input streams must have a strong ref to the pipe in order to keep it alive since normally external code only references the streams directly.  For most of the streams in mInputList the nsPipe only keeps weak references that get cleared when the input stream destructs.  In order to always return the same original input stream, though, we must keep a strong ref to it from the pipe which creates the cycle.

I tried to write a comment to this effect, but I'll expand it.

> nsRefPtr<nsPipe> p(new nsPipe());
> return p->QueryInterface(...whatever the args are...);
> 
> will end up nulling out mOriginalInput, which doesn't seem correct.

Doesn't this need to return an already_AddRefed<> regardless in order to avoid deleting the pipe on return?  I don't think the pipe here would act any different from other ref counted objects in this case.

	Nathan Froyd [:froydnj]
	Comment 51 • 10 years ago

(In reply to Ben Kelly [:bkelly] from comment #50)
> > @@ +414,5 @@
> > > +    delete (this);
> > > +    return 0;
> > > +  }
> > > +  if (mOriginalInput && count == 1) {
> > > +    mOriginalInput = nullptr;
> > 
> > This seems sketchy to me.  If the refcnt of the pipe is one, we're going to
> > release the original input stream?  Why would we do that?
> 
> Because there is a ref-cycle between the nsPipe object and mOriginalInput
> stream.  The input streams must have a strong ref to the pipe in order to
> keep it alive since normally external code only references the streams
> directly.  For most of the streams in mInputList the nsPipe only keeps weak
> references that get cleared when the input stream destructs.  In order to
> always return the same original input stream, though, we must keep a strong
> ref to it from the pipe which creates the cycle.

Hm.  So this case essentially says, "if we have this stream that holds a reference to us, and we have a current refcount of 1, then destroy that thing holding the ref to us, which in turn destroys us too"?

This sounds still sounds sketchy, but I can see the rationale.

> > nsRefPtr<nsPipe> p(new nsPipe());
> > return p->QueryInterface(...whatever the args are...);
> > 
> > will end up nulling out mOriginalInput, which doesn't seem correct.
> 
> Doesn't this need to return an already_AddRefed<> regardless in order to
> avoid deleting the pipe on return?  I don't think the pipe here would act
> any different from other ref counted objects in this case.

No, because QI addrefs its argument (assuming that it succeeds).  But I missed the point where the input stream is holding an additional reference to the pipe, so the above code would return something with a refcount of 2.

	Nathan Froyd [:froydnj]
	Comment 53 • 10 years ago

Comment on attachment 8561685 [details] [diff] [review]
P4 interdiff 002 address review feedback

Review of attachment 8561685 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsPipe3.cpp
@@ +789,5 @@
>  
>  void
>  nsPipe::SetAllNullReadCursors()
>  {
> +  mReentrantMonitor.AssertCurrentThreadIn();

Bonus points for adding all these extra assertions!

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 60 • 10 years ago

One more try since this does such extensive changes to nsIPipe which is used all over the tree:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=24be4dab1739

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 61 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb6d729cd22
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f14875d011dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/507718807f6d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/db0d65c0b7a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef97268b8d6a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48906d15a28e

	Carsten Book [:Tomcat]
	Comment 62 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0fb6d729cd22
https://hg.mozilla.org/mozilla-central/rev/f14875d011dc
https://hg.mozilla.org/mozilla-central/rev/507718807f6d
https://hg.mozilla.org/mozilla-central/rev/db0d65c0b7a7
https://hg.mozilla.org/mozilla-central/rev/ef97268b8d6a
https://hg.mozilla.org/mozilla-central/rev/48906d15a28e