support cloneable nsIInputStreams

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
24 days ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(6 attachments, 25 obsolete attachments)

9.64 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.05 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.72 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
7.20 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
47.02 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
9.50 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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
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?
Sure.  I was just being conservative about not exposing it where its not needed.
Summary: support cloneable nsIInputStreams in C++ code → support cloneable nsIInputStreams
I'm excited! We'd definitely have use for this in ImageLib once it becomes available.
(Assignee)

Updated

4 years ago
Blocks: 1073231
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.
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.
(Assignee)

Updated

4 years ago
Blocks: serviceworker-cache
No longer blocks: 940273
Just started looking at this in earnest.  Here's a start with one of the easy streams.
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.
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
Attachment #8538065 - Attachment is obsolete: true
Attachment #8538708 - Flags: feedback?(nfroyd)
Attachment #8538708 - Attachment is obsolete: true
Attachment #8538708 - Flags: feedback?(nfroyd)
Attachment #8538720 - Flags: feedback?(nfroyd)
Looks like I introduced a ref-count cycle that causes pipes to leak.  I'll look at fixing that later today.
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.
Attachment #8538720 - Flags: feedback?(nfroyd)
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.
Attachment #8538720 - Attachment is obsolete: true
Posted patch P4 v2 to v3 interdiff. (obsolete) — Splinter Review
Add [infallible] to nsICloneableInputStream cloneable attribute.  This doesn't change the signature of any of the implementing methods in later patches, of course, though.
Attachment #8537461 - Attachment is obsolete: true
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
Attachment #8539549 - Attachment is obsolete: true
Attachment #8539551 - Attachment is obsolete: true
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.
Flags: needinfo?(bent.mozilla)
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.)
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.
(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.
Flags: needinfo?(bent.mozilla)
Attachment #8538707 - Attachment is obsolete: true
Attachment #8556683 - Flags: review?(nfroyd)
Attachment #8540450 - Attachment is obsolete: true
Attachment #8556684 - Flags: review?(nfroyd)
Attachment #8537462 - Attachment is obsolete: true
Attachment #8556686 - Flags: review?(nfroyd)
Attachment #8537859 - Attachment is obsolete: true
Attachment #8556689 - Flags: review?(nfroyd)
Attachment #8540454 - Attachment is obsolete: true
Attachment #8556692 - Flags: review?(nfroyd)
Attachment #8540966 - Attachment is obsolete: true
Attachment #8556694 - Flags: review?(nfroyd)
(Assignee)

Updated

4 years ago
Blocks: 1127531
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.
I forgot to hg qref some additional comments I added.
Attachment #8556692 - Attachment is obsolete: true
Attachment #8556692 - Flags: review?(nfroyd)
Attachment #8556713 - Flags: review?(nfroyd)
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
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..."
Attachment #8556684 - Flags: review?(nfroyd) → review+
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?
Attachment #8556683 - Flags: review?(nfroyd) → review+
Attachment #8556686 - Flags: review?(nfroyd)
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.
(Assignee)

Updated

4 years ago
See Also: → 1128588
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...
Attachment #8556689 - Flags: review?(nfroyd) → review+
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
Attachment #8556686 - Attachment is obsolete: true
Attachment #8558049 - Flags: review?(nfroyd)
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.
Attachment #8556694 - Flags: review?(nfroyd) → review+
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?
(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.
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
New pipe patch after applying the interdiff.
Attachment #8556713 - Attachment is obsolete: true
Attachment #8556713 - Flags: review?(nfroyd)
Attachment #8558654 - Flags: review?(nfroyd)
Attachment #8558049 - Flags: review?(nfroyd) → review+
Ben, friendly reminder to provide that interdiff we talked about last week.  Thanks!
Flags: needinfo?(bkelly)
Here is the interdiff.  I had to rebase the old patch first, so its not a perfect interdiff, but its pretty close.
Flags: needinfo?(bkelly)
Attachment #8561440 - Flags: review?(nfroyd)
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.
Attachment #8561440 - Flags: review?(nfroyd) → feedback+
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.
Attachment #8558654 - Flags: review?(nfroyd)
(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.
(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.
Attachment #8561685 - Flags: review?(nfroyd)
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!
Attachment #8561685 - Flags: review?(nfroyd) → review+
Attachment #8558652 - Attachment is obsolete: true
Attachment #8558654 - Attachment is obsolete: true
Attachment #8561440 - Attachment is obsolete: true
Attachment #8561685 - Attachment is obsolete: true
Attachment #8562866 - Flags: review+
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
(Assignee)

Updated

4 years ago
Depends on: 1130803
(Assignee)

Updated

4 years ago
Depends on: 1133939
(Assignee)

Updated

4 years ago
Depends on: 1136453
Depends on: 1136762
You need to log in before you can comment on or make changes to this bug.