1453340 - Supporting nsIAsyncInputStream::AsyncWait(callback) when there is an already pending operation

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Andrea Marchesini [:baku]]

This seems supported by the nsIAsyncInputStream interface documentation, but we have many classes that do not. This bug is about fixing them all.

I put this bug under 'XPCOM' component but it will also require reviews from necko, dom and image peers.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsBufferedInputStream

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - IDB StreamWrapper

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Image inputStreams

Note that here I also introduce a lock to make mCallback safe to be used on multiple threads.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - IPCBlobInputStream

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - nsIMIMEInputStream

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - nsMultiplexInputStream

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 7 - PartiallySeekableInputStream

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 8 - SlicedInputStream

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8967006 [details] [diff] [review]
part 3 - Image inputStreams

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

LGTM. Lots of code duplication though, it should probably be moved into a common base class at some point.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8967011 [details] [diff] [review]
part 7 - PartiallySeekableInputStream

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

Thanks for all the tests!  Do you think we need a test for AsyncWait(callback,..); AsyncWait(nullptr,..); ASSERT(!callback->called()); ?  it's probably impossible to construct one like that, tho, there is no point to sync on...

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8967006 [details] [diff] [review]
part 3 - Image inputStreams

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

::: image/encoders/png/nsPNGEncoder.cpp
@@ +762,1 @@
>      callback->OnInputStreamReady(this);

regarding the comment above `ReentrantMonitorAutoEnter autoEnter(mReentrantMonitor);` do we want to leave the monitor to call the callback here?  I have no idea how this code works, just to make sure we don't regress under some tight race condition.

(applies to all classes in this patch)

Comment 12

[Andrea Marchesini [:baku]]

> Thanks for all the tests!  Do you think we need a test for
> AsyncWait(callback,..); AsyncWait(nullptr,..); ASSERT(!callback->called());
> ?  it's probably impossible to construct one like that, tho, there is no
> point to sync on...

Yes, would be nice, but as you said, I don't have anything to check against.

Comment 13

[Andrea Marchesini [:baku]]

> autoEnter(mReentrantMonitor);` do we want to leave the monitor to call the
> callback here?  I have no idea how this code works, just to make sure we
> don't regress under some tight race condition.

I think we must, otherwise, if in the callback, we do anther AsyncWait() we are in a deadlock.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> > autoEnter(mReentrantMonitor);` do we want to leave the monitor to call the
> > callback here?  I have no idea how this code works, just to make sure we
> > don't regress under some tight race condition.
> 
> I think we must, otherwise, if in the callback, we do anther AsyncWait() we
> are in a deadlock.

I think the reentrant monitor allows that (in contrary to Mutex), but we'll see, maybe the patch as is won't cause any problems.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8967010 [details] [diff] [review]
part 6 - nsMultiplexInputStream

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

I don't know if I'm entirely comfortable with this set of patches.  The nsIAsyncInputStream documentation would have to make it very clear that this is a possible thing: I agree with you that the behavior being proposed in these patches is implied by the documentation, but it would be better to be explicit.  AsyncWait'ing multiple times on a given stream with callbacks still outstanding means previous callbacks are unceremoniously dropped, which sounds like quite unfriendly behavior.

I would be a little more comfortable if comment 9's request for some sort of base class were implemented, and comments in nsIAsyncInputStream or similar pointed out that you should use this base class to guarantee common functionality.  Even better if you could enforce the need to use the base class with types somehow.

I'd really like to understand the motivation for these changes a little better, and to hear your thoughts on why you think this isn't going to cause problems.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8967012 [details] [diff] [review]
part 8 - SlicedInputStream

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

See comments on part 6.  Comments from other people not baku are also welcome, of course, since y'all are probably hip deep in stream code more often than I am. :)

Comment 17

[Andrea Marchesini [:baku]]

I think that the main point here is to be consistent. There are many nsIInputStream implementations supporting the 'replacement' of the callback (pipe and other necko inputStreams). Other don't, but the documentation doesn't say anything about it.

Implementing a base class for nsIAsyncInputStream objects would be nice. As you probably know, I did several improvements/bug-fixing in nsIInputStreams and one of the issues I would like to work on is to cleaning up/unifying code. But I don't think this bug is the right place to do this because here there is a logical change to discuss and review.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #17)
> I think that the main point here is to be consistent.

Yes, that was why I wanted to open this bug.  

Problem is that these asyncWait callback restrictions have landed as part of bug 1360887 et al on 55 w/o a proper necko peer review.  Now we have inconsistent behavior for a long time in the tree.  Hence, probably no need to deal with the behavior consolidation right now, when we are also landing number of other changes to stream classes.

