1093357 - Implement a cross-process pipe

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This still has some rough edges, but it ought to be good enough to start testing.

It applies on top of http://hg.mozilla.org/mozilla-central/rev/532df34a8482.  Unfortunately http://hg.mozilla.org/mozilla-central/rev/b2dc4f1dc9b3 rewrote the xpcshell test harness and my test doesn't work anymore :(

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'll work on incorporating this now.  Note, however, the HTTP channel code still has to be integrated, so I won't be able to do a full test till then.  I expect that nsm will use an nsPipe which will give me an nsPipeInputStream that I will pass to this new pipe.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Take all the time you need.  I'm happy not to touch this for a while ;)

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What do I need to do to my xpcshell test to update it for bug 1033126?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Well, I'm hoping to get an initial version of Cache landed in the next 3 weeks.  So it would be great if we could finalize this by then as well.  Thanks!

Comment 5

[Chris Manchester (limited bugmail, email directly)]

The logging changes shouldn't impose new requirements on tests, so maybe you found a bug. I'll take a look.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I got an error when compiling on linux gcc 4.7.3:

/srv/maple/ipc/glue/CrossProcessPipe.cpp:911:2086: error: deleting object of polymorphic class type ‘{anonymous}::CrossProcessPipe’ which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor]

I had to add virtual destructors to:

  SerializationPlaceholderStream
  CrossProcessPipe
  TestShellTest
  TestShellTestParent
  TestShellTestChild

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Chris Manchester [:chmanchester] from comment #5)
> The logging changes shouldn't impose new requirements on tests, so maybe you
> found a bug. I'll take a look.

This test is kind of ... special.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Nit: It would be a nice convenience if there were getters for just nsIInputStream and nsIOutputStream.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> (In reply to Chris Manchester [:chmanchester] from comment #5)
> > The logging changes shouldn't impose new requirements on tests, so maybe you
> > found a bug. I'll take a look.
> 
> This test is kind of ... special.

Unearthed bug 1093834

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8516316 [details] [diff] [review]
Patch

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

I integrated this into my patch queue against maple.  Overall it seems to work well in e10s mode.

Unfortunately, though, I also need support for same-process since PBackground is designed to abstract that away.  The Cache always goes through the PBackground IPC layer so that it doesn't need special same-process vs out-of-process logic.

So I think CrossProcessPipe should at least detect same-process and use a shared nsIPipe under the hood in that case.

Even better would be to somehow integrate this into a serialization handler for nsIPipe directly in SerializeInputStream.  If an nsPipeInputStream is passed then a CrossProcessPipe would automatically be created and copied to for out-of-process.  If its same-process, then the existing nsIPipe would be handed across the IPC boundary behind the scenes.

Finally, I found a few issues while running my cache test in e10s mode with the pipe.  See the comments below and the previous comment about the virtual destructors.

Thanks for working on this!  f- for now due to same-process issue.

::: ipc/glue/CrossProcessPipe.cpp
@@ +290,5 @@
> +    if (mInMainProcess) {
> +      mozilla::ipc::AssertIsOnBackgroundThread();
> +    }
> +
> +    mTransportThread = do_GetCurrentThread();

Please document that this class cannot be used on a thread pool (like StreamTransportService).  If you can assert that, that would be ideal.

@@ +603,5 @@
> +  }
> +
> +  if (*aBytesRead > 0) {
> +    return NS_OK;
> +  }

Please explicitly set *aBytesRead to zero at the start of ReadSegments().  Currently a stream copy can get into a loop if a non-zero *aBytesRead is passed in and NS_BASE_STREAM_WOULD_BLOCK should be returned since this conditional is not hit.

@@ +824,5 @@
> +  MOZ_ASSERT(!mIsTransportActive);
> +
> +  NoteTransportThread();
> +
> +  if (!mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this)) {

It would be nice to call BackgroundChild::GetForCurrentThread() first to see if the actor is already created.  This avoids an unnecessary async thread schedule in the most common case.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ben Kelly [:bkelly] from comment #10)
> Comment on attachment 8516316 [details] [diff] [review]
> Patch
> 
> Review of attachment 8516316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I integrated this into my patch queue against maple.  Overall it seems to
> work well in e10s mode.
> 
> Unfortunately, though, I also need support for same-process since
> PBackground is designed to abstract that away.  The Cache always goes
> through the PBackground IPC layer so that it doesn't need special
> same-process vs out-of-process logic.

Noted.

> So I think CrossProcessPipe should at least detect same-process and use a
> shared nsIPipe under the hood in that case.

I'm not sure what the best way to implement this is yet, but yeah, we should support the same process case without serializing everything through IPDL.

> Even better would be to somehow integrate this into a serialization handler
> for nsIPipe directly in SerializeInputStream.  If an nsPipeInputStream is
> passed then a CrossProcessPipe would automatically be created and copied to
> for out-of-process.  If its same-process, then the existing nsIPipe would be
> handed across the IPC boundary behind the scenes.

That's interesting.  I'll have to think more about that.

> ::: ipc/glue/CrossProcessPipe.cpp
> @@ +290,5 @@
> > +    if (mInMainProcess) {
> > +      mozilla::ipc::AssertIsOnBackgroundThread();
> > +    }
> > +
> > +    mTransportThread = do_GetCurrentThread();
> 
> Please document that this class cannot be used on a thread pool (like
> StreamTransportService).  If you can assert that, that would be ideal.

I'll have to investigate what goes wrong a bit more.

> @@ +603,5 @@
> > +  }
> > +
> > +  if (*aBytesRead > 0) {
> > +    return NS_OK;
> > +  }
> 
> Please explicitly set *aBytesRead to zero at the start of ReadSegments(). 
> Currently a stream copy can get into a loop if a non-zero *aBytesRead is
> passed in and NS_BASE_STREAM_WOULD_BLOCK should be returned since this
> conditional is not hit.

Noted.

> @@ +824,5 @@
> > +  MOZ_ASSERT(!mIsTransportActive);
> > +
> > +  NoteTransportThread();
> > +
> > +  if (!mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this)) {
> 
> It would be nice to call BackgroundChild::GetForCurrentThread() first to see
> if the actor is already created.  This avoids an unnecessary async thread
> schedule in the most common case.

Nice catch.  I had assumed that we would get called back synchronously if we already had the actor constructed, but that's not the case.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

> > Please document that this class cannot be used on a thread pool (like
> > StreamTransportService).  If you can assert that, that would be ideal.
> 
> I'll have to investigate what goes wrong a bit more.

Pretty sure trying to use PBackground and its thread local state on a thread pool is just always going to be bad news.

> > Even better would be to somehow integrate this into a serialization handler
> > for nsIPipe directly in SerializeInputStream.  If an nsPipeInputStream is
> > passed then a CrossProcessPipe would automatically be created and copied to
> > for out-of-process.  If its same-process, then the existing nsIPipe would be
> > handed across the IPC boundary behind the scenes.
> 
> That's interesting.  I'll have to think more about that.

Seems possible, although I'm not sure which thread you could use to do the NS_AsyncCopy from the passed nsPipeInputStream to the CrossProcessPipe.  We almost need some non-pool version of STS.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well the pipe is designed to accept Write()/etc on a different thread than the actual transport actor lives on.  I suspect the real problem is that the thread the actor is created on shuts down before the pipe is finished writing.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Write may work from other threads as long as the transport is activated.  I was seeing the ActorCreated() callback being lost because the thread went away. :-\

Maybe activate the transport in the constructor to get a stable thread?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, maybe.  Will have to think about it.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8516316 [details] [diff] [review]
Patch

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

::: ipc/glue/CrossProcessPipe.cpp
@@ +248,5 @@
> +    , mLock("XPPActiveStream lock")
> +  {
> +    if (!sInFlightList) {
> +      sInFlightList = new nsTArray<nsRefPtr<XPPActiveStream>>;
> +    }

Also, shouldn't this be done GetOrCreateForId() under the mutex?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Revised.  The API has changed a bit.  We have to get a ContentParent pointer to initialize the pipe, unfortunately.  Without that we can't link up the two sides in the ContentParent case.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

That crash you saw was a dumb mistake.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8537338 [details] [diff] [review]
Patch

This passes my tests.  I haven't looked at the code at all, though.  Thanks!

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, I wanted to know if it works for your purposes.  Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Also note that I pretty much only tried this for small streams that were probably sent over in a single chunk.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Flagging jduell to review the stream implementation and bent for the IPC stuff.

We do still need to change the external interface so that you only have to pass in the content parent once.

Comment 23

[Jason Duell]

Comment on attachment 8553246 [details] [diff] [review]
Patch

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

I'm giving this to Honza (note: you only need to look at the streams part of the patch)

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8553246 [details] [diff] [review]
Patch

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

::: ipc/glue/CrossProcessPipe.cpp
@@ +132,5 @@
> +{
> +  MOZ_ASSERT(mCrossProcessPipe);
> +
> +  mozilla::ipc::PipeInputStreamParams params;
> +  params.pipeId() = (intptr_t)mCrossProcessPipe.get();

hmm.. so ptr value (cast to int) is used as an id... that is dangerous, since ptrs gets recycled (mainly on win) and you have to take *great* care to never link two objects that actually don't belong each other.  since we are in IPC world, it may be tricky to keep this sync all the time.  you are opening a chance for a flaw..

@@ +314,5 @@
> +  // nsIInputStream
> +  NS_IMETHOD
> +  Close() MOZ_OVERRIDE
> +  {
> +    return CloseWithStatus(NS_OK);

close with NS_BASE_STREAM_CLOSED or let CloseWithStatus convert NS_OK to NS_BASE_STREAM_CLOSED (preferred).

@@ +376,5 @@
> +
> +/*static*/
> +mozilla::StaticMutex XPPActiveStream::sInFlightTableLock;
> +/*static*/
> +nsAutoPtr<InfallibleTArray<nsRefPtr<XPPActiveStream>>> XPPActiveStream::sInFlightTable;

isn't this a static initializer?

@@ +407,5 @@
> +  MOZ_ASSERT(mIsTransportActive);
> +
> +  mozilla::MutexAutoLock lock(mLock);
> +
> +  // XXXkhuey should CloseWithStatus

hmm.. I think not.  You do two things:
- always post OnReady events and don't respect mIsClosed
- check for mIsClosed in ReadSegments() as the last thing

This both ensures that all data are delivered despite the stream is closed.  nsPipe3 behaves the same.

@@ +411,5 @@
> +  // XXXkhuey should CloseWithStatus
> +  mResultCode = aResult;
> +  mClosed = true;
> +  MaybeFireCallbacks();
> +  mIsTransportActive = false;

maybe add few notes why you change the states/call MaybeFireCallbacks this way.

@@ +445,5 @@
> +  }
> +
> +  // But if the transport was interrupted we need to close things.
> +  // XXXtodo close things
> +  mIsTransportActive = false;

definitely at least NS_WARNING this ;)

@@ +496,5 @@
> +XPPActiveStream::CloseWithStatus(nsresult aRv)
> +{
> +  if (mClosed) {
> +    return NS_OK;
> +  }

two tight threads could both pass this check and enter this method.

@@ +515,5 @@
> +    if (mIsTransportActive) {
> +      if (IsOnTransportThread()) {
> +        CloseTransport(aRv);
> +      }
> +      else {

} else { ?

@@ +572,5 @@
> +    if (!mClosed) {
> +      return NS_OK;
> +    }
> +
> +    return mResultCode == NS_OK ? NS_BASE_STREAM_CLOSED : mResultCode;

NS_SUCCEEDED?  or do you expect some NS_SUCCESS_XXX code be in mResultCode?  Also, mResultCode should not be a success when the stream is closed (as above)

@@ +599,5 @@
> +    auto countToCopy = std::min<uint32_t>(aCount, mData.Length());
> +    while (countToCopy) {
> +      uint32_t countCopied = 0;
> +      nsresult rv = aWriter(this, aClosure, (char*)mData.Elements(), *aBytesRead,
> +                            countToCopy, &countCopied);

hopefully no-one will reenter this stream's methods from aWriter

@@ +607,5 @@
> +        return NS_OK;
> +      }
> +
> +      MOZ_ASSERT(countCopied <= countToCopy);
> +      mData.RemoveElementsAt(0, countCopied);

when you remove the elements you've copied you break the nsWriteSegmentFun contract.  Either don't remove (preferred) or pass 0 instead of *aBytesRead as the offset.

@@ +618,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (mClosed) {
> +    return NS_OK;

this should return (failed) mResultCode.

@@ +634,5 @@
> +  {
> +    mozilla::MutexAutoLock lock(mLock);
> +
> +    if (mClosed) {
> +      return NS_BASE_STREAM_CLOSED;

return mResultCode

@@ +696,5 @@
> +      uint32_t oldLength = mData.Length();
> +      mData.SetLength(oldLength + aCount);
> +
> +      nsresult rv = aReader(this, aClosure, (char*)mData.Elements(),
> +                            *aBytesWritten, aCount, &countRead);

IMO would be safer (from point of maintaining this code) to pass here |*aBytesWritten + originalLength|.  having to subtract originalLength before every return statement is not good.

@@ +711,5 @@
> +
> +    *aBytesWritten -= originalLength;
> +  }
> +
> +  // XXXkhuey queue flush.

so, seems like you just let the data linger but until flush is called, nothing happens.  this is for r-

@@ +725,5 @@
> +  MOZ_ASSERT(IsReceiving());
> +
> +  nsCOMPtr<nsIEventTarget> currentThread = do_GetCurrentThread();
> +  if (aEventTarget && currentThread != aEventTarget) {
> +    return NS_ERROR_NOT_IMPLEMENTED;

NS_ERROR this.

OTOH, why exactly this limitation?  I can see the stream is mostly well thread-safe and you always post anyway.

@@ +733,5 @@
> +
> +  MOZ_ASSERT(!mOutputAsyncWaitCallback);
> +  if (NS_WARN_IF(mInputAsyncWaitCallback)) {
> +    MOZ_ASSERT(mResultCode == NS_OK); // We shouldn't be closed if we're waiting
> +    return NS_ERROR_UNEXPECTED;

I wouldn't do this.  It's OK to call this more then once even with a different callback.  Maybe only NS_WARN that callback is exchanged?

@@ +738,5 @@
> +  }
> +
> +  // WAIT_CLOSURE_ONLY is kind of silly.
> +  if (aFlags & nsIAsyncInputStream::WAIT_CLOSURE_ONLY) {
> +    return NS_ERROR_NOT_IMPLEMENTED;

please log/NS_ERROR this... better to see what we break

@@ +748,5 @@
> +  if (aRequestedCount == 0) {
> +    aRequestedCount = 1;
> +  }
> +
> +  mRequestedAsyncWaitCount = aRequestedCount;

this is widely not implemented and we could live well w/o this support

@@ +754,5 @@
> +  mInputAsyncWaitCallback =
> +    NS_NewInputStreamReadyEvent(aCallback, currentThread);
> +
> +  if (!mIsTransportActive && !mClosed && !mInMainProcess) {
> +    InitializeTransport();

so, you happily ignore the result?

@@ +764,5 @@
> +}
> +
> +NS_IMETHODIMP
> +XPPActiveStream::AsyncWait(nsIOutputStreamCallback* aCallback, uint32_t aFlags,
> +                           uint32_t aRequestedCount, nsIEventTarget* aEventTarget)

applies the same for this one...

@@ +844,5 @@
> +  if (!mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this)) {
> +    CloseWithStatus(NS_ERROR_FAILURE);
> +    return NS_ERROR_FAILURE;
> +  }
> +    

ws

@@ +882,5 @@
> +                                    mClosed)) {
> +      NS_WARNING("Firing callback");
> +      mInputAsyncWaitCallback->OnInputStreamReady(this);
> +      mRequestedAsyncWaitCount = 0;
> +      mInputAsyncWaitCallback = nullptr;

please add a comment that m*WaitCallback is always an event that dispatches.  otherwise this code would be completely wrong.
OTOH, could we avoid posting if possible?  that has great impact on performance when target is the main thread.  (for non main threads I would not bother).

@@ +920,5 @@
> +    for (uint32_t length = XPPActiveStream::sInFlightTable->Length(), i = 0; i < length; ++i) {
> +      nsRefPtr<XPPActiveStream>& stream = XPPActiveStream::sInFlightTable->ElementAt(i);
> +      if (stream->PipeId() == aPipeId) {
> +        if (stream->ContentParentId() == aContentParentId) {
> +          NS_WARNING("Found existing stream in default process");

why the warning?

@@ +1051,5 @@
> +  nsRefPtr<XPPActiveStream> stream =
> +    dont_AddRef(XPPActiveStream::FromParent(aActor));
> +
> +  // XXXkhuey should we check the table to see if the stream is still in it?
> +  // That could happen if the stream is never serialized.

you definitely need to take good care to keep the table in 100% good health.

::: ipc/glue/CrossProcessPipe.h
@@ +60,5 @@
> +GetOrCreatePipeInputStreamForId(intptr_t aContentParent, intptr_t aPipeId);
> +
> +} // namespace detail
> +} // namespace ipc
> +} // namspace mozilla

namspace

::: ipc/glue/InputStreamUtils.cpp
@@ +161,5 @@
> +	  auto contentParent = ContentParent::GetCurrentlyProcessingMessages();
> +	  contentParentId = (intptr_t)static_cast<nsIContentParent*>(contentParent);
> +	  NS_WARNING("Got ContentParent contentParent");
> +	}
> +	if (!contentParentId) {

missing else or what?  or are the messages in NS_WARNINGs just wrong?

@@ +163,5 @@
> +	  NS_WARNING("Got ContentParent contentParent");
> +	}
> +	if (!contentParentId) {
> +	  // Else we must be on a BackgroundChild.
> +	  // XXXkhuey assert that.

you are in XRE_GetProcessType() == GeckoProcessType_Default branch

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +495,5 @@
> +        nsCOMPtr<nsIXPConnectJSObjectHolder> wrappedNative;
> +        xpc->WrapNative(cx, JS_THIS_OBJECT(cx, vp), test,
> +                        NS_GET_IID(nsITestShellTest),
> +                        getter_AddRefs(wrappedNative));
> +        

ws

::: xpcom/build/XPCOMInit.cpp
@@ +29,5 @@
>  #include "nsScriptableInputStream.h"
>  #include "nsBinaryStream.h"
>  #include "nsStorageStream.h"
>  #include "nsPipe.h"
> +#include "mozilla/ipc/CrossProcessPipe.h"

really needed?

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8553246 [details] [diff] [review]
Patch

And also forgot to mention: please turn some of your "logging" NS_WARNINGs to regular NSPR logs (always useful!)

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Kyle, I recently tried this patch in this patch queue:

  https://github.com/wanderview/gecko-patches/tree/serviceworkers

I got a failure in non-e10s mode:

[17760] WARNING: Created new stream in default process: file /srv/mozilla-central/ipc/glue/CrossProcessPipe.cpp, line 939
[17760] WARNING: Got stuff, deserializing: file /srv/mozilla-central/ipc/glue/InputStreamUtils.cpp, line 144
[17760] WARNING: Got PBackground contentParent: file /srv/mozilla-central/ipc/glue/InputStreamUtils.cpp, line 158
[17760] WARNING: Got nothing: file /srv/mozilla-central/ipc/glue/InputStreamUtils.cpp, line 168
[17760] WARNING: Found existing stream in default process: file /srv/mozilla-central/ipc/glue/CrossProcessPipe.cpp, line 927
[17760] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../dist/include/nsCOMPtr.h, line 401
#01: mozilla::ipc::DeserializeInputStream(mozilla::ipc::InputStreamParams const&, nsTArray<mozilla::ipc::FileDescriptor> const&) (/srv/mozilla-central/ipc/glue/InputStreamUtils.cpp:173)
#02: already_AddRefed<nsIInputStream>::take() (/srv/mozilla-central/obj-x86_64-unknown-linux-gnu-debug/ipc/glue/../../dist/include/mozilla/AlreadyAddRefed.h:109)
#03: mozilla::dom::cache::CacheParent::DeserializeCacheStream(mozilla::dom::cache::PCacheReadStreamOrVoid const&) (/srv/mozilla-central/dom/cache/CacheParent.cpp:253)
[17760] WARNING: Maybe fire callbacks: file /srv/mozilla-central/ipc/glue/CrossProcessPipe.cpp, line 879
#04: mozilla::dom::cache::CacheParent::RecvPut(unsigned long const&, mozilla::dom::cache::CacheRequestResponse const&) (/srv/mozilla-central/dom/cache/CacheParent.cpp:127)

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8553246 [details] [diff] [review]
Patch

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

::: ipc/glue/BackgroundImpl.cpp
@@ +1833,5 @@
> +ChildImpl::GetCurrentlyProcessingMessages()
> +{
> +  auto actor = static_cast<ChildImpl*>(ChildImpl::GetForCurrentThread());
> +
> +  if (!actor || !actor->mCurrentlyProcessingMessages) {

mCurrentlyProcessingMessages is debug only.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #19)
> Comment on attachment 8537338 [details] [diff] [review]
> Patch
> 
> This passes my tests.  I haven't looked at the code at all, though.  Thanks!

Hmm, I can no longer duplicate this on non-e10s.

Kyle, should the CrossProcessPipe work for non-e10s when sending through PBackground?  I keep hitting the MOZ_ASSERT(IsReceiving()) in XPPActiveStream::ReadSegments().

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Kyle, any updates on this? Thanks!

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #29)
> Kyle, any updates on this? Thanks!

Nikhil, I talked to Kyle and I'm working on an alternative cache-specifix solution.  Do you have another use case?

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I don't, just concerned about offline script caching.

Comment 32

[Andrew Overholt [:overholt]]

Ben told me we're going to go with a Cache-specific pipe so I'm dropping the dependency here.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We decided to go with bkelly's solution.

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Ben Kelly [:bkelly] from comment #30)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #29)
> > Kyle, any updates on this? Thanks!
> 
> Nikhil, I talked to Kyle and I'm working on an alternative cache-specifix
> solution.  Do you have another use case?

Ben, any idea how far away this current solution is?  I believe the DevTools team could use something like this to improve some high-bandwidth code paths for e10s.

Are there any major architectural issues?  Or is it more a matter of getting tests to pass, etc.?

Also, is there a bug / where can I learn about the service worker version you're changing to?

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Kyle might have a better insight into the exact issues here, but I think much of the difficulty comes down to meeting the following requirements:

1) IPC actors must be used from their owning thread
2) IPC actors are not ref-counted
3) IPC actors can go away at any time
4) Stream objects are ref-counted
5) Stream objects must be usable on other threads like STS

Issues (1) to (3) are really hard to hide from client code while also providing an end-point that can be used like (4) and (5).  Being general purpose in an IPC-agnostic really ups the complexity it seems.

I think if we wanted some kind of IPC streaming capability we should be a little less general and not try to hide the IPCness.  Instead, provide an actor type in ipc/glue that is like the PCachePushStream I wrote in bug 1110814.  This would be an actor that drains an nsIInputStream on one side and the other side's actor would just dump bytes into an nsPipe.  What I wrote is currently tightly coupled to Cache and only goes from child-to-parent, but it could probably be broken out into something slightly more general.  Then things like devtools would have to declare that they create one of these actors and ensure its used according to all the IPC rules.

Hope that helps.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Josh also indicated he has a need for this for handling synthesized redirects with e10s.

I'm going to re-open for now.  We can talk about the best way to implement.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I would encourage anyone who wants to do this to not make it as "magic" as I attempted to.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

We're also going to need something like this when fetch Request bodies can be streamed from js in the future.  I'll try to figure out a course of action.  Probably not until Q3.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, how urgently do we need this?  I could possibly punt some work to Q3 and work on it immediately.

Comment 41

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #40)
> Ehsan, how urgently do we need this?  I could possibly punt some work to Q3
> and work on it immediately.

Not urgently at all, this only causes us to disable two tests in e10s for now (see https://hg.mozilla.org/integration/mozilla-inbound/rev/a207eb8fe63a).  This can wait until Q3, I think.

Comment 42

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

For what it's worth, I'm going to wind up reinventing this wheel over in bug 1101100 (the use case is an nsIOutputStream in the parent that the child writes to), but fortunately nsIDocumentEncoder is all main-thread I/O so I won't need to deal with PBackground.

Also, as I understand it IPC has no flow control, so if the protocol just sends async messages of uint8_t[] and the receiving process can't keep up, the IPC buffer memory usage will increase without bound.  That might be a problem.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jed Davis [:jld] {UTC-7} from comment #42)
> For what it's worth, I'm going to wind up reinventing this wheel over in bug
> 1101100 (the use case is an nsIOutputStream in the parent that the child
> writes to), but fortunately nsIDocumentEncoder is all main-thread I/O so I
> won't need to deal with PBackground.

Actually, this could be the exact same use case.  If we make pipe input stream serializable then you just instantiate a pipe on the child, write to it, and then serialize the reader side.  So you don't have to serialize nsIOutputStream at all.

> Also, as I understand it IPC has no flow control, so if the protocol just
> sends async messages of uint8_t[] and the receiving process can't keep up,
> the IPC buffer memory usage will increase without bound.  That might be a
> problem.

Correct there is no flow control in what I implemented in Cache.  It could be added with a windowing scheme in the future, though.

I think if you used the pipe case I outlined above you would at least get some flow control as the pipe has a limited immediate buffer.

How soon do you need this?  I have some plans already in mind.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Obviously you would then have to NS_AsyncCopy() from the serialized pipe input stream to your output stream.

Also, what is the actual nsIOutputStream in the parent?

Comment 45

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Ben Kelly [:bkelly] from comment #44)
> Also, what is the actual nsIOutputStream in the parent?

Either an nsIFileOutputStream or an nsIStorageStream, and that's a good point: those could be changed to PR_Open and NS_OpenAnonymousTemporaryFile respectively, to use ipc::FileDescriptor.  So I can do that, if I don't (or reviewers don't) like throwing around uint8_t[]s.

I wonder if it would be possible to create a simple thread-safe cross-process pipe the same way: use PR_CreatePipe or equivalent, pass a FileDescriptor over IPC, then inherit from nsFileStreamBase to adapt NSPR I/O to XPCOM.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

As far as I know, we don't allow writing to an nsIFileOutputStream in the child process because we can't guarantee quota guarantees, etc.  You might want to talk to bent about that.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

We need this to ship service workers because otherwise network uploads fail with interception in e10s mode.

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Neither desktop nor Android use e10s on the release channel.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

We had to disable service worker interception on aurora desktop when e10s was enabled there.  See bug 1180886.

Also, I think its hard to say whether e10s or service workers will go to desktop first (or at the same time).

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

If we're going to have any hope of enabling interception on aurora we have to fix this.  Taking to work immediately.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

I plan to implement this to support child-to-parent streaming for now.  I don't know of a use case for streaming parent-to-child since that is already covered by necko's IPC code.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

I was looking at the crashes in bug 1180886 to try to determine what kind of test I need to provoke the original problem.  I think something like the following would work:

0) Register a service worker.
1) Create blob.
2) Put blob in IDB.
3) Get blob out of IDB.  This gives us a file backed blob.
4) Perform an XHR.send() with the file backed blob.
5) Ignore the result SW FetchEvent.  Let the default thing happen.
6) See if the origin channel can still perform the upload.

This fails because between (4) and (5) the ServiceWorkerManager performs a clone:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3736

Since file streams cannot be cloned directly we end up with a pipe instead.  The pipe cannot be serialized over IPC.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=khuey

Port the PCachePushStream protocol to a general ipc/glue PSendStream protocol.  Please see the SendStream.h header comments for some explanation of its use.  The next patch will provide an RAII object to make it easier to use.  I need the SendStream actor exposed itself, though, so that I can use it directly in Cache.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add an RAII type to safely handle serialized stream actors. r=khuey

Add an RAII class to make it easier to use the new SendStream actor in other code.  This should integrate easily with most code.  (Cache is a different story since it wants to extend the stream concept with an additional actor.)

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Convert Cache to use SendStream instead of CachePushStream. r=ehsan

This is largely a mechanical replacement of CachePushStream with the new ipc::SendStream.

I also had to adjust some headers due to unified bustage when I removed the CachePushStream files.  I fixed some header include ordering while I was there.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update necko HTTP upload stream to use send stream actor. r=jduell

Modify PNeckoChild and http channels to support the SendStream actor for the uploadStream.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

I still need to add a test that exercises the conditions in comment 52, but I wanted to get these patches in for review since they pass existing tests and Kyle is going on travel soon.

The current Cache tests fully exercise the new actor on PBackground.  The existing mochitests that do uploads exercise much of the AutoIPCStreamChild class via necko.

The new test is necessary to verfiy the AutoIPCStreamChild works when SendStream is needed.  This will also verify SendStream on PContent.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Explicitly advertise that nsPipeInputStream supports ReadSegments(). r=froydnj

Make nsPipeInputStream explicitly implement nsIBufferedInputStream so that NS_IsBufferedInputStream() always returns for pipes streams.  This allows us to avoid wrapping pipe streams with nsBufferedInputStreams in necko.

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0.1 Implement an NS_InputStreamIsCloneable() method. r=froydnj

Add a simple utility method to determine if an nsIInputStream can be directly cloned.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0.2 HTTP channels should clone streams instead of rewinding when possible. r=jduell

Now that we can end up with nsPipeInputStream objects as the http upload stream, we can no longer solely rely on rewinding the stream when a redirect occurs.  While nsPipeInputStream implements nsISeekableStream, it doesn't actually do anything.

Instead, we should first try to clone the upload stream before we consume it.  This is a very cheap operation for all streams that directly support cloning.  When this is possible, we no longer need to try rewinding the stream.

If cloning is not directly supported (like for file streams), then we do use nsISeekableStream to rewind.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Test file blob uploads with service worker interception. r=ehsan

Work-in-progress test that uploads a file-backed blob.  Right now its just a small blob with an empty service worker.  I want to expand this to exercise redirects before flagging for review.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Bill, do you think you would be able to help review the ipc/glue code in this bug?  Kyle is on travel and we'd like to get this landed at the end of the week if possible so we have some bake time before the merge.

I should have refreshed patches ready later today or tonight.

Comment 64

[Nathan Froyd [:froydnj]]

Comment on attachment 8658475 [details] [diff] [review]
P0 Explicitly advertise that nsPipeInputStream supports ReadSegments(). r=froydnj

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

::: xpcom/io/nsPipe3.cpp
@@ +1116,2 @@
>                          nsIClassInfo)
> +*/

Please just delete the commented-out code.

@@ +1125,5 @@
> +    NS_INTERFACE_TABLE_ENTRY(nsPipeInputStream, nsIClassInfo)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsPipeInputStream, nsIInputStream,
> +                                       nsIAsyncInputStream)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsPipeInputStream, nsISupports,
> +                                       nsIAsyncInputStream)

Can you add tests for all these entries, just to make sure we got the table correct?

@@ +1143,5 @@
> +NS_IMETHODIMP
> +nsPipeInputStream::Init(nsIInputStream*, uint32_t)
> +{
> +  MOZ_CRASH("nsPipeInputStream should never be initialized with "
> +            "nsIBufferedInputStream::Init!\n");

Weren't you just complaining about classes that implement interfaces, only to have those implementations do nothing useful? ;)

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Test file blob uploads with service worker interception. r=nsm

This test now exercises POST requests using file backed blobs for the uploads for single requests and redirected tests.  With very large sizes it can also provoke the failure described in bug 1134372.

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0.2 HTTP channels should clone streams instead of rewinding when possible. r=jduell

Update the http cloning patch to also clone when consuming the stream during open on the child side.  I think this is necessary to allow the upload stream to be correct during redirect callbacks in the child.

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=khuey

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add an RAII type to safely handle serialized stream actors. r=khuey

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 70

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8658933 [details] [diff] [review]
P1 Implement an actor for streaming pipes from child to parent. r=khuey

(Taking over the reviews for these two, as discussed on IRC.)

Comment 71

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8658924 [details] [diff] [review]
P5 Test file blob uploads with service worker interception. r=nsm

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

::: dom/workers/test/serviceworkers/redirect_post.sjs
@@ +19,5 @@
> +  var body = decodeURIComponent(
> +    escape(String.fromCharCode.apply(null, bodyBytes)));
> +
> +  // coerce to a number
> +  var currentHop = ~~query.hop;

Please use parseInt(query.hop), if hop is unset this will return NaN. You can explicitly Number.isNaN() it and set to 0.

@@ +26,5 @@
> +  if (currentHop < obj.hops) {
> +    var newURL = '/tests/dom/workers/test/serviceworkers/redirect_post.sjs?hop=' +
> +                 (1 + currentHop);
> +    response.setStatusLine(null, 307, 'redirect');
> +    response.setHeader('Location', newURL);

return; or do you want to send the body in the response even on redirects?

::: dom/workers/test/serviceworkers/test_file_blob_upload.html
@@ +4,5 @@
> +-->
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test interception of file blob uploads</title>

Bug number

@@ +23,5 @@
> +  }
> +
> +  function unregister() {
> +    if (iframe) {
> +      var content = document.getElementById("content");

you can simply do iframe.remove()

@@ +30,5 @@
> +
> +    return registration.unregister().then(function(result) {
> +      ok(result, "Unregister should return true.");
> +    }, function(e) {
> +      dump("Unregistering the SW failed with " + e + "\n");

Also, ok(false, ...)

Comment 72

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8658933 [details] [diff] [review]
P1 Implement an actor for streaming pipes from child to parent. r=khuey

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

r=me modulo comments.

::: ipc/glue/SendStreamChild.cpp
@@ +237,5 @@
> +  static const uint64_t kMaxBytesPerMessage = 32 * 1024;
> +  static_assert(kMaxBytesPerMessage <= static_cast<uint64_t>(UINT32_MAX),
> +                "kMaxBytesPerMessage must cleanly cast to uint32_t");
> +
> +  while (!mClosed) {

Is there anything in the loop that could change the value of mClosed and isn't followed by a return?

@@ +263,5 @@
> +    buffer.SetLength(expectedBytes);
> +
> +    uint32_t bytesRead = 0;
> +    rv = mStream->Read(buffer.BeginWriting(), buffer.Length(), &bytesRead);
> +    buffer.SetLength(bytesRead);

Is it safe to depend on the value of bytesRead when rv is a failure?

::: ipc/glue/SendStreamParent.cpp
@@ +72,5 @@
> +
> +  // This should only fail if we hit an OOM condition.
> +  nsresult rv = mWriter->Write(aBuffer.get(), aBuffer.Length(), &numWritten);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    RecvClose(rv);

There's a problem with this: If there's another Buffer message in flight then it will be received after the actor is destroyed, and we'll intentionally crash both processes.  If this can only fail due to OOM, then it means we're more or less treating the pipe buffer as infallible, which is still probably not a good thing.

One solution would be to send the child a RequestClose message, which it responds to with Close, which the parent responds to with __delete__ (having set a flag to ignore any more Buffer messages).  That's a little tedious, but it seems to be the usual way to avoid this kind of problem.  This would also allow some IPDL state declarations like this (untested):

state START:
  recv Buffer goto START;
  recv Close goto CLOSING;
  send RequestClose goto REQCLOSE;
state REQCLOSE:
  recv Buffer goto REQCLOSE; // ignored due to error on earlier write
  recv Close goto CLOSING;
state CLOSING:
  send __delete__;

The important property here is that no state transition removes options from the opposite side; i.e., there's no way to get an unexpected message because conflicting messages crossed in transit.  However, invalid state transitions aren't enforced by the IPDL compiler yet, only NS_WARNING()ed.

@@ +95,5 @@
> +  MOZ_ASSERT(mReader);
> +  MOZ_ASSERT(mWriter);
> +}
> +
> +} // anonymouns namespace

Nit: typo of "anonymous".

Comment 73

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8658934 [details] [diff] [review]
P2 Add an RAII type to safely handle serialized stream actors. r=khuey

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

r=me with a few comments.

::: ipc/glue/InputStreamParamsOrSendStream.ipdlh
@@ +7,5 @@
> +
> +namespace mozilla {
> +namespace ipc {
> +
> +struct InputStreamParamsOrSendStream

This name is a little awkward… but I can't think of anything better to suggest.

@@ +11,5 @@
> +struct InputStreamParamsOrSendStream
> +{
> +  OptionalInputStreamParams optionalStream;
> +  OptionalFileDescriptorSet optionalFds;
> +  nullable PSendStream optionalSendStream;

It might be good to have a comment about why the struct is defined like this, instead of as a union of PSendStream and a struct for the other case.  (I notice that the all-null case is used internally.)

::: ipc/glue/InputStreamUtils.cpp
@@ +289,5 @@
> +      sendStream->StartDestroy();
> +      return;
> +    }
> +
> +    // If we the SendStream was taken to be sent to the parent, then we need to

Nit: "If we the"?

@@ +404,5 @@
> +void
> +AutoIPCStreamChild::SerializeOrSend(nsIInputStream* aStream,
> +                                    PBackgroundChild* aManager)
> +{
> +  MOZ_ASSERT(aStream);

This requires a non-null aStream but the PContentChild version doesn't?

@@ +455,5 @@
> +             value.optionalStream().type() != OptionalInputStreamParams::Tvoid_t);
> +
> +  // If the file descriptors are set, then we must have a serialized stream.
> +  MOZ_ASSERT_IF(value.optionalFds().type() != OptionalFileDescriptorSet::Tvoid_t,
> +                value.optionalStream().type() != OptionalInputStreamParams::Tvoid_t);

Is it intentional that these assertions are applied only when returing an optional value via TakeValue()?

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

I need to figure out a try failure as well:

  https://treeherder.mozilla.org/logviewer.html#?job_id=11236386&repo=try

It seems a NetworkError is being returned in some cases:

  JavaScript error: , line 0: TypeError: NetworkError when attempting to fetch resource.

This doesn't happen locally, so its probably a timing thing.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

So, it seems there are other problems with using nsPipe for http upload streams.  HTTP 1.x protocols require fixed length uploads.  This is incompatible with variable length streams like nsPipe.

Therefore, I need to change tack here.  I intend to land everything here except the necko bits.  This gets us the SendStream actor in ipc/glue so other people can use it.  Cache API will continue to be the only consumer for now.

I will then write a separate bug to fix the file blob uploads with service workers.  My plan there is to try a direct clone, but not fall back on the nsPipe.  If cloning does not directly work then we will delay firing the FetchEvent until an nsStorageStream can be fully populated.  This will then be set as the upload stream for the channel and cloned for the service worker.  This will add some latency to the fetch event when there is a file upload, but right now its unavoidable.

I will make the new bug block SW v1, and unhook this bug from the blockers.

Here's the IRC conversation with Patrick which led to this decision:

2:47 PM <bkelly> under what circumstances to we execute nsHttpTransaction::Restart()?
2:50 PM <bkelly> seems maybe only if pipelining was in effect
2:51 PM <jduell> mcmanus: ^^^
2:51 PM <jduell> nwgh: ^^^
2:51 PM <mcmanus> not just pipelines
2:51 PM <mcmanus> race condition between sending request and server closing pconn is the common case
2:52 PM <bkelly> hmm
2:52 PM  → Anupkumar joined  ⇐ sicking quit  
2:54 PM <bkelly> problem I'm trying to solve is when the we have an nsPipe for the request upload body... it doesn't really support rewinding
2:55 PM <bkelly> this is my current attempt to fix it, but I think I need to add something for the nsHttpTransaction::Restart case: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1093357&attachment=8658931
2:55 PM <bkelly> mcmanus: jduell: ^^^
2:59 PM ⇐ ferjm quit ([e-mail]) Quit: My Mac has gone to sleep. ZZZzzz…
3:01 PM <mcmanus> so, as I understand it, upload bodies are generally seekable
3:01 PM <mcmanus> coming from xhr, or forms or whatnot
3:01 PM <mcmanus> occasionally they are file backed and the seekable thing fails
3:01 PM <mcmanus> but that's not the main case
3:01 PM <bkelly> mcmanus: not since this landed:  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?case=true&from=ServiceWorkerManager.cpp#3736
3:02 PM <bkelly> mcmanus: now if a service worker is registered the upload gets cloned... which can result in an nsPipe if the original stream was file backed
3:03 PM <bkelly> mcmanus: we have to clone it because its possible for both the service worker and the network stack to be reading the upload at the same time... so they need duplicate streams
3:03 PM <bkelly> mcmanus: and once we implement streams... there will be potentially infinite streams from js script that can be passed for the upload
3:05 PM <bkelly> ignore that js stream case... I could try to us an nsStorageStream instead of the pipe for service workers... but the problem there is we then have to wait for the upload to fully load into memory before executing the service worker... which would suck for file backed upload streams
3:05 PM <mcmanus> does that break avaialble() too?
3:06 PM <bkelly> mcmanus: the pipe?  yea, it will probably have a bad Content-Length on the upload...
3:06 PM <mcmanus> cause that will screw up progress meters on the request side (which aren't used that much afaict)
3:06 PM <bkelly> mcmanus: well, nsPipeInputStream uses Tell() for the progress, AFAICT... which does seem to work
3:07 PM <bkelly> but I guess it needs the full Available() to know percent done
3:07 PM <mcmanus> I think the content-length header is set via the API that adds the request stream to the channel
3:08 PM <mcmanus> not via querying the stream
3:08 PM <mcmanus> which is good, cause h1 pretty much can't work without an accurate c-l on the request
3:08 PM <bkelly> mcmanus: HttpBaseChannel::SetExplicitUploadStream() tries to use Available() if -1 is passed for the length
3:09 PM <bkelly> but... i think it has the blob stream at that point... which does give a size
3:09 PM <bkelly> an estimated size
3:10 PM <bkelly> mcmanus: so h1 will never be able to support variable length uploads?
3:10 PM <mcmanus> long story - but yes
3:10 PM <mcmanus> cant terminate with chunked because chunked is 1.1 feature and when sending request you don't know if the server implements 1.1
3:11 PM <mcmanus> and beacuse of that, defacto many 1.1 servers didn't implement receipt of chunked anyhow
3:11 PM <bkelly> but we don't set chunked for uploads now, do we?
3:11 PM <mcmanus> right
3:11 PM <bkelly> I wonder if anyone actually supports that
3:11 PM <mcmanus> chunked is the alternative to c-l
3:11 PM <mcmanus> some do
3:11 PM <mcmanus> but there is no way to negotiate it
3:11 PM <mcmanus> so fail rates would be way to high
3:13 PM <mcmanus> so.. 
3:13 PM <mcmanus> seems like we need a seekable stream of some sort
3:13 PM <bkelly> mcmanus: sounds like chunked uploads will be hard... so I should probably abandon nsPipe for now
3:14 PM <bkelly> which means a lot of my work in bug 1093357 was a waste of time :-\
3:15 PM <bkelly> mcmanus: so I think it would be better for me to make ServiceWorkerManager fill an nsStorageStream
3:15 PM <mcmanus> don't know that one off the top of my head
3:15 PM <bkelly> mcmanus: nsStorageStream or the ServiceWorkerManager bit?
3:16 PM <mcmanus> the stream.. looking
3:16 PM <bkelly> mcmanus: its like a pipe, but never forgets old data
3:16 PM <bkelly> so its cloneable
3:17 PM <mcmanus> what to do about avaialble()?
3:17 PM <bkelly> and its also IPC serializable, but cuts it off wherever it is (which seems dubious to me, but whatever)
3:17 PM <mcmanus> or really c-l.. available() is a detail
3:17 PM <bkelly> mcmanus: I will make ServiceWorkerManager basically wait to fire the FetchEvent until the nsStorageStream is fully populated
3:17 PM <mcmanus> or do you just fill up the storage stream completely before starting the channel
3:18 PM <bkelly> yea, that
3:18 PM <mcmanus> :)
3:18 PM <bkelly> mcmanus: it just means creating latency for the file io :-\
3:18 PM <bkelly> but uploading file backed blobs is kind of a corner case
3:19 PM <mcmanus> yeah.. if you knew the c-l before it was loaded you could probably overload available() somehow
3:19 PM <bkelly> mcmanus: I need it fully loaded anyway to make the IPC serialization work
3:19 PM <bkelly> mcmanus: I wrote the IPC pipe actor in bug 1093357, but nsStorageStream pretends its done and can serialize immediately no matter what... so it wouldn't use my new thing
3:20 PM <mcmanus> sorry about this. h1 is really crappy about uploads
3:20 PM <mcmanus> something to think about is if you want an h2 path
3:20 PM <mcmanus> which could be streamable
3:21 PM <mcmanus> but might be premature optimization
3:21 PM <bkelly> mcmanus: we will need non-h1 streamable at some point I think... but its unclear to me how to expose this in the fetch API
3:22 PM <bkelly> mcmanus: do we have to go through h1 to get to h2?
3:22 PM <mcmanus> no
3:22 PM <mcmanus> bootstrapped as part of tls handshake
3:22 PM <bkelly> mcmanus: so maybe fetch api can require a chunked encoding header and fail fast if the server is h1... if the chunked encoding is not set then fetch buffers the upload before issuing the request
3:23 PM <bkelly> kind of terribly
3:23 PM <bkelly> terrible
3:24 PM <mcmanus> can't the implementation just buffer if its h1 and stream if its h2?
3:24 PM <mcmanus> is there a semantic exposed there?
3:24 PM <bkelly> mcmanus: if the script expects to stream something "infinitely", it would OOM
3:24 PM <mcmanus> infinite anything causes at least a timeout :)
3:25 PM <mcmanus> but I hear ya
3:25 PM <bkelly> mcmanus: hmm... so even in h2 the request is never really "complete" and gets a response status code without ending the upload?
3:25 PM <bkelly> I think my mental model was wrong (obviously)
3:26 PM <mcmanus> generally no.. the upload would complete
3:26 PM <mcmanus> it just isn't c-l based
3:26 PM <mcmanus> essentially everything is chunked in h2, though we don't call it that
3:26 PM <mcmanus> (frames and end of stream bits, etc..)
3:26 PM <bkelly> mcmanus: even in chunked, I can't get the 200 status code back until I stop streaming the upload, right?
3:26 PM <bkelly> making sure I understand
3:26 PM <mcmanus> generally not
3:26 PM <mcmanus> especially a 200
3:27 PM <mcmanus> cause declaring it ok, before its over isn't a good thing
3:27 PM <mcmanus> you might get an error back
3:27 PM <mcmanus> but generally responses only come back after requests are complete
3:27 PM <bkelly> I guess thats fine
3:27 PM <mcmanus> its a request-reply protocol afterall
3:27 PM <bkelly> mcmanus: whats the timeout on h2 if we are continuing to upload data continuously?
3:28 PM <mcmanus> as long as data is flowing it won't timeout
3:28 PM <bkelly> mcmanus: so it could upload infinitely without OOMing or timing out
3:28 PM <mcmanus> I believe so
3:28 PM <mcmanus> or its a bug
3:28 PM <bkelly> I have a benchmark server+page I wrote that would do that
3:28 PM <mcmanus> but I think I fixed that bug once :)
3:29 PM <mcmanus> same thing with an infinite response
3:29 PM <mcmanus> its ok as long as data is flowing
3:29 PM <bkelly> it does a GET from the server and streams the response... transforms the data... then POSTs back to the server in a separate request to stream the result
3:29 PM <bkelly> and it just stays like that continuously streaming data to measure response time
3:29 PM <bkelly> right now, of course, I have to use websockets instead of a POST
3:30 PM <bkelly> mcmanus: well, this has all been informative... but I think my short term message is ETOOCOMPLICATED... I'll try the storage stream
3:30 PM <bkelly> thanks for your help
3:31 PM <mcmanus> buena suerte
3:31 PM <bkelly> mcmanus: do you have a github account?  writing an issue on the fetch-with-streams repo
3:31 PM <mcmanus> https://github.com/mcmanus
3:31 PM <bkelly> thanks
3:31 PM nsm → nsm|away
3:35 PM <bkelly> jduell: the good news here is that you have escaped some reviews :-)
3:36 PM → sicking joined ([e-mail])
3:36 PM <bkelly> mcmanus: I think I am going to write a patch that MOZ_ASSERTs that the upload channel implements nsIIPCSerializableInputStream
3:36 PM <bkelly> at least
3:37 PM <mcmanus> I wonder if there are some callers that don't need that
3:37 PM <mcmanus> like ocsp post maybe
3:37 PM ⇐ ddamjano quit ([e-mail]) Ping timeout: 121 seconds
3:37 PM <bkelly> mcmanus: well, its the best way I know to tell if the stream is a pipe
3:37 PM <mcmanus> but I'm just wondering
3:37 PM <bkelly> and the pipe is broken for uploads because of Available, Seek, etc
3:38 PM <bkelly> we need to not allow it
3:38 PM <mcmanus> probly.. wondering about corner cases
3:38 PM <bkelly> mcmanus: do you know why necko permits streams that are not seekable as upload streams?  it seems that would break a lot of stuff like restarts, redirects, etc
3:38 PM <mcmanus> might as well assert and then enumerate them
3:39 PM <mcmanus> its in the category of its always been like that
3:39 PM <bkelly> awesome
3:39 PM <mcmanus> thus assert and enumerate I suppose
3:40 PM <mcmanus> plausibly there are callers that can simply tolerate greater error rates
3:40 PM <bkelly> mcmanus: I could assert nsIIPCSerializableInputStream only if in a child process
3:40 PM <mcmanus> or have their own retry (like I'm guessing ocsp)
3:40 PM <mcmanus> yeah - that's probably a better starting place

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8658924 [details] [diff] [review]
P5 Test file blob uploads with service worker interception. r=nsm

Addressed review comments and moved to bug 1203680.

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8658476 [details] [diff] [review]
P0.1 Implement an NS_InputStreamIsCloneable() method. r=froydnj

Moved to bug 1203680.

Comment 78

[(no longer active)]

FWIW this has caused us to disable some tests in dom/tests/mochitest/fetch.

Comment 79

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b25230c0a193

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Ignore comment 79.  It landed with the wrong bug number.

Comment 81

[Ben Kelly [:bkelly, not reviewing]]

I moved the P0 patch over to bug 1255070.

Comment 82

[Honza Bambas (:mayhemer)]

(Valentin, just FYI)

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Explicitly advertise that nsPipeInputStream supports ReadSegments(). r=froydnj

Rebased P0.  I will probably just land this here and then do bug 1255070 later.

Comment 84

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=jld

Rebased for a number of tree changes.  I still need to address a review item, though.

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add an RAII type to safely handle serialized stream actors. r=jld

Rebased.  I think this code also needs a test since I never used it in necko.

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Convert Cache to use SendStream instead of CachePushStream. r=ehsan

Rebase.  The dom/cache tests pass locally we these patches applied.

Comment 87

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 interdiff 001 address review feedback

This interdiff addresses the action items against P1 in comment 72.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 interdiff 001 address review feedback

This interdiff addresses the review feedback.  Its a big change because I ended up implementing the union type.  Unfortunately this came at the cost of another intermediate type and a slightly longer final type name.  Its now InputStreamParamsWithFdsOrSendStream.

Comment 89

[Ben Kelly [:bkelly, not reviewing]]

I think I am going to make dom/cache use the new AutoIPCStreamChild class instead of creating a separate test.  Its more work, but will reduce duplicated code and prove that the new RAII code works.

Comment 90

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add an RAII type to safely handle serialized stream actors. r=jld

Jed, this is a rewrite of the P2 patch you previously reviewed here.  Sorry its been so long since you did that review!

I'm asking for re-review because I've changed it a bit.  I've shortened the top level type name from:

  InputStreamParamsWithFdsOrSendStream

To just:

  IPCStream

I've also made the serialization and deserialization methods work in both the child and parent.  The only thing thats not supported is serializing to a PSendStream on the parent side at the moment.  If I ever add parent-to-child pipe serialization, though, I would add it into this type.

I made these changes because I found they helped me integrate the type more easily into dom/cache.  I think they are more likely to be useful to other consumers this way as well.

Please let me know if you don't think you will have time for this review.  Thanks!

Comment 91

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Convert Cache to use IPCStream and AutoIPCStream. r=asuth

This is a rewrite of the P3 patch to not only use PSendStream, but also to use the AutoIPCStream RAII type.  I worked through this in order to ensure the RAII type had adequate test coverage.  I found a number of issues and ultimately rewrote a lot of the RAII code resulting in the new P2 patch.

Ehsan agreed to that Andrew could do this review.

This is a rather complex use of AutoIPCStream.  The dom/cache code embeds its streams in CacheReadStream structures.  Depending on the type of the CacheOp there may be one stream, many streams, or no streams.

The dom/cache code previously had its own AutoUtils.cpp code to manage cleaning these streams up.  A lot of this code is replaced by using AutoIPCStream objects, however.

The main tricky thing here is that AutoUtils.cpp objects now hold a list of AutoIPCStream objects.  These AutoIPCStream objects are attached to the embedded streams in the CacheReadStream structures.

This is a bit risky because if the array of CacheReadStream structs gets expanded and realloc'd then the address of all the streams changes.  This then breaks the AutoIPCStream attachment and causes UAFs.

I've solved this particular problem by pre-allocating the capacity of the various arrays.  I then have strong MOZ_RELEASE_ASSERTs to ensure that a realloc does not occur.  I think this addresses the risk adequately, but I wanted to call it out for the review.

Comment 92

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8748846 [details] [diff] [review]
P2 Add an RAII type to safely handle serialized stream actors. r=jld

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

(Sorry for the delay; I was out sick the past couple days.)

Looks good, with one comment/question:

::: ipc/glue/IPCStreamUtils.cpp
@@ +96,5 @@
> +  } else {
> +    PFileDescriptorSetParent* fdSet =
> +      aManager->SendPFileDescriptorSetConstructor(fds[0]);
> +    for (uint32_t i = 1; i < fds.Length(); ++i) {
> +      Unused << fdSet->SendAddFileDescriptor(fds[i]);

Can these Send* methods fail?

@@ +233,5 @@
> +    return sendStream->TakeReader();
> +  }
> +
> +  // Note, we explicitly do not support deserializing the PSendStream actor on
> +  // the child side.  It can only be send from child to parent.

Typo nit: "sent".

Comment 93

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #92) 
> (Sorry for the delay; I was out sick the past couple days.)

No problem, thanks for the looking at it again after I took so long on this bug!

> ::: ipc/glue/IPCStreamUtils.cpp
> @@ +96,5 @@
> > +  } else {
> > +    PFileDescriptorSetParent* fdSet =
> > +      aManager->SendPFileDescriptorSetConstructor(fds[0]);
> > +    for (uint32_t i = 1; i < fds.Length(); ++i) {
> > +      Unused << fdSet->SendAddFileDescriptor(fds[i]);
> 
> Can these Send* methods fail?

I believe the only way this can fail is if we race with the child process closing.  In that case we might get the PFileDescriptorSetParent actor, but then fail to send the remaining file descriptors.

In theory the ultimate sending of the IPCStream object would subsequently fail as well.

I could set void_t() for the OptionalFileDescriptorSet in this case, but not much more.  This is what Blobs currently do:

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#344

I *think*, though, that the FileDescriptor class destructor will correctly close the file descriptor held by this c++ code.

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.h#81

The file descriptor duped to the ipdl message is probably leaked, though.

What do you think?

Comment 94

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

Comment on attachment 8748853 [details] [diff] [review]
P3 Convert Cache to use IPCStream and AutoIPCStream. r=asuth

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

r=asuth via deferral from ehsan.  Everything is nits and I'm fine with the minor un-bit-rotting that will be needed from the crasher fixes you've landed or other similar minor drift fixes.

Meta-wise, I really like the cleanup of this entire stack of patches (and related precursors).  Kudos!

::: dom/cache/AutoUtils.cpp
@@ +467,5 @@
>        StorageMatchResult& result = mOpResult.get_StorageMatchResult();
> +      // Ensure that we don't realloc the array since this can result
> +      // in our AutoIPCStream objects to reference the wrong memory
> +      // location.  This should never happen and is a UAF if it does.
> +      // Therefore make this a release assertion.

I don't think you meant to add this comment here since it doesn't apply?  (StorageMatchResult and CacheMatchResult have the same signature and otherwise the equivalent logic, so if you did mean to do something here, do it there too, probably.)

::: dom/cache/CacheOpChild.cpp
@@ +201,2 @@
>  {
> +  MOZ_CRASH("CacheOpChild should never create a send stream actor!");

nit: The change to this assertion string seems insufficient given the method name change, the lack of a comment on TypeUtils::GetIPCManager, and the likelihood future code might be interested in the PBackgroundChild*.

Since crash strings can't be essays, I'd suggest adding a block comment to TypeUtils::GetIPCManager that provides some context like: "Get the managing PBackgroundChild with the assumed purpose of creating a PSendStream.  If you want the PBackgroundChild for other reasons, you may need to modify existing implementations which may assert if they weren't expecting this to happen."

::: dom/cache/CacheStorage.cpp
@@ +584,3 @@
>  {
>    // This is true because CacheStorage always uses IgnoreBody for requests.
> +  MOZ_CRASH("CacheStorage should never create a send stream.");

Same deal with the crash message as CacheOpChild.

::: dom/cache/TypeUtils.cpp
@@ +490,5 @@
>  
>    // Option 1: Send a cache-specific ReadStream if we can.
>    RefPtr<ReadStream> controlled = do_QueryObject(aStream);
>    if (controlled) {
> +    printf_stderr("### ### create a controlled stream\n");

Debug logging that should probably be removed or converted to a MOZ_LOG.

While here, maybe remove the "Option 1:" bit from the comment above that is confusing with the option 2/3 comments removed as they were absorbed by the AutoIPCStream usage.

Comment 95

[Ben Kelly [:bkelly, not reviewing]]

I ended up setting the optionalFds() to void_t() if the SendAddFileDescriptor() call from parent-to-child returns false.  I also Send__delete__() the fdSet in case the failed return value was due to memory and the child process is still there.

I don't do this in the child-to-parent case.  I believe IPC will intentionally crash the child process before it returns false from the child actor async method.  None of the other uses of SendAddFileDescriptor() from child-to-parent check the return value as well.

Comment 96

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=jld

Fold addressed review items into a single P1 patch.

Comment 97

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add an RAII type to safely handle serialized stream actors. r=jld

Address review feedback.

Comment 98

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Convert Cache to use IPCStream and AutoIPCStream. r=asuth

Rebase for ReadStream::Serialize() taking an ErrorResult now and also address review feedback.

Comment 99

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 100

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=jld

Fix some bustage revealed by try.

Comment 101

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Convert Cache to use IPCStream and AutoIPCStream. r=asuth

Fix more try bustage.  Its annoying that std::max() is so strict on types when our nsTArray::Length() seems to change types depending on platform.

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

Comment 102

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement an actor for streaming pipes from child to parent. r=jld

Fix winxp bustage because khuey changes nsICancelableRunnable::Cancel() from NS_IMETHOD to just nsresult return value.

I can't push another try build until the trees reopen.

Comment 103

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 104

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9ee46f3b93
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1df4a2ab7c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f92c3038f215
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f7004b976a

Comment 105

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c9ee46f3b93
https://hg.mozilla.org/mozilla-central/rev/b1df4a2ab7c2
https://hg.mozilla.org/mozilla-central/rev/f92c3038f215
https://hg.mozilla.org/mozilla-central/rev/47f7004b976a

Comment 106

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

What's with the commented-out NS_IMPL_QUERY_INTERFACE in P0?

Comment 107

[Ben Kelly [:bkelly, not reviewing]]

Oh, good catch!  I forgot that I put the updated patch on bug 1255070 instead of here.  I will land a follow-on patch in this bug ASAP to fix.

Comment 108

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31c7784e29b

Comment 109

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Follow-on to nsPipe3.cpp to address review feedback missing in P0 patch. r=me

Follow-on patch that address review feedback for P0 patch.  Sorry I landed the wrong patch in my initial commit!

Comment 110

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e31c7784e29b

Comment 111

[Chris Peterson [:cpeterson]]

To clean up the e10s bug queries, I am noting that we don't want to uplift this fix from Aurora 49 to Beta 48.