Closed Bug 1371699 Opened 4 years ago Closed 3 years ago

Implement nsIAsyncInputStream for string stream

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox56 - wontfix
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 10 obsolete files)

part 1 - NonBlockingAsyncInputStream
12.66 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 2 - Use of NonBlockingAsyncInputStream
13.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 3 - gTests
10.69 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 4 - ReadSegments writer function
4.75 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 5 - Correct canceling of the AsyncWait() callback
8.68 KB, patch
froydnj
: review+
Details | Diff | Splinter Review 
There's no reason we should use an offthread streampump to pump this.
Depends on: 1371893
This will make it simple to implement CloseWithStatus.
Attachment #8876565 - Flags: review?(bkelly)
It's just luck that we haven't ended up with a multiplex stream on workers that contains at least one async stream.
Attachment #8876566 - Flags: review?(bkelly)
This will prevent ambiguous nsIInputStream inheritance once we also inherit from nsIAsyncInputStream.
Attachment #8876567 - Flags: review?(bkelly)
This is a preexisting issue that makes nsMultiplexInputStream multiple-inherit
from nsIInputStream: once via nsIMultipartInputStream and once via
nsIAsyncInputStream.  This causes problems once we end up with more multiplex
streams that are async streams, because then some assingments to
nsCOMPtr<nsIInputStream> start asserting.  This patch just removes the footgun
by getting rid of the multiple inheritance.
Attachment #8876568 - Flags: review?(bkelly)
This aligns the code more closely with how the input stream pump works: 0 available bytes when the stream itself told us it's ready means the stream is at the end.
Attachment #8876569 - Flags: review?(bkelly)
Comment on attachment 8876565 [details] [diff] [review]
part 1.  Give nsStringInputStream an mStatus variable

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

::: xpcom/io/nsStringStream.cpp
@@ +54,2 @@
>    {
> +    Clear(); // Sets mStatus too.

I think it would be safer to set mStatus in the constructor initializer.  I realize Clear() sets it, but what if that changes in the future?  It would be easy to not realize initializer is missing mStatus.  Lets err on the side of safety instead of the micro-optimization.

@@ +91,2 @@
>    {
> +    MOZ_ASSERT(mData.IsVoid() == NS_FAILED(mStatus),

It seems we only all mStatus to ever be NS_OK or NS_BASE_STREAM_CLOSED here, right?  If so, can we change this to `MOZ_ASSERT(mData.IsVoid() && mStatus == NS_BASE_STREAM_CLOSED)`?

If you want to use NS_FAILED() here, then it feels like we should ensure that we don't change other error codes to NS_BASE_STREAM_CLOSED in UpdateStatus().  AFAICT, your later patches don't protect against that.
Attachment #8876565 - Flags: review?(bkelly) → review+
Comment on attachment 8876566 [details] [diff] [review]
part 2.  Implement nsICancelableRunnable on AsyncWaitRunnable

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

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +886,5 @@
> +void
> +nsMultiplexInputStream::AsyncWaitCanceled()
> +{
> +  MutexAutoLock lock(mLock);
> +  mAsyncWaitCallback = nullptr;

I think this should set the stream status to ABORTED and then call OnInputStreamReady().  The idl contract says the callback should called "when the stream becomes readable or closed".
Attachment #8876566 - Flags: review?(bkelly) → review-
> It seems we only all mStatus to ever be NS_OK or NS_BASE_STREAM_CLOSED here, right?

That's going to change once we implement CloseWithStatus(), which is part of the nsIAsyncInputStream API.

> then it feels like we should ensure that we don't change other error codes to NS_BASE_STREAM_CLOSED in UpdateStatus()

Conceptually, UpdateStatus() is being called from places which "reopen" the stream by setting it to a new data string.  We _could_ have UpdateStatus() preserve the last-closed-with status when SetData() is called with a void nsACString, but that's the only edge case this would matter in.  And I'm not actually sure preserving mStatus in that case is semantically the right thing; it seems more consistent to treat SetData as a reopening, possibly followed by an immediate closing if the passed-in string is void.
> I think this should set the stream status to ABORTED and then call OnInputStreamReady().

Are there any guarantees that the Cancel() of a cancelable runnable is called on the thread the runnable was dispatched to?  I see no such obvious guarantees, so we'd need to at the very least check we're on the right thread or something.

The Cancel() from a worker being shutdown presumably _would_ come on the worker thread, but is calling OnInputStreamReady() really the right semantic response there?
Flags: needinfo?(bkelly)
Comment on attachment 8876567 [details] [diff] [review]
part 3.  Make nsIStringInputStream not inherit from nsIInputStream anymore

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

This seems ok to me.  We do have more QI in the code now, but it doesn't seem like a problem to me.

Nathan, are you ok with making nsIStringInputStream not actually extend nsIInputStream?

::: netwerk/streamconv/converters/nsUnknownDecoder.cpp
@@ +775,2 @@
>  
> +      listener->OnStopRequest(request, nullptr, rv);

This changes the logic to always call OnStopRequest() here, even if the string stream fails.  That seems reasonable since returning an error but not calling OnStopRequest() is probably a bug.

::: xpcom/io/nsStringStream.h
@@ +7,5 @@
>  #ifndef nsStringStream_h__
>  #define nsStringStream_h__
>  
>  #include "nsIStringStream.h"
> +#include "nsIInputStream.h"

Is this just to avoid having to add the header to a bunch of other files?
Attachment #8876567 - Flags: review?(bkelly)
Attachment #8876567 - Flags: review+
Attachment #8876567 - Flags: feedback?(nfroyd)
> This changes the logic to always call OnStopRequest() here, even if the string stream fails.

Right, I should have been clear about this being a drive-by bugfix of code that wasn't following the "You always get OnStopRequest if you got OnStartRequest" contract.

> Is this just to avoid having to add the header to a bunch of other files?

Yes, especially because hunting them all down is hard with unified compilation...
Comment on attachment 8876568 [details] [diff] [review]
part 4.  Make nsIMultiplexInputStream not inherit from nsIInputStream

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

Nathan, same question here.  Are you ok with making nsIMulitplexInputStream not inherit nsIInputStream directly?

::: dom/html/HTMLFormSubmission.h
@@ +225,5 @@
> +
> +  /**
> +   * The same stream, but as an nsIInputStream.
> +   */
> +  nsCOMPtr<nsIInputStream> mPostDataStream;

We're holding two ref's to the same object here.  I guess its not worth the risk of a raw pointer to optimize that out.
Attachment #8876568 - Flags: review?(bkelly)
Attachment #8876568 - Flags: review+
Attachment #8876568 - Flags: feedback?(nfroyd)
Comment on attachment 8876569 [details] [diff] [review]
part 6.  Fix IPCStreamSource's handling of async streams returning 0 from Available

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

::: ipc/glue/IPCStreamSource.cpp
@@ +234,5 @@
>  
>      if (available == 0) {
> +      // If we're just starting up, this could mean that the stream just has no
> +      // data yet.  But if we were notified the stream is ready to be read and
> +      // it claims it has nothing available, that indicates end of stream.

Can you expand this comment or add another comment that clearly higlights that some streams will return NS_BASE_STREAM_CLOSED from Available() and some will return 0?
Attachment #8876569 - Flags: review?(bkelly) → review+
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10)
> > I think this should set the stream status to ABORTED and then call OnInputStreamReady().
> 
> Are there any guarantees that the Cancel() of a cancelable runnable is
> called on the thread the runnable was dispatched to?  I see no such obvious
> guarantees, so we'd need to at the very least check we're on the right
> thread or something.

Right now we only use nsICancelableRunnable for worker threads AFAICT.  I think we could make this part of the interface contract.  Either dispatch to the nsIEventTarget fails or you will get canceled on the target thread before the thread shuts down.

> The Cancel() from a worker being shutdown presumably _would_ come on the
> worker thread, but is calling OnInputStreamReady() really the right semantic
> response there?

We could have native code blocked trying to from read from the stream.  I think we need to wake that up so it can detect the closure and cleanup.  Otherwise we could effectively leak the stream, right?
See comment 15.
Flags: needinfo?(bkelly)
OK, fair.  I'll give that change a spin through try and put it up for review when that's green.
Comment on attachment 8876570 [details] [diff] [review]
part 6.  Implement nsIAsyncInputStream for nsStringInputStream

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

::: xpcom/io/nsStringStream.cpp
@@ +465,5 @@
> +
> +void
> +nsStringInputStream::MaybeNotifyAsyncCallback()
> +{
> +  if (mAsyncWaitCallback) {

nit: Short-circuit style logic like `if (!mAsyncWaitCallback) { return; }`
Attachment #8876570 - Flags: review?(bkelly) → review+
Comment on attachment 8876567 [details] [diff] [review]
part 3.  Make nsIStringInputStream not inherit from nsIInputStream anymore

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

I think it's sort of weird to have nsIFooInputStream only implement nsIInputStream through QI, but I suppose this is not the first time we've had to do something like this...It's too bad our stream classes aren't builtinclass so we could provide some sort of fast conversion.
Attachment #8876567 - Flags: feedback?(nfroyd) → feedback+
Attachment #8876568 - Flags: feedback?(nfroyd) → feedback+
nsIStringInputStream is in fact builtinclass, so we could provide a fast conversion there if you want.
Attachment #8876569 - Attachment is obsolete: true
Comment on attachment 8876920 [details] [diff] [review]
Part 5 with a fix to not stop reading too soon when have more than kMaxBytesPerMessage bytes available

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

Sorry I didn't catch that in my review!
Attachment #8876920 - Flags: review?(bkelly) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eedcb67a9fb6
part 1.  Give nsStringInputStream an mStatus variable.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/503c9d22e6bb
part 2.  Implement nsICancelableRunnable on AsyncWaitRunnable.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d67624a01dc
part 3.  Make nsIStringInputStream not inherit from nsIInputStream anymore.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/30405ec37e1e
part 4.  Make nsIMultiplexInputStream not inherit from nsIInputStream.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e494fa90087
part 5.  Fix IPCStreamSource's handling of async streams returning 0 from Available. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/67a27cf0ab80
part 6.  Implement nsIAsyncInputStream for nsStringInputStream.  r=bkelly
FYI, this push seems to have caused this bc7 failure on Windows 7 VM Debug:
> 180 ERROR TEST-UNEXPECTED-FAIL | leakcheck | default process: 403944 bytes leaked (Annotators, Array, AsyncLatencyLogger, BackstagePass, CParserContext, ...)
Depends on: 1372770
Argh.  I'd run lots of tests on try, but not on Windows.

Bug 1372770 is caused by the SVG image tests just being totally broken in general.  See bug 1333899 comment 56.

Indeed, the only reason for the patches above is so we can land the major performance/memory improvements from bug 1333899 without regressing the SVG image tests.

So I have a modest proposal: I stop wasting time on this (several days worth of extra work now), we disable the broken tests, and land bug 1333899...

The other option is to try implementing the close-on-eof behavior discussed in bug 1372410 and see whether that fixes the imagelib blob bits.  But I bet that would still leave the lovely timing-dependent Windows leak and may well not fix the oranges anyway.  :(
Flags: needinfo?(bzbarsky) → needinfo?(tnikkel)
Anyway, I may try the EOF thing if I can scrounge up the time, but I really need to get some other things done too, so it's not going to be a high priority for a while.
It's just a couple tests right? Seems fine. We could also rewrite the tests so they never get a cache hit in the img cache, that should avoid the bug.
Flags: needinfo?(tnikkel)
I'm gonna steal the high quality part 5 patch for release regression bug 1393063.
See Also: → 1393063
Comment on attachment 8876920 [details] [diff] [review]
Part 5 with a fix to not stop reading too soon when have more than kMaxBytesPerMessage bytes available

Obsoleting part 5 since it landed as:
https://hg.mozilla.org/mozilla-central/rev/ce120febd926
in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1393063#c18
Attachment #8876920 - Attachment is obsolete: true
Once this lands, we may want to uplift to beta along with bug 1393063.
status-firefox56: --- → affected
status-firefox57: --- → affected
tracking-firefox56: --- → +
This is not going to land any time soon, should not be uplifted anywhere, and shouldn't be tracked for 56.
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #32)
> This is not going to land any time soon, should not be uplifted anywhere,
> and shouldn't be tracked for 56.

Is this primarily about the comment 26 test nightmare/snafu and (maybe) underlying SVG image issues or have you come to reconsider this effort for other reasons?  More specifically, is it okay for someone else (probably :baku) to steal this bug?

At least in the context of the PBlob refactor and its fallout, the Gecko world would be a more sane place if everything was a non-blocking nsIAsyncInputStream, and the closer we can get to that, the better.  There are going to be some gymnastics with nsFileStream/friends[1] and :baku is already dealing with HttpChannel Available()/Content-Length uploadStream issues, but we can get there.

1: Maybe consumers that actually want blocking sync reads can be shifted off to a completely separate interface.  Such a change would be a win for them because they potentially end up creating blocking pipes when faced with streams like nsStringInputStream that claim to be non-blocking but already have the entirety of their streams synchronously available.
Flags: needinfo?(bzbarsky)
> Is this primarily about the comment 26 test nightmare/snafu

Yes, and my lack of time for dealing with it until at least stylo ships.  Note that it's not just the imagelib tests; the Windows leak needs addressing too.

> More specifically, is it okay for someone else (probably :baku) to steal this bug?

Absolutely.
Flags: needinfo?(bzbarsky)
:baku, does this make sense for you to take this as part of your nsIAsyncInputStream work with http channels and general PBlob-refactoring cleanup?
Flags: needinfo?(amarchesini)
I rebased these patches and pushed them to try:

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

Sprinkling some debugging information around, I discovered the nsStringInputStream that we're leaking in mochitest-browser-chrome-4 is allocated here:

14:10:11     INFO -  #00: NS_LogAddRef [xpcom/base/nsTraceRefcnt.cpp:1036]
14:10:11     INFO -  #01: nsStringInputStream::AddRef() [xpcom/io/nsStringStream.cpp:119]
14:10:11     INFO -  #02: NS_NewCStringInputStream(nsIInputStream * *,nsACString const &) [xpcom/io/nsStringStream.cpp:520]
14:10:11     INFO -  #03: mozilla::dom::StringBlobImpl::GetInternalStream(nsIInputStream * *,mozilla::ErrorResult &) [dom/file/StringBlobImpl.cpp:49]
14:10:11     INFO -  #04: mozilla::dom::MultipartBlobImpl::GetInternalStream(nsIInputStream * *,mozilla::ErrorResult &) [dom/file/MultipartBlobImpl.cpp:77]
14:10:11     INFO -  #05: nsHostObjectProtocolHandler::NewChannel2(nsIURI *,nsILoadInfo *,nsIChannel * *) [dom/file/nsHostObjectProtocolHandler.cpp:848]
14:10:11     INFO -  #06: mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI *,nsIURI *,unsigned int,nsILoadInfo *,nsIChannel * *) [netwerk/base/nsIOService.cpp:842]
14:10:11     INFO -  #07: mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2(nsIURI *,nsIURI *,unsigned int,nsIDOMNode *,nsIPrincipal *,nsIPrincipal *,unsigned int,unsigned int,nsIChannel * *) [netwerk/base/nsIOService.cpp:941]
14:10:11     INFO -  #08: mozilla::net::nsIOService::NewChannelFromURI2(nsIURI *,nsIDOMNode *,nsIPrincipal *,nsIPrincipal *,unsigned int,unsigned int,nsIChannel * *) [netwerk/base/nsIOService.cpp:741]
14:10:11     INFO -  #09: NS_NewChannelInternal(nsIChannel * *,nsIURI *,nsINode *,nsIPrincipal *,nsIPrincipal *,unsigned int,unsigned int,nsILoadGroup *,nsIInterfaceRequestor *,unsigned int,nsIIOService *) [netwerk/base/nsNetUtil.cpp:252]
14:10:11     INFO -  #10: NS_NewChannel(nsIChannel * *,nsIURI *,nsINode *,unsigned int,unsigned int,nsILoadGroup *,nsIInterfaceRequestor *,unsigned int,nsIIOService *) [netwerk/base/nsNetUtil.cpp:359]
14:10:11     INFO -  #11: mozilla::dom::XMLHttpRequestMainThread::CreateChannel() [dom/xhr/XMLHttpRequestMainThread.cpp:2541]
14:10:11     INFO -  #12: mozilla::dom::XMLHttpRequestMainThread::Open(nsACString const &,nsACString const &,bool,nsAString const &,nsAString const &) [dom/xhr/XMLHttpRequestMainThread.cpp:1653]
14:10:11     INFO -  #13: mozilla::dom::XMLHttpRequestMainThread::Open(nsACString const &,nsAString const &,bool,nsAString const &,nsAString const &,mozilla::ErrorResult &) [dom/xhr/XMLHttpRequestMainThread.cpp:1540]
14:10:11     INFO -  #14: mozilla::dom::XMLHttpRequestMainThread::Open(nsACString const &,nsAString const &,mozilla::ErrorResult &) [dom/xhr/XMLHttpRequestMainThread.cpp:1526]
14:10:11     INFO -  #15: mozilla::dom::XMLHttpRequestBinding::open [s3:gecko-generated-sources-l1:9a3492291313226978bdec7cc62ddc21746dc0dc651f53aee3aced49d15a8a62703863d50cff78f6ca411144802ffdddc874b24827489eef14fe1586a294a46a/dom/bindings/XMLHttpRequestBinding.cpp::934]

and the nsInputStreamPump we're leaking is from here:

14:10:11     INFO -  #00: NS_LogAddRef [xpcom/base/nsTraceRefcnt.cpp:1036]
14:10:11     INFO -  #01: nsInputStreamPump::AddRef() [netwerk/base/nsInputStreamPump.cpp:162]
14:10:11     INFO -  #02: nsInputStreamPump::Create(nsInputStreamPump * *,nsIInputStream *,__int64,__int64,unsigned int,unsigned int,bool,nsIEventTarget *) [netwerk/base/nsInputStreamPump.cpp:68]
14:10:11     INFO -  #03: nsBaseChannel::BeginPumpingData() [netwerk/base/nsBaseChannel.cpp:260]
14:10:11     INFO -  #04: nsBaseChannel::AsyncOpen(nsIStreamListener *,nsISupports *) [netwerk/base/nsBaseChannel.cpp:697]
14:10:11     INFO -  #05: nsBaseChannel::AsyncOpen2(nsIStreamListener *) [netwerk/base/nsBaseChannel.cpp:730]
14:10:11     INFO -  #06: mozilla::dom::XMLHttpRequestMainThread::InitiateFetch(nsIInputStream *,__int64,nsACString &) [dom/xhr/XMLHttpRequestMainThread.cpp:2827]
14:10:11     INFO -  #07: mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const *) [dom/xhr/XMLHttpRequestMainThread.cpp:3103]
14:10:11     INFO -  #08: mozilla::dom::XMLHttpRequestMainThread::Send(JSContext *,mozilla::dom::Nullable<mozilla::dom::DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> const &,mozilla::ErrorResult &) [dom/xhr/XMLHttpRequestMainThread.cpp:2993]
14:10:11     INFO -  #09: mozilla::dom::XMLHttpRequestBinding::send [s3:gecko-generated-sources-l1:9a3492291313226978bdec7cc62ddc21746dc0dc651f53aee3aced49d15a8a62703863d50cff78f6ca411144802ffdddc874b24827489eef14fe1586a294a46a/dom/bindings/XMLHttpRequestBinding.cpp::1252]

Probably just some missing consideration of (now async) string streams in the blob/xhr code?
I'm OK to take this bug, I have a more generic consideration to do: nsStringInputStream is not the only non-blocking non-async stream we have. Also nsMultiplexInputStream could be non-blocking and non-async. For instance: new Blob([new Blob('hello'), new Blob('world')]). Also a SlicedInputStream can have a similar behavior.

Wondering if it is better to implement: AsyncNonBlockingInputStream that takes a non blocking inputStream and make it async.
Then, in FileReader, IPCBlobInputStream, and elsewhere, we can use this wrapper instead using a pipe.

If we do this, I can audit all the use of pipe stream and convert it to AsyncNonBlockingInputStream.
Flags: needinfo?(amarchesini)
I filed bug 1401171 to land part 4 of this bug. This can land immediately and it's not related to making nsStringInputStream async.
I would like to propose a different approach as described in comment 37.
This patch introduces a NonBlockingAsyncInputStream wrapper for non-blocking non-async inputStream.
Assignee: bzbarsky → amarchesini
Attachment #8912297 - Flags: review?(nfroyd)
Here the use of NonBlockingAsyncInputStream via NS_MakeAsyncNonBlockingInputStream.
Attachment #8912299 - Flags: review?(nfroyd)
Comment on attachment 8876568 [details] [diff] [review]
part 4.  Make nsIMultiplexInputStream not inherit from nsIInputStream

This patch landed in bug 1401171.
Attachment #8876568 - Attachment is obsolete: true
Comment on attachment 8912297 [details] [diff] [review]
part 1 - NonBlockingAsyncInputStream

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

I like the idea of using pipes in fewer places; our pipe implementation is pretty complicated, and if we could make it go away, that would be great.  But I have lots of questions.

::: xpcom/io/NonBlockingAsyncInputStream.cpp
@@ +45,5 @@
> +                                     mWeakCloneableInputStream || !mInputStream)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream,
> +                                     mWeakIPCSerializableInputStream || !mInputStream)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISeekableStream,
> +                                     mWeakSeekableInputStream || !mInputStream)

How would mInputStream ever be null, if we require it to be non-null when the stream is created?

@@ +63,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  MOZ_ASSERT(nonBlocking);

It seems like this check should be on all the time: the contract--well, at least your review comment for the patch, it's not spelled out in the code anywhere--is that the underlying stream should be non-blocking.

Also, the aforementioned review comment says that the input stream should be non-async...shouldn't we be checking for that here as well?

@@ +222,5 @@
> +
> +  if (!aCallback) {
> +    // Canceling previous callback.
> +    mWaitClosureOnlyRunnable = nullptr;
> +    mWaitClosureOnlyEventTarget = nullptr;

Maybe these should be combined into a small structure WaitClosure and a member Maybe<WaitClosure>, so we can always deal with them together?

@@ +265,5 @@
> +
> +mozilla::Maybe<uint64_t>
> +NonBlockingAsyncInputStream::ExpectedSerializedLength()
> +{
> +  MOZ_ASSERT(mWeakIPCSerializableInputStream);

Shouldn't we just return None here, so we don't crash if mWeakIPCSerializableInputStream is nullptr?

I guess the QI implementation is set up to not be QI'able to nsIIPCSerializableInputStream if we don't have mWeakIPCSerializableInputStream...but we null-check our mWeak* members for nearly all the other methods.  I don't see why we wouldn't do it here.

::: xpcom/io/NonBlockingAsyncInputStream.h
@@ +12,5 @@
> +#include "nsICloneableInputStream.h"
> +#include "nsIIPCSerializableInputStream.h"
> +#include "nsISeekableStream.h"
> +
> +// A wrapper for a slice of an underlying input stream.

A new class should have a little more documentation than this.  Even this minimal documentation means that we should clearly state that you shouldn't access the wrapped stream once you've created an instance of this class, correct?  If that's the case, do we have any debug-mode checking of this constraint?

We should also have some tests for this class, testing things like the above if possible.

@@ +14,5 @@
> +#include "nsISeekableStream.h"
> +
> +// A wrapper for a slice of an underlying input stream.
> +
> +class NonBlockingAsyncInputStream final : public nsIAsyncInputStream

We should probably prefix this with `ns`, and the files that implement it accordingly.  Or place it in the mozilla:: namespace, your choice.

@@ +29,5 @@
> +  NS_DECL_NSISEEKABLESTREAM
> +
> +  static nsresult
> +  Create(nsIInputStream* aInputStream,
> +         nsIAsyncInputStream** aAsyncInputStream);

Any constraints on the input stream (e.g. non-blocking, non-async) should be mentioned in documentation here.

@@ +40,5 @@
> +
> +  // Raw pointers because these are just QI of mInputStream.
> +  nsICloneableInputStream* mWeakCloneableInputStream;
> +  nsIIPCSerializableInputStream* mWeakIPCSerializableInputStream;
> +  nsISeekableStream* mWeakSeekableInputStream;

Please annotate these with MOZ_NONOWNING_REF; we are trying to eliminate raw pointers to refcounted objects, or at least indicate that we are confident we understand the behavior of them when they are used.
Attachment #8912297 - Flags: review?(nfroyd) → review-
Comment on attachment 8912299 [details] [diff] [review]
part 2 - Use of NonBlockingAsyncInputStream

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

I like the amount of code this cleans up.  Canceling review until we get the initial interface and the documentation sorted.

::: xpcom/io/nsStreamUtils.h
@@ +295,5 @@
>                      nsIInputStream** aReplacementOut = nullptr);
>  
> +/*
> + * Create a nsIAsyncInputStream wrapper around a non-blocking non-async
> + * inputStream.

This documentation comment is not quite correct, because:

a) it's really blocking non-async streams that we care about here, right?
b) we handle other variants inside this function, correct?

@@ +299,5 @@
> + * inputStream.
> + */
> +extern nsresult
> +NS_MakeAsyncNonBlockingInputStream(nsIInputStream* aSource,
> +                                   nsIAsyncInputStream** aAsyncInputStream);

We should have some tests for this function.  We should also try and make it very clear how this function differs from NonBlockingAsyncInputStream::Create.
Attachment #8912299 - Flags: review?(nfroyd)
Attachment #8912297 - Attachment is obsolete: true
Attachment #8912580 - Flags: review?(nfroyd)
Attachment #8912299 - Attachment is obsolete: true
Attachment #8912581 - Flags: review?(nfroyd)
Attached patch part 3 - gTests (obsolete) — — Splinter Review
Attachment #8912582 - Flags: review?(nfroyd)
Attachment #8876565 - Attachment is obsolete: true
Attachment #8876566 - Attachment is obsolete: true
Attachment #8876567 - Attachment is obsolete: true
Attachment #8876570 - Attachment is obsolete: true
Comment on attachment 8912582 [details] [diff] [review]
part 3 - gTests

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

The Available() and ReadSegments methods needs to be tested.  It would be really nice to have more sophisticated testing of AsyncWait, e.g. when we have an event target but didn't pass the closure flag and when we have an event target and we did pass the closure flag.

We should test QIs of NonBlockingAsyncInputStream to various classes and have tests for those as well--preferably both where the underlying stream QIs to the class we are testing and where the underlying stream does not.

::: xpcom/tests/gtest/TestNonBlockingAsyncInputStream.cpp
@@ +26,5 @@
> +  ASSERT_TRUE(nonBlocking);
> +
> +  // Here the non-blocking stream.
> +  ASSERT_EQ(NS_OK,
> +            NonBlockingAsyncInputStream::Create(stream, getter_AddRefs(async)));

We need to test error cases of ::Create as well.

@@ +37,5 @@
> +  // Read works fine.
> +  char buffer[1024];
> +  uint32_t read = 0;
> +  ASSERT_EQ(NS_OK, async->Read(buffer, sizeof(buffer), &read));
> +  ASSERT_EQ(uint32_t(12), read);

Could we replace this bare 12 with something like data.Length()?
Attachment #8912582 - Flags: review?(nfroyd)
Comment on attachment 8912580 [details] [diff] [review]
part 1 - NonBlockingAsyncInputStream

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

Looks reasonable.

::: xpcom/io/NonBlockingAsyncInputStream.cpp
@@ +131,5 @@
> +    nsCOMPtr<nsIRunnable> waitClosureOnlyRunnable =
> +      mWaitClosureOnly->mRunnable;
> +
> +    nsCOMPtr<nsIEventTarget> waitClosureOnlyEventTarget =
> +      mWaitClosureOnly->mEventTarget;

Maybe .forget() into these two local variables to avoid unnecessary refcounting?

::: xpcom/io/NonBlockingAsyncInputStream.h
@@ +16,5 @@
> +
> +// This class aims to wrap a non-blocking and non-async inputStream and expose
> +// it as nsIAsyncInputStream.
> +// Probably you don't want to use this class directly. Use instead
> +// NS_MakeAsyncNonBlockingInputStream().

Nit: idiomatic english would say "Instead use..."

Maybe add "...NS_MakeAsyncNonBlockingInputStream(), as it will handle different stream variants without requiring you to special-case them yourself." or something?  Just to inform the user why they would want to use this other function, rather than just telling them to do so.
Attachment #8912580 - Flags: review?(nfroyd) → review+
Comment on attachment 8912581 [details] [diff] [review]
part 2 - Use of NonBlockingAsyncInputStream

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

I guess we have a few tests for this over in part 3, even if they're not obviously separated out.
Attachment #8912581 - Flags: review?(nfroyd) → review+
Attached patch part 3 - gTests — — Splinter Review
Attachment #8912582 - Attachment is obsolete: true
Attachment #8914034 - Flags: review?(nfroyd)
Comment on attachment 8914034 [details] [diff] [review]
part 3 - gTests

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

This seems reasonable, thank you.

::: xpcom/tests/gtest/TestNonBlockingAsyncInputStream.cpp
@@ +263,5 @@
> +
> +TEST(TestNonBlockingAsyncInputStream, QI) {
> +  // Let's test ::Create() returning error.
> +  nsCOMPtr<nsIInputStream> stream = new QIInputStream(true, true, true, true);
> +  

Nit: trailing whitespace.

@@ +273,5 @@
> +  for (int i = 0; i < 8; ++i) {
> +    stream = new QIInputStream(false, i & 0x01, i & 0x02, i & 0x4);
> +    ASSERT_EQ(NS_OK, NonBlockingAsyncInputStream::Create(stream, getter_AddRefs(async)));
> +
> +    nsCOMPtr<nsICloneableInputStream> cloneable = do_QueryInterface(async);

Maybe comment before this block:

// The returned async stream should be cloneable only if the underlying stream is.

and similar comments for the two blocks below.

@@ +274,5 @@
> +    stream = new QIInputStream(false, i & 0x01, i & 0x02, i & 0x4);
> +    ASSERT_EQ(NS_OK, NonBlockingAsyncInputStream::Create(stream, getter_AddRefs(async)));
> +
> +    nsCOMPtr<nsICloneableInputStream> cloneable = do_QueryInterface(async);
> +    ASSERT_EQ(!!cloneable, !!(i & 0x01));

Maybe pull these magic values out at the beginning of the loop:

bool shouldBeCloneable = !!(i & 0x01);
bool shouldBeSerializable = !!(i & 0x02);
bool shouldBeSeekable = !!(i & 0x04);

and then you can use those in the appropriate places in the asserts and the creation of the QIInputStreams.
Attachment #8914034 - Flags: review?(nfroyd) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919a4eb29621
Implement NonBlockingAsyncInputStream - nsIAsyncInputStream wrapper for non-blocking non-async streams, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c033bdb24e14
Use of NonBlockingAsyncInputStream in our code base, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bda814e3957
Use of NonBlockingAsyncInputStream in our code base, r=froydnj
Backed out for frequently asserting at image/SourceBuffer.cpp:473, e.g. in browser-chrome's browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js on OS X and Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f05be53d0137b110002acc7098e952ce41a464f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/742dea7da9e24461e62fa9e2912ea744265a9d1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65b85ce6fa885a837444f619d7cffa2923f2e23

Push which failed 3/10 test runs on OS X opt: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=40f7a69cec34a1107e91d7192077d4fbc1efc983&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134620223&repo=mozilla-inbound

05:16:19     INFO - PROCESS-CRASH | Main app process exited normally | application crashed [@ mozilla::image::RasterImage::OnImageDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int)]
05:16:19     INFO - Crash dump filename: /var/folders/cg/3bk3f8rj41d019_kmck07vt000000w/T/tmp_CO_N3.mozrunner/minidumps/13A99592-EFC0-46ED-8AF3-C997E6AE0730.dmp
05:16:19     INFO - Operating system: Mac OS X
05:16:19     INFO -                   10.10.5 14F27
05:16:19     INFO - CPU: amd64
05:16:19     INFO -      family 6 model 69 stepping 1
05:16:19     INFO -      4 CPUs
05:16:19     INFO - 
05:16:19     INFO - GPU: UNKNOWN
05:16:19     INFO - 
05:16:19     INFO - Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
05:16:19     INFO - Crash address: 0x0
05:16:19     INFO - Process uptime: 603 seconds
05:16:19     INFO - 
05:16:19     INFO - Thread 20 (crashed)
05:16:19     INFO -  0  XUL!mozilla::image::RasterImage::OnImageDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int) [SourceBuffer.cpp:40f7a69cec34 : 472 + 0x0]
05:16:19     INFO -     rax = 0x0000000000000000   rdx = 0x00007fff7d0811f8
05:16:19     INFO -     rcx = 0x0000000000000000   rbx = 0x0000000000001536
05:16:19     INFO -     rsi = 0x0003a9000003a900   rdi = 0x0003a8000003a903
05:16:19     INFO -     rbp = 0x0000000119f03520   rsp = 0x0000000119f03500
05:16:19     INFO -      r8 = 0x0000000119f034b0    r9 = 0x0000000119f04000
05:16:19     INFO -     r10 = 0x00007fff8c4fc3ef   r11 = 0x00007fff8c4fc3c0
05:16:19     INFO -     r12 = 0x0000000100dc1fb0   r13 = 0x00000001118b2040
05:16:19     INFO -     r14 = 0x0000000000000000   r15 = 0x0000000111983260
05:16:19     INFO -     rip = 0x0000000101f36d73
05:16:19     INFO -     Found by: given as instruction pointer in context
05:16:19     INFO -  1  XUL!imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int) [imgRequest.cpp:40f7a69cec34 : 1206 + 0x25]
05:16:19     INFO -     rbx = 0x0000000119f03690   rbp = 0x0000000119f036d0
05:16:19     INFO -     rsp = 0x0000000119f03530   r12 = 0x0000000100dc1fb0
05:16:19     INFO -     r13 = 0x00000001118b2040   r14 = 0x000000011d70a630
05:16:19     INFO -     r15 = 0x0000000000000000   rip = 0x0000000101f5c39a
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  2  XUL!nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int) [nsBaseChannel.cpp:40f7a69cec34 : 882 + 0xc]
05:16:19     INFO -     rbx = 0x000000011d218510   rbp = 0x0000000119f03720
05:16:19     INFO -     rsp = 0x0000000119f036e0   r12 = 0x0000000000000000
05:16:19     INFO -     r13 = 0x0000000000001536   r14 = 0x000000011d17c140
05:16:19     INFO -     r15 = 0x0000000124d075f0   rip = 0x0000000100e810c5
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  3  XUL!nsInputStreamPump::OnStateTransfer() [nsInputStreamPump.cpp:40f7a69cec34 : 597 + 0x9]
05:16:19     INFO -     rbx = 0x0000000000001536   rbp = 0x0000000119f03790
05:16:19     INFO -     rsp = 0x0000000119f03730   r12 = 0x000000011d17c1a8
05:16:19     INFO -     r13 = 0x0000000000001536   r14 = 0x000000011d17c1e0
05:16:19     INFO -     r15 = 0x000000011d17c140   rip = 0x0000000100ea5dc4
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  4  XUL!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) [nsInputStreamPump.cpp:40f7a69cec34 : 430 + 0x8]
05:16:19     INFO -     rbx = 0x000000011d17c140   rbp = 0x0000000119f037e0
05:16:19     INFO -     rsp = 0x0000000119f037a0   r12 = 0x0000000111cd1190
05:16:19     INFO -     r13 = 0x0000000111cd11b8   r14 = 0x000000011d17c1e0
05:16:19     INFO -     r15 = 0x0000000000000001   rip = 0x0000000100ea56e8
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  5  XUL!mozilla::(anonymous namespace)::AsyncWaitRunnable::Run() [NonBlockingAsyncInputStream.cpp:40f7a69cec34 : 33 + 0x6]
05:16:19     INFO -     rbx = 0x0000000111cd11b8   rbp = 0x0000000119f037f0
05:16:19     INFO -     rsp = 0x0000000119f037f0   r12 = 0x0000000111cd1190
05:16:19     INFO -     r13 = 0x0000000111cd11b8   r14 = 0x0000000119f03870
05:16:19     INFO -     r15 = 0x0000000000000001   rip = 0x0000000100d864cf
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  6  XUL!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:40f7a69cec34 : 1039 + 0x6]
05:16:19     INFO -     rbx = 0x0000000111cd11b8   rbp = 0x0000000119f03db0
05:16:19     INFO -     rsp = 0x0000000119f03800   r12 = 0x0000000111cd1190
05:16:19     INFO -     r13 = 0x0000000111cd11b8   r14 = 0x0000000119f03870
05:16:19     INFO -     r15 = 0x0000000000000001   rip = 0x0000000100dd4c74
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  7  XUL!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp:40f7a69cec34 : 524 + 0xd]
05:16:19     INFO -     rbx = 0x0000000000000001   rbp = 0x0000000119f03dd0
05:16:19     INFO -     rsp = 0x0000000119f03dc0   r12 = 0x0000000113851ef0
05:16:19     INFO -     r13 = 0x0000000113851ee0   r14 = 0x0000000113851ec0
05:16:19     INFO -     r15 = 0x0000000000000000   rip = 0x0000000100de35af
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  8  XUL!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [MessagePump.cpp:40f7a69cec34 : 368 + 0xe]
05:16:19     INFO -     rbx = 0x0000000111cca680   rbp = 0x0000000119f03e30
05:16:19     INFO -     rsp = 0x0000000119f03de0   r12 = 0x0000000113851ef0
05:16:19     INFO -     r13 = 0x0000000113851ee0   r14 = 0x0000000113851ec0
05:16:19     INFO -     r15 = 0x0000000000000000   rip = 0x000000010138e7c3
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO -  9  XUL!MessageLoop::Run() [message_loop.cc:40f7a69cec34 : 319 + 0x5]
05:16:19     INFO -     rbx = 0x0000000111cca680   rbp = 0x0000000119f03e60
05:16:19     INFO -     rsp = 0x0000000119f03e40   r12 = 0x0000000000007503
05:16:19     INFO -     r13 = 0x00000000000008ff   r14 = 0x0000000111cd11b8
05:16:19     INFO -     r15 = 0x0000000111cd1190   rip = 0x000000010133fdd5
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO - 10  XUL!nsThread::ThreadFunc(void*) [nsThread.cpp:40f7a69cec34 : 427 + 0x5]
05:16:19     INFO -     rbx = 0x0000000111cca680   rbp = 0x0000000119f03ec0
05:16:19     INFO -     rsp = 0x0000000119f03e70   r12 = 0x0000000000007503
05:16:19     INFO -     r13 = 0x00000000000008ff   r14 = 0x0000000111cd11b8
05:16:19     INFO -     r15 = 0x0000000111cd1190   rip = 0x0000000100dd2349
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO - 11  libnss3.dylib!_pt_root [ptthread.c:40f7a69cec34 : 216 + 0x3]
05:16:19     INFO -     rbx = 0x0000000109355410   rbp = 0x0000000119f03ef0
05:16:19     INFO -     rsp = 0x0000000119f03ed0   r12 = 0x0000000000007503
05:16:19     INFO -     r13 = 0x00000000000008ff   r14 = 0x0000000119f04000
05:16:19     INFO -     r15 = 0x0000000000000000   rip = 0x0000000100b1e3a9
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO - 12  libsystem_pthread.dylib!_pthread_body + 0x83
05:16:19     INFO -     rbx = 0x0000000119f04000   rbp = 0x0000000119f03f10
05:16:19     INFO -     rsp = 0x0000000119f03f00   r12 = 0x0000000000007503
05:16:19     INFO -     r13 = 0x00000000000008ff   r14 = 0x0000000109355410
05:16:19     INFO -     r15 = 0x0000000100b1e290   rip = 0x00007fff8c4ff05a
05:16:19     INFO -     Found by: call frame info
05:16:19     INFO - 13  libsystem_pthread.dylib!_pthread_start + 0xb0
05:16:19     INFO -     rbp = 0x0000000119f03f50   rsp = 0x0000000119f03f20
05:16:19     INFO -     rip = 0x00007fff8c4fefd7
05:16:19     INFO -     Found by: previous frame's frame pointer
05:16:19     INFO - 14  libsystem_pthread.dylib!thread_start + 0xd
05:16:19     INFO -     rbp = 0x0000000119f03f78   rsp = 0x0000000119f03f60
05:16:19     INFO -     rip = 0x00007fff8c4fc3ed
05:16:19     INFO -     Found by: previous frame's frame pointer
05:16:19     INFO - 15  libnss3.dylib + 0x144290
05:16:19     INFO -     rsp = 0x0000000119f04030   rip = 0x0000000100b1e290
05:16:19     INFO -     Found by: stack scanning
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8914825 - Flags: review?(nfroyd)
Comment on attachment 8914825 [details] [diff] [review]
part 4 - ReadSegments writer function

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

And tests too, love it.

::: xpcom/io/NonBlockingAsyncInputStream.cpp
@@ +168,5 @@
> +{
> +public:
> +  ReadSegmentsData(NonBlockingAsyncInputStream* aStream,
> +                     nsWriteSegmentFun aFunc,
> +                     void* aClosure)

Nit: very funky indentation here.
Attachment #8914825 - Flags: review?(nfroyd) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e253e370b8d
Implement NonBlockingAsyncInputStream - nsIAsyncInputStream wrapper for non-blocking non-async streams, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/097c44e15165
Use of NonBlockingAsyncInputStream in our code base, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b88b3fbd7deb
Tests for NonBlockingAsyncInputStream, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/950c069a0192
NonBlockingAsyncInputStream::ReadSegments passes the correct stream to the writer callback, r=froydnj
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e1afcd3bd1
Backed out changeset 950c069a0192 
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca6494e3ebc
Backed out changeset b88b3fbd7deb 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d30fa46016
Backed out changeset 097c44e15165 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcedc6ad433d
Backed out changeset 2e253e370b8d for frequently assertin in browser-chrome at SourceBuffer.cpp:473. r=backout on a CLOSED TREE
Backed out for frequently assertin in browser-chrome at SourceBuffer.cpp:473:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcedc6ad433d9030f2a17b2dd370f9f2a0ee59a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d30fa4601619f622e1e8c8d02c077c862a5822
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca6494e3ebcc2336e5af343ffdb1db2c863baad
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e1afcd3bd1f5c71765228accfb5c3854efe067

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=10ca1d0023d72a15fff4dfd968c44697800da521&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135106521&repo=mozilla-inbound

02:35:20     INFO - GECKO(719) | --DOCSHELL 0x129b88800 == 31 [pid = 719] [id = {0db7f424-b565-bc49-8e8f-9485bdceef6e}]
02:35:20     INFO - GECKO(719) | Assertion failure: bytesRead == aCount (AppendToSourceBuffer should consume everything), at /builds/worker/workspace/build/src/image/SourceBuffer.cpp:473
02:35:20     INFO - GECKO(719) | #01: imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int) [image/imgRequest.cpp:1208]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #02: nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int) [netwerk/base/nsBaseChannel.cpp:882]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #03: nsInputStreamPump::OnStateTransfer() [netwerk/base/nsInputStreamPump.cpp:597]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #04: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) [netwerk/base/nsInputStreamPump.cpp:427]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #05: mozilla::(anonymous namespace)::AsyncWaitRunnable::Run() [xpcom/io/NonBlockingAsyncInputStream.cpp:34]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #06: nsThread::ProcessNextEvent(bool, bool*) [mfbt/Maybe.h:224]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #07: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:524]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #08: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:368]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #09: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:599]
02:35:20     INFO - 
02:35:20     INFO - GECKO(719) | #10: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:429]
02:35:20     INFO - 
02:35:21     INFO - GECKO(719) | #11: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:219]
02:35:21     INFO - 
02:35:21     INFO - GECKO(719) | #12: libsystem_pthread.dylib + 0x405a
02:35:21     INFO - 
02:35:21     INFO - GECKO(719) | #13: libsystem_pthread.dylib + 0x3fd7
Flags: needinfo?(amarchesini)
Blocks: 1407084
This is the last missing bit. If AsyncWait() is called with a new callback, or with a nullptr, the previous callback does not be dispatched.

In order to do it, I have to introduce a mutex, and check if the callback has been nullified in the meantime.
Flags: needinfo?(amarchesini)
Attachment #8922375 - Flags: review?(nfroyd)
Comment on attachment 8922375 [details] [diff] [review]
part 5 - Correct canceling of the AsyncWait() callback

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

My apologies for the long delay on this.

A test for this would be superb.  I think this all makes sense, just one design question below.

::: xpcom/io/NonBlockingAsyncInputStream.h
@@ +73,4 @@
>    Maybe<WaitClosureOnly> mWaitClosureOnly;
>  
> +  // This is protected by mLock.
> +  RefPtr<AsyncWaitRunnable> mAsyncWaitCallback;

Should we combine mAsyncWaitCallback into `WaitClosureOnly` -- and rename that structure -- to make it clear these members are related?

Oh wait, no, we don't want to do that because mWaitClosureOnly and mAsyncWaitCallback are mututally exclusive, right?  We shouldn't even run into a situation where mWaitClosureOnly.isSome() && mAsyncWaitCallback, correct?  Should we be asserting that in various places or--better--structuring things so we couldn't ever run into that in the first place?  (Something like Variant<Closed, Maybe<WaitClosureOnly>, RefPtr<AsyncWaitRunnable>>?)
Attachment #8922375 - Flags: review?(nfroyd) → review+
> Oh wait, no, we don't want to do that because mWaitClosureOnly and
> mAsyncWaitCallback are mututally exclusive, right? 

The documentation in nsIAsyncInputStream doesn't say that. I prefer to keep them separated.
Btw, almost nobody is going to use WAIT_CLOSURE_ONLY. So far we just have: TCPSocket.cpp and PresentationTCPSessionTransport.cpp (similar code as TCPSocket) and nsAStreamCopier (this is the most important one).

Probably we could consider the idea to remove the support of WAIT_CLOSURE_ONLY.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccb0581b72c
Implement NonBlockingAsyncInputStream - nsIAsyncInputStream wrapper for non-blocking non-async streams, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b418c8febf
Use of NonBlockingAsyncInputStream in our code base, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/782a4eefd04b
gtest for NonBlockingAsyncInputStream, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba730a1d259a
NonBlockingAsyncInputStream::ReadSegments passes the correct stream to the writer callback, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf81d20f6713
Correct canceling of the AsyncWait() callback in NonBlockingAsyncInputStream, r=froydnj
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99428b135058
Backed out changeset cf81d20f6713 for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux
https://hg.mozilla.org/integration/mozilla-inbound/rev/eef6e0a51e2d
Backed out changeset ba730a1d259a for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a6496729e2
Backed out changeset 782a4eefd04b for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca2fa99bd7e
Backed out changeset 52b418c8febf for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3713e297077
Backed out changeset 5ccb0581b72c for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux. CLOSED TREE
Blocks: 1435899
No longer blocks: 1333899
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/042c5cd8abb0
Implement NonBlockingAsyncInputStream - nsIAsyncInputStream wrapper for non-blocking non-async streams, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a8765481eb
Use of NonBlockingAsyncInputStream in our code base, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d691e5aaca74
gtest for NonBlockingAsyncInputStream, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa1a308b5c0
NonBlockingAsyncInputStream::ReadSegments passes the correct stream to the writer callback, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c85fdcd9f6
Correct canceling of the AsyncWait() callback in NonBlockingAsyncInputStream, r=froydnj
Depends on: 1437114
Depends on: 1437932
Depends on: 1480354
Depends on: 1517071
You need to log in before you can comment on or make changes to this bug.