The question is what is the right approach here - discard (=release and not ever notify) the old callback (btw, that release MUST happen outside any locks of the stream!) and replace with the new callback (what we have generally in necko, afaik) or, should we fail the new asyncWait while it's already pending - or even call both/all (totally crazy)?  not sure what can break things more, but I personally more tend to let the second asyncWait call win and override.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

I'd like to echo :froydnj's requests for improved documentation in nsIAsyncInputStream or clearly linked to from there.  In particular, I think it would be useful to:

- Update the asyncWait documentation to explain that callers will overwrite their callback and explain why they would clear or overwrite their callback and perhaps provide concrete examples as pointers.  This need not be exhaustive, but there are a massive number of async stream consumers in tree, not to mention concrete C++ subclasses (28 per DXR, 3 of which are test-only).

- This may be too much for this bug, but at a higher level, explain the different expected behaviors of stream consumers as it relates to setup, steady-state, and exceptional conditions like cancellation or re-POSTs, and the different thread/thread-pools that may be involved.  :mayhemer's concern on https://bugzilla.mozilla.org/show_bug.cgi?id=1451731#c0 seems more than theoretical, and that would be useful knowledge for people dealing with the code.  Much of that may derive from the fact that necko code tends to use multiple parallel potentially-racing threads via StreamTransportService, whereas most DOM code has an explicit single thread or logical-thread via TaskQueue.

  This again need not be exhaustive, and people could add (and should be encouraged to add) other scenarios  they're aware of to the docs as the gaps become apparent.  Note that I'm not suggesting this as a means to hand-wave away properly dealing with concurrency issues, but to better understand and properly handle the situations that will occur and place assertion guards against the things that really shouldn't.


Also, for my part, I would like to apologize for not properly recognizing when I should be redirecting reviews or adding necko peers as complementary reviews in the blob-refactor fallout.

Comment 20

[Andrea Marchesini [:baku]]

Are we OK if I improve the documentation in nsIAsyncInputStream.idl describing this behavior?
I know that is not enough, but these patches are green on try.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> Problem is that these asyncWait callback restrictions have landed as part of
> bug 1360887 et al on 55 w/o a proper necko peer review. 
(yeah, only a superreviewer review, which is technically enough. But should have asked necko peer review)

To make API usage less error prone, throwing when trying to replace existing callback feels sanest to me.
Why would we want any other behavior?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967007 [details] [diff] [review]
part 4 - IPCBlobInputStream

Unless there is some strong good use case for replacing callback, I don't see why to go this way and make the API more error prone.

Does anyone feel strongly that replacing should be supported? If so, why?
If there is a good use case, ask review again.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Andrea Marchesini [:baku] from comment #20)
> Are we OK if I improve the documentation in nsIAsyncInputStream.idl
> describing this behavior?

Please add documentation regardless! ;)

smaug has some objections, so we have to work something out.  I guess one usecase would be something like:

1. Call AsyncWait().
2. AsyncWait() returns
3. We decide to wait again, but with different behavior.
4. Call AsyncWait() with a different callback.

Whereas with the throwing behavior proposed in comment 21 (and implemented by a couple places in the tree), the client has to remember "have I already set a callback" and manually clear it out by calling AsyncWait(nullptr).

I don't know how common it is to call AsyncWait() multiple times on the same stream, but with different callbacks; I suspect not very?  (And trying to find out by making AsyncWait() throw seems somewhat difficult.)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

That is not really a concrete use case. Do we really rely on changing the callback?
It feels error prone also from racy-ness point of view. The first callback may or may not have been called before 3.
If there is need for replacing callback, I'd rather do that explicitly than implicitly.

But again, if there are currently good use cases for replace using the current API, then that behavior should be fine.

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #21)
> (In reply to Honza Bambas (:mayhemer) from comment #18)
> > Problem is that these asyncWait callback restrictions have landed as part of
> > bug 1360887 et al on 55 w/o a proper necko peer review. 
> (yeah, only a superreviewer review, which is technically enough. But should
> have asked necko peer review)
> 
> To make API usage less error prone, throwing when trying to replace existing
> callback feels sanest to me.
> Why would we want any other behavior?

I think the behavior of 'replaceability' was something we were always having in the tree, at least in some of heavily used and old necko classes.  When I've seen a new code being written differently, it appeared wrong and inconsistent to me and, mainly, non-discussed.

This is really a big change we talk about here.  I will walk all the asyncWait implementations around the code base and try to make a conclusion on which of the two is better, or if it should even be left in discretion of the implementation as suitable (allow both possibilities, and make inconsistency officially acceptable)

Or, we may go with an in-the-middle option: disallow CHANGE of a callback when there is one already pending.  If we try to set the same callback again (mCallback == aCallback), do whatever appropriate, if anything, and return NS_OK; wrapping streams pass on to the inner stream to deal with it at will, for consistency.

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #24)
> That is not really a concrete use case. Do we really rely on changing the
> callback?
> It feels error prone also from racy-ness point of view. The first callback
> may or may not have been called before 3.
> If there is need for replacing callback, I'd rather do that explicitly than
> implicitly.

Problem is that our streams (I mean - the interface) are heavily used by number of different consumers.  There is a wide possibility of combinations among all of them.  If sometimes a page resource doesn't load, it's hard to find out where the error comes from with so many different implementation behavior of every other class.

I think the real concern of mine is the case when we call asyncWait again with the _same_ non-null callback (I believe a possible and valid use case).  The `if (aCallback && mCallback) { return ERROR; }` line will break calls to AsyncWait in some situation somewhere/sometimes leaving inconsistent state - asyncWait threw, what now?  and we still got a call later, what the hell?

Although, replacing - regardless we allow that now and historically on some places - may be a wrong thing to do, as callbacks don't get called (break of a contract.)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Honza Bambas (:mayhemer) from comment #25)
> Or, we may go with an in-the-middle option: disallow CHANGE of a callback
> when there is one already pending.
This is what I was talking about. Why we'd let one to change the callback.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #25)
> > Or, we may go with an in-the-middle option: disallow CHANGE of a callback
> > when there is one already pending.
> This is what I was talking about. Why we'd let one to change the callback.

OK, sounds like something we agree on then.

To sum:

* mCallback == null, aCallback == null -> likely a no-op, but a wrapping streams passes on to its inner, always
* mCallback == null, aCallback != null -> as before
* mCallback != null, aCallback == null -> cancel pending operation/throw callback ref etc., pass on inner
* mCallback != null, aCallback == mCallback -> update logic if applicable, pass on inner
* mCallback != null, aCallback && aCallback != mCallback -> THROW

Comment 29

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsBufferedInputStream

Comment 30

[Andrea Marchesini [:baku]]

Attachment: part 2 - IDB StreamWrapper

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 3 - Image inputStreams

Comment 32

[Andrea Marchesini [:baku]]

Attachment: part 4 - IPCBlobInputStream

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 5 - nsIMIMEInputStream

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 6 - nsMultiplexInputStream

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 7 - PartiallySeekableInputStream

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 8 - Pipe Stream

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 9 - SlicedInputStream

Comment 38

[Andrea Marchesini [:baku]]

Attachment: part A - some necko inputStreams

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part B - some more necko inputStreams

Comment 40

[Andrea Marchesini [:baku]]

Attachment: part C - NonBlockingAsyncInputStream

Comment 41

[Andrea Marchesini [:baku]]

I think I covered all the nsIAsyncInputStream implementations. Maybe we want a similar approach for nsIAsyncOutputStream objects, but in a separate bug.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Hmm, do we want to just return early if same callback is passed or call possibly asyncWait again on some internal stream?

I guess we should ensure that callback member variable is stored on a local variable just before OnInputStreamReady is called, callback member variable then set to null and OnInputStreamReady called then using the local variable.
Then we could just always throw if there is already a callback when AsyncWait is called.
Comments?

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #41)
> I think I covered all the nsIAsyncInputStream implementations. Maybe we want
> a similar approach for nsIAsyncOutputStream objects, but in a separate bug.

Definitely a separate bug, thanks.


(In reply to Olli Pettay [:smaug] from comment #42)
> Hmm, do we want to just return early if same callback is passed or call
> possibly asyncWait again on some internal stream?

I think it would be better to call on the internal stream as well, for consistency, as I suggested earlier in comment 28.

> 
> I guess we should ensure that callback member variable is stored on a local
> variable just before OnInputStreamReady is called, callback member variable
> then set to null and OnInputStreamReady called then using the local variable.

That's a standard and a must.

> Then we could just always throw if there is already a callback when
> AsyncWait is called.
> Comments?

See comment 28 again.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

comment 28 doesn't explain why.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

But I think I could live with that. Rather odd API, but ok.

Comment 46

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] from comment #44)
> comment 28 doesn't explain why.

The caller to AsyncWait doesn't know what stream it has in hands.  If it's a simple stream (e.g. socket transport) a duplicated call to AsyncWait is obviously made.  That can have some (desired) effects.  If the stream is e.g. a buffered stream, (nsBufferedStream -> nsSocketStream) and we don't forward, the socket transport's stream is not called the duplicate AsyncWait.  That can then behave a bit differently, potentially beyond expectation.

I don't have a strong and clear reason to do so, just my guts tell me that it is a more correct approach in general, as there is nearly infinite possible combinations of consumers and streams.  If you believe it's not necessary to call the inner stream, feel free to object and discuss further (comment 28 is now 14 days old).

Comment 47

[Honza Bambas (:mayhemer)]

Comment on attachment 8971513 [details] [diff] [review]
part 3 - Image inputStreams

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

this definitely needs an image lib peer review!

::: image/encoders/jpeg/nsJPEGEncoder.cpp
@@ +331,5 @@
> +    // We set the callback absolutely last, because NotifyListener uses it to
> +    // determine if someone needs to be notified.  If we don't set it last,
> +    // NotifyListener might try to fire off a notification to a null target
> +    // which will generally cause non-threadsafe objects to be used off the
> +    // main thread

I think this comment is no longer valid

Comment 48

[Honza Bambas (:mayhemer)]

Thanks, baku!  is there a try build?

Comment 49

[Andrea Marchesini [:baku]]

Attachment: part 8 - Pipe Stream

Yes, I pushed to try and I broke all. The reason why is because we have JS implementations of nsIInputStreamCallback interface and, of course, they are not thread-safe (probably we have also some C++ implementations, but this is not the reason why everything was orange).

When nsPipe InputStream::AsyncWait() is called with a nsInputStreamCallback and a nSIEventTarget, the code wraps these 2 parameters with a 
NS_NewInputStreamReadyEvent(): https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/xpcom/io/nsPipe3.cpp#1503-1508

nsPipe InputStream calls callback->OnInputStreamReady() on an I/O thread, but, because of NS_NewInputStreamReadyEvent, the callback is called and released on the correct eventTarget thread (the owning one, always the main-thread).

With my patch, I was creating NS_NewInputStreamReadyEvent just before calling OnInputStreamReady() and, doing so, I was increasing the refcount value of the JS callback out of the main-thread. This triggers the crash.

The non-thread-safe nsIInputStreamCallback doesn't seem to be documented or enforced in any other nsIAsyncInputStream implementation.

I tried to find a solution: I wrote a patch that implements a nsIInputStreamCallbackWrapper that wraps a JS nsIInputStreamCallback. The idea is to have a thread-safe wrapper, but it turned out that we have too many JS nsIInputStreamCallback objects.

For the scope of this bug, this patch fixes the crashes, but we have to decide if we want to go with the wrapper approach, or we want to enforce the releasing of the callback on the correct eventTarget thread, or something else.

Comment 50

[Andrea Marchesini [:baku]]

Attachment: part 8 - Pipe Stream

If you agree with this patch, I would like to extend this wrapper to other components.

Comment 51

[Andrea Marchesini [:baku]]

Attachment: part 8 - Pipe Stream

Comment 52

[Andrea Marchesini [:baku]]

Attachment: part D - documentation updated and InputStreamCallbackWrapper

Comment 53

[Andrea Marchesini [:baku]]

Attachment: part E - IDB + InputStreamCallbackWrapper

Comment 54

[Andrea Marchesini [:baku]]

Attachment: part F - image streams + InputStreamCallbackWrapper

I don't think this and IDB inputStream are directly exposed to JS, but it's better to cover all of the nsIAsyncInputStream implementations

Comment 55

[Andrea Marchesini [:baku]]

Attachment: part G - necko streams + InputStreamCallbackWrapper

I think I have covered all. The 'meta' inputStreams (sliced, mime, partially-seekable, ...) are already OK.

Comment 56

[Andrea Marchesini [:baku]]

Attachment: part E - IDB + InputStreamCallbackWrapper

Comment 57

[Andrea Marchesini [:baku]]

Attachment: part G - necko streams + InputStreamCallbackWrapper

Comment 58

[Andrea Marchesini [:baku]]

Attachment: part F - image streams + InputStreamCallbackWrapper

Comment 59

[Andrea Marchesini [:baku]]

Attachment: part D - documentation updated and InputStreamCallbackWrapper

Comment 60

[Andrea Marchesini [:baku]]

Attachment: part 8 - Pipe Stream

Comment 61

[Honza Bambas (:mayhemer)]

After talking to :baku on IRC I will postpone reviews here, this turned out to be more work with not high priority and with larger impact and I unfortunately won't be available for reviews next few weeks.

Comment 62

[Andrea Marchesini [:baku]]

Comment on attachment 8971978 [details] [diff] [review]
part E - IDB + InputStreamCallbackWrapper

Canceling the review requests because it turned out that we have many places where we replace the callback with a new one. TCPSocket for instance: here we set a WAIT_CLOSURE_ONLY callback in a nsPipeInputStream, but then we use a nsInputStreamPump. The pump uses AsyncWait(), overwriting the first callback.

Here 2 comments:
1. all these patches must be reconsidered.
2. what's the purpose of WAIT_CLOSE_ONLY in TCPSocket?