Closed
Bug 1371699
Opened 4 years ago
Closed 3 years ago
Implement nsIAsyncInputStream for string stream
Categories
(Core :: XPCOM, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: bzbarsky, Assigned: baku)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 10 obsolete files)
12.66 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
13.32 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
10.69 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.75 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
There's no reason we should use an offthread streampump to pump this.
![]() |
Reporter | |
Comment 1•4 years ago
|
||
This will make it simple to implement CloseWithStatus.
Attachment #8876565 -
Flags: review?(bkelly)
![]() |
Reporter | |
Comment 2•4 years ago
|
||
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)
![]() |
Reporter | |
Comment 3•4 years ago
|
||
This will prevent ambiguous nsIInputStream inheritance once we also inherit from nsIAsyncInputStream.
Attachment #8876567 -
Flags: review?(bkelly)
![]() |
Reporter | |
Comment 4•4 years ago
|
||
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)
![]() |
Reporter | |
Comment 5•4 years ago
|
||
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)
![]() |
Reporter | |
Comment 6•4 years ago
|
||
Attachment #8876570 -
Flags: review?(bkelly)
Comment 7•4 years ago
|
||
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 8•4 years ago
|
||
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-
![]() |
Reporter | |
Comment 9•4 years ago
|
||
> 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.
![]() |
Reporter | |
Comment 10•4 years ago
|
||
> 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 11•4 years ago
|
||
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)
![]() |
Reporter | |
Comment 12•4 years ago
|
||
> 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 13•4 years ago
|
||
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 14•4 years ago
|
||
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+
Comment 15•4 years ago
|
||
(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?
![]() |
Reporter | |
Comment 17•4 years ago
|
||
OK, fair. I'll give that change a spin through try and put it up for review when that's green.
Comment 18•4 years ago
|
||
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 19•4 years ago
|
||
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+
![]() |
||
Updated•4 years ago
|
Attachment #8876568 -
Flags: feedback?(nfroyd) → feedback+
![]() |
Reporter | |
Comment 20•4 years ago
|
||
nsIStringInputStream is in fact builtinclass, so we could provide a fast conversion there if you want.
![]() |
Reporter | |
Comment 21•4 years ago
|
||
Attachment #8876920 -
Flags: review?(bkelly)
![]() |
Reporter | |
Updated•4 years ago
|
Attachment #8876569 -
Attachment is obsolete: true
Comment 22•4 years ago
|
||
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+
Comment 23•4 years ago
|
||
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
Comment 24•4 years ago
|
||
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, ...)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3611ff7a79e6b616088bc4bf14e7ba8cdfe8bebf for the leaks and for causing bug 1372770.
Flags: needinfo?(bzbarsky)
![]() |
Reporter | |
Comment 26•4 years ago
|
||
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)
![]() |
Reporter | |
Comment 27•4 years ago
|
||
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.
Comment 28•4 years ago
|
||
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)
Comment 29•4 years ago
|
||
I'm gonna steal the high quality part 5 patch for release regression bug 1393063.
See Also: → 1393063
Comment 30•4 years ago
|
||
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.
![]() |
Reporter | |
Comment 32•4 years ago
|
||
This is not going to land any time soon, should not be uplifted anywhere, and shouldn't be tracked for 56.
Comment 33•4 years ago
|
||
(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)
![]() |
Reporter | |
Comment 34•4 years ago
|
||
> 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)
Comment 35•4 years ago
|
||
: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)
![]() |
||
Comment 36•4 years ago
|
||
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?
Assignee | ||
Comment 37•4 years ago
|
||
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)
Comment 38•4 years ago
|
||
Per comment #32, mark 56 won't fix.
Assignee | ||
Comment 39•4 years ago
|
||
I filed bug 1401171 to land part 4 of this bug. This can land immediately and it's not related to making nsStringInputStream async.
Assignee | ||
Comment 40•4 years ago
|
||
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)
Assignee | ||
Comment 41•4 years ago
|
||
Here the use of NonBlockingAsyncInputStream via NS_MakeAsyncNonBlockingInputStream.
Attachment #8912299 -
Flags: review?(nfroyd)
Assignee | ||
Comment 42•4 years ago
|
||
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 43•4 years ago
|
||
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 44•4 years ago
|
||
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)
Assignee | ||
Comment 45•4 years ago
|
||
Attachment #8912297 -
Attachment is obsolete: true
Attachment #8912580 -
Flags: review?(nfroyd)
Assignee | ||
Comment 46•4 years ago
|
||
Attachment #8912299 -
Attachment is obsolete: true
Attachment #8912581 -
Flags: review?(nfroyd)
Assignee | ||
Comment 47•4 years ago
|
||
Attachment #8912582 -
Flags: review?(nfroyd)
Assignee | ||
Updated•4 years ago
|
Attachment #8876565 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8876566 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8876567 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8876570 -
Attachment is obsolete: true
![]() |
||
Comment 48•4 years ago
|
||
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 49•4 years ago
|
||
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 50•4 years ago
|
||
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+
Assignee | ||
Comment 51•4 years ago
|
||
Attachment #8912582 -
Attachment is obsolete: true
Attachment #8914034 -
Flags: review?(nfroyd)
![]() |
||
Comment 52•4 years ago
|
||
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+
Comment 53•4 years ago
|
||
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)
Assignee | ||
Comment 55•4 years ago
|
||
Flags: needinfo?(amarchesini)
Attachment #8914825 -
Flags: review?(nfroyd)
![]() |
||
Comment 56•4 years ago
|
||
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+
Comment 57•4 years ago
|
||
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
Comment 58•4 years ago
|
||
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)
Assignee | ||
Comment 60•3 years ago
|
||
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 61•3 years ago
|
||
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+
Assignee | ||
Comment 62•3 years ago
|
||
> 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.
Comment 63•3 years ago
|
||
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
Comment 64•3 years ago
|
||
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
Backed out for failing wpt webvtt/parsing/cue-text-parsing/tests/entities.html on Linux (both bug 1405974 and bug 1371699): https://hg.mozilla.org/integration/mozilla-inbound/rev/99428b135058caf21100a375bcd0e9c03d16b257 https://hg.mozilla.org/integration/mozilla-inbound/rev/eef6e0a51e2d50db3b02fd10c83d88ddec58d758 https://hg.mozilla.org/integration/mozilla-inbound/rev/85a6496729e208255aeb2a3c3abba688d58bdf81 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca2fa99bd7e58722dc3f02952ad2d40c5f012cc https://hg.mozilla.org/integration/mozilla-inbound/rev/d3713e2970775bc22ac6c37d9339383c72632da3 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cf81d20f671314bd737e8db7ef9ed97f48f183b1&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=159621190&repo=mozilla-inbound TEST-UNEXPECTED-FAIL | /webvtt/parsing/cue-text-parsing/tests/entities.html | WebVTT cue data parser test entities - 3686fc0cdc60dc536e75df054b0bd372273db2cc - cue is undefined
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•3 years ago
|
Flags: needinfo?(amarchesini)
Comment 66•3 years ago
|
||
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
Comment 67•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/042c5cd8abb0 https://hg.mozilla.org/mozilla-central/rev/a1a8765481eb https://hg.mozilla.org/mozilla-central/rev/d691e5aaca74 https://hg.mozilla.org/mozilla-central/rev/5fa1a308b5c0 https://hg.mozilla.org/mozilla-central/rev/68c85fdcd9f6
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•