1274343 - Add parent-to-child pipe streaming to IPCStream

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Bug 1093357 added the IPCStream type which includes the ability to stream a pipe across IPC.  It only added child-to-parent support, though.  This bug is to expand that to parent-to-child support.

I think the main issue making this complicated is that IPCStream supports PBackground which can be targeting a child Worker thread.  Since these threads can die, its not safe to just send actor messages without some kind of WorkerFeature holding the thread alive.

Currently we just ignore this for PSendFileDescriptor and just assume the code using the actor has done the right thing.  We could do the same for PSendStream.

I think we might want to go one step beyond, though.  I'd like to assert that any time a PBackground manager sends a message to a worker thread that there is at least one WorkerFeature registered to hold the thread alive.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Kyle, what do you think of the idea in the last paragraph of comment 0?  Would you be ok with that?

Comment 2

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

That assertion won't pass today.  One example: if you have an IndexedDB database open, but no ongoing transactions or requests, there will be no outstanding features.  But if someone else tries to open the database at a higher version we will get a RecvVersionChange over a PBackground actor.  Adding a feature for that would also mean that as long as there's a database you haven't closed the worker can't be GCd, which is also a non-starter.

I think doing something smart here would be a good thing, but I'm not sure what it would be.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> That assertion won't pass today.  One example: if you have an IndexedDB
> database open, but no ongoing transactions or requests, there will be no
> outstanding features.  But if someone else tries to open the database at a
> higher version we will get a RecvVersionChange over a PBackground actor. 
> Adding a feature for that would also mean that as long as there's a database
> you haven't closed the worker can't be GCd, which is also a non-starter.

How can that possibly work without creating a crashing race?  If you get the RecvVersionChange while the worker is shutting down...

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Another option here would be to build out parent-to-child PSendStream support, but only allow PContent and main-thread-targeted PBackground.  If you tried to send the pipe to a worker thread PBackground it would MOZ_CRASH.

Comment 5

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

(In reply to Ben Kelly [:bkelly] from comment #3)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> > That assertion won't pass today.  One example: if you have an IndexedDB
> > database open, but no ongoing transactions or requests, there will be no
> > outstanding features.  But if someone else tries to open the database at a
> > higher version we will get a RecvVersionChange over a PBackground actor. 
> > Adding a feature for that would also mean that as long as there's a database
> > you haven't closed the worker can't be GCd, which is also a non-starter.
> 
> How can that possibly work without creating a crashing race?  If you get the
> RecvVersionChange while the worker is shutting down...

Because during shut down we close the actor and then we don't get any more messages.

Jonas thinks we might actually want it to keep the worker alive though ....

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Because during shut down we close the actor and then we don't get any more
> messages.

Is there a mechanism to learn about worker shutdown besides WorkerFeature::Notify()?

Comment 7

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

Garbage collection.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> Garbage collection.

So you call PBackgroundUIDBDatabase::Close() to trigger the Send__delete__() from the parent process.

Before the actor is destroyed in the parent:

1) The worker thread can die
2) The parent can send a VersionChange message to the child

If this happens you will crash.

Ben Turner described this to me in IRC here:

http://logs.glob.uno/?c=mozilla%23content&s=11+Feb+2015&e=11+Feb+2015#c265454

He even describes how IDB holds a WorkerFeature to avoid the race for IDB requests for this reason.

And you can't immediately just call Send__delete__() on GC because that too can race with a message coming from parent-to-child and cause a crash.

Anyway, happy to be wrong about all this, but this is my understanding of the constraints.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Sorry, meant on GC you call PBackgroundIDBDatabase::SendClose():

https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBDatabase.cpp#289

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, it would make sense to me to require that any time we target a runnable at a worker thread, that that thread has active features. At the very least that should be the case if the runnable can trigger web-visible callbacks, such as DOM Events.

I'm not sure if other gecko-internal stuff, like measuring memory usage for about:memory, uses runnables. But if they do, such runnables might happen even if there aren't any active features.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I don't think we can easily assert for all runnable for all sourcez.  Or rather that would be more work than I'm willing to do here.

I think asserting for IPC messages would be a good start and relatively achievable.  It would also make me comfortable auto-creating a parent-to-child actor with the caveat "make sure you hold workers alive".

Comment 12

[Andrea Marchesini [:baku]]

This bug is really really interesting. When we have this, we can have a huge simplification of our Blob IPC code.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming PSendStream to PSendStreamCtoP

In theory, we could make PSendStream able to work on both direction but that would make the protocol unclear in "what is call where". We had a similar setup for PBlob and, yes, I don't like it.

This first patch is just about renaming PSendStream to PSendStreamCtoP (child to parent).

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - PSendStreamPtoC

This second patch introduces the PSendStreamPtoC (Parent to Child).
In order to reuse the same code, this patch does:

1. SendStreamSource is a class for anything that owns the nsIInputStream. Parent in PtoC, Child in CtoP.

2. SendStreamDestination is, of course, the other side.

3. The actors:
- SendStreamSourceChild implements SendStreamSource and PSendStreamCtoPChild
- SendStreamSourceParent implements SendStreamSource and PSendStreamPtoCParent
- SendStreamDestinationChild implements SendStreamDestination and PSendStreamCtoPParent
- SendStreamDestinationParent implements SendStreamDestination and PSendStreamPtoCChild

Note that PSendStream{CtoP,PtoC} are not really exposed. Everything happens via IPCStream. This IPDL union covers both directions. The same if for AutoIPCStream.

Comment 15

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

(In reply to Andrea Marchesini [:baku] from comment #13)
> Created attachment 8841940 [details] [diff] [review]
> part 1 - Renaming PSendStream to PSendStreamCtoP
> 
> In theory, we could make PSendStream able to work on both direction but that
> would make the protocol unclear in "what is call where". We had a similar
> setup for PBlob and, yes, I don't like it.
> 
> This first patch is just about renaming PSendStream to PSendStreamCtoP
> (child to parent).


I don't like the name. There is really no good reason for CtoP shorthand.
Shorthands are nice for writing but bad for readability, especially when someone not familiar with the code reads it.
PSendStreamToParent or PSendStreamFromChildToParent. I think I like PSendStreamToParent.

(Though, it is a bit unclear to me why the name has "Send" when we aren't really sending any stream, but streaming data. Sure, the protocol is used for cross-process inputstream handling, but right now its name tells about its usage, not what it does. But perhaps better to not change that part of the name now.)

Comment 16

[Andrea Marchesini [:baku]]

About 'send' I just follow the existing code. I'm OK with renaming it.
And I agree with CtoP to ToParent to FromChildToParent.

Comment 17

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

Comment on attachment 8841940 [details] [diff] [review]
part 1 - Renaming PSendStream to PSendStreamCtoP

So, use either PSendStreamToParent or PSendStreamFromChildToParent or PChildToParentStream.  (maybe the last one is nicest, but up to you)

With that, r+.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

This "send" part of the name was because I had originally thought reversing the direction would require pulling data from child-to-parent.  So I was thinking there might be a "recv" stream or something.

Of course, when this code lived in dom/cache it was called PCachePushStream.  Not sure why I switched from "push" to "send".

I don't mind if you drop the Send part.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8841948 [details] [diff] [review]
part 2 - PSendStreamPtoC

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

I didn't review the whole patch, but I think these two issues should be fixed.

Thanks for taking this bug on!

::: ipc/glue/IPCStreamUtils.h
@@ +133,5 @@
>  public:
> +  enum Direction
> +  {
> +    eChildToParent,
> +    eParentToChild,

Do you really need this?  It seems like you could infer this from the manager argument.  If its a *Parent manager type then it must be parent-to-child.  Right?

::: ipc/glue/SendStreamParent.cpp
@@ +88,5 @@
> +    return nullptr;
> +  }
> +
> +  if (!aManager->SendPSendStreamPtoCConstructor(source)) {
> +    delete source;

I think this is wrong.  I believe the Send*Constructor() call will execute the normal actor destruction sequence if the constructor call fails.  So ActorDestroy() is called and the actor deallocation occurs.  So you should not call delete manually here.

Also, you should make sure ActorDestroy() does something sane if its called before ::Create() exits.

Comment 20

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

Comment on attachment 8841948 [details] [diff] [review]
part 2 - PSendStreamPtoC

This patch will be updated, I'm told.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming PSendStream to PChildToParentStream

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment 24

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

Comment on attachment 8842987 [details] [diff] [review]
part 2 - PParentToChildStream

Wrong patch. This is for the one changing principal handling

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment 27

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

I think there should be some explanation about the setup, in the code.
How IPCStreamChild/Parent related to IPCStreamSource/Destination? Why do we need them all?

Looks like there isn't such things as IPCStreamChild/Parent, but they are only file names.


So IPCStreamSource/Destination are abstract classes, and then actual child or parent implementations are in IPCStreamChild/Parent...


Trying to understand the setup here...

Comment 28

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

Comment on attachment 8845291 [details] [diff] [review]
part 2 - PParentToChildStream


>+// Returns false if the serialization should not proceed. This means that the
>+// inputStream is null.
>+bool
>+NormalizeOptionalValue(nsIInputStream* aStream, 
>+                       IPCStream* aValue,
>+                       OptionalIPCStream* aOptionalValue)
Ah, so this patch isn't up to date. (I think this relevant code was changed in a simpler way in another bug)
Could you rebase and ask review again. 

+// The IPCStream IPC actor is designed to push an nsIInputStream from child to
+// parent or parent to child incrementally.  This is mainly needed for streams
+// such as nsPipe that/ may not yet have all their data available when the
+// stream must be sent across/ an IPC boundary.  While many streams are handled
What are the / characters there in middle of sentences?


+  void OnStreamReady(Callback* aCallback);
+
+  nsCOMPtr<nsIAsyncInputStream> mStream;
+  RefPtr<Callback> mCallback;
+
+  dom::workers::WorkerPrivate* mWorkerPrivate;
In some way prove that using raw pointer here is safe and add a comment about it.


+    bool result = HoldWorker(workerPrivate, Canceling);
This needs a comment explaining when the worker isn't kept alive anymore.


This is so massive patch that it needs couple of read-throughs.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment 30

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming PSendStream to PChildToParentStream

Comment 31

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

Comment on attachment 8846525 [details] [diff] [review]
part 2 - PParentToChildStream

> CacheStreamControlParent::SerializeStream(CacheReadStream* aReadStreamOut,
>                                           nsIInputStream* aStream,
>                                           nsTArray<UniquePtr<AutoIPCStream>>& aStreamCleanupList)
> {
>   NS_ASSERT_OWNINGTHREAD(CacheStreamControlParent);
>   MOZ_DIAGNOSTIC_ASSERT(aReadStreamOut);
>   MOZ_DIAGNOSTIC_ASSERT(aStream);
>   UniquePtr<AutoIPCStream> autoStream(new AutoIPCStream(aReadStreamOut->stream()));
>-  autoStream->Serialize(aStream, Manager());
>+  Unused << autoStream->Serialize(aStream, Manager());
This is weird. You add MOZ_MUST_USE, but then just ignore the result.
Sounds like we should at least MOZ_DIAGNOSTIC_ASSERT if nothing else can be done
Same also elsewhere.


>+++ b/ipc/glue/IPCStreamSource.cpp
...
>+  IPCStreamSource* mSource;
This needs a comment explaining why raw pointer is safe to use here.
I know, mostly old code, but still.


>+IPCStreamSource::OnEnd(nsresult aRv)
> {
>-  NS_ASSERT_OWNINGTHREAD(SendStreamChild);
>+  NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
>   MOZ_ASSERT(aRv != NS_BASE_STREAM_WOULD_BLOCK);
> 
>-  if (mClosed) {
>+  if (mState == eClosed) {
>     return;
>   }
> 
>-  mClosed = true;
>+  mState = eClosed;
Nit, Tiny bit surprising to set state eClosed in OnEnd, yet then call method 
Close(aRv); At least assert in Close() implementations that mState == eClosed




>+
>+    nsCOMPtr<nsIEventTarget> target =
>+        do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
2 spaces for indentation





>+SerializeInputStreamParent(nsIInputStream* aStream, M* aManager,
>+                           IPCStream* aValue,
>+                           OptionalIPCStream* aOptionalValue)
> {
>-  if (!aStream) {
>-    aValue = void_t();
>-    return;
>+  MOZ_ASSERT(aStream);
>+  MOZ_ASSERT(aManager);
>+  MOZ_ASSERT(aValue || aOptionalValue);
>+
>+  // If a stream is known to be larger than 1MB, prefer sending it in chunks.
>+  const uint64_t kTooLargeStream = 1024 * 1024;
>+
>+  nsCOMPtr<nsIIPCSerializableInputStream> serializable =
>+    do_QueryInterface(aStream);
>+  uint64_t expectedLength =
>+    serializable ? serializable->ExpectedSerializedLength().valueOr(0) : 0;
Could you add some comment about ExpectedSerializedLength() behavior here. It is very well hidden atm that
FileStreams don't return file size here. Add the comment also to the other place where this method is used.
And please file a followup bug to rename ExpectedSerializedLength. I don't yet have too good suggestions for the name though.


>+++ b/ipc/glue/MessageLink.cpp
>@@ -13,16 +13,17 @@
> 
> #include "mozilla/Assertions.h"
> #include "mozilla/DebugOnly.h"
> #include "nsDebug.h"
> #ifdef MOZ_CRASHREPORTER
> #include "nsExceptionHandler.h"
> #endif
> #include "nsISupportsImpl.h"
>+#include "nsPrintfCString.h"
Why is this? Perhaps some leftover from debugging?


Again, rather small changes, but could you update the patch and ask review again. Need to read it still.

Comment 32

[Andrea Marchesini [:baku]]

> >-  mClosed = true;
> >+  mState = eClosed;
> Nit, Tiny bit surprising to set state eClosed in OnEnd, yet then call method 
> Close(aRv); At least assert in Close() implementations that mState == eClosed

I cannot do that, because Close() is implemented by the child/parent class.
I could rename Close() to CloseInternal() and add the assertion in Close().

> >+#include "nsPrintfCString.h"
> Why is this? Perhaps some leftover from debugging?

This is needed because, adding some .cpp file, the header block got broken.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 2 - PParentToChildStream

Comment applied

Comment 34

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

Comment on attachment 8846691 [details] [diff] [review]
part 2 - PParentToChildStream

> CacheStreamControlParent::SerializeStream(CacheReadStream* aReadStreamOut,
>                                           nsIInputStream* aStream,
>                                           nsTArray<UniquePtr<AutoIPCStream>>& aStreamCleanupList)
> {
>   NS_ASSERT_OWNINGTHREAD(CacheStreamControlParent);
>   MOZ_DIAGNOSTIC_ASSERT(aReadStreamOut);
>   MOZ_DIAGNOSTIC_ASSERT(aStream);
>+
>   UniquePtr<AutoIPCStream> autoStream(new AutoIPCStream(aReadStreamOut->stream()));
>-  autoStream->Serialize(aStream, Manager());
>+  bool ok = autoStream->Serialize(aStream, Manager());
>+  MOZ_DIAGNOSTIC_ASSERT(ok);
I wonder if this will warn about unused variable when compiling opt beta or release build.


>   if (body) {
>     aAutoStream.reset(new mozilla::ipc::AutoIPCStream(aIPCResponse->body()));
>-    aAutoStream->Serialize(body, aManager);
>+    bool ok = aAutoStream->Serialize(body, aManager);
>+    MOZ_DIAGNOSTIC_ASSERT(ok);
same here.


>+  // This is a raw pointer because the source keeps alive the callback and,
>+  // before beeing destroyed, it nullify this pointer.
>+  IPCStreamSource* mSource;
nullifies
Want to clarify where/how it nullifies?



>+  // A source can be used on any thread, but we only support IPCStream on
>+  // main thread and Worker threads right now.  This is due to the requirement
>+  // that the thread be guaranteed to live long enough to receive messages.
>+  // We can enforce this guarantee with a feature on worker threads, but not
>+  // other threads.
>+  WorkerPrivate* workerPrivate = nullptr;
>+  if (!NS_IsMainThread()) {
>+    workerPrivate = GetCurrentThreadWorkerPrivate();
>+    MOZ_ASSERT(workerPrivate);
Can you make this release assert. Though, I guess we crash anyhow if there isn't workerPrivate...
But release assert would at least make it clear.

>+IPCStreamSource::OnEnd(nsresult aRv)
> {
>-  NS_ASSERT_OWNINGTHREAD(SendStreamChild);
>+  NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
>   MOZ_ASSERT(aRv != NS_BASE_STREAM_WOULD_BLOCK);
> 
>-  if (mClosed) {
>+  if (mState == eClosed) {
>     return;
>   }
> 
>-  mClosed = true;
>+  mState = eClosed;
> 
>   mStream->CloseWithStatus(aRv);
> 
>   if (aRv == NS_BASE_STREAM_CLOSED) {
>     aRv = NS_OK;
>   }
...
>+  // This will trigger an ActorDestroy() from the other side
>+  Close(aRv);
So, please, assert somehow in Close that the state is right.
Why you can assert it in both Close() implementations?

> //
> //  1) The data flow must be explicitly initiated by calling
>-//     SendStreamChild::Start() after the actor has been sent to the parent.
>-//  2) If the actor is never sent to the parent, then the child code must
>-//     call SendStreamChild::StartDestroy() to avoid memory leaks.
>-//  3) The SendStreamChild actor can only be used on threads that can be
>+//     IPCStreamChild::Start() after the actor has been sent to the
>+//     other-side actor.
IPCStreamChild? What about when sending from parent to child?


>+//  2) If the actor is never sent to the other-side, then this code must
>+//     call IPCStreamChild::StartDestroy() to avoid memory leaks.
ditto



>+bool
>+SerializeInputStreamWithFdsChild(nsIIPCSerializableInputStream* aStream,
>                                  IPCStream& aValue,
>                                  M* aManager)
> {
>   MOZ_ASSERT(aStream);
Perhaps make this MOZ_RELEASE_ASSERT, given that the old code uses MOZ_CRASH


This is scary stuff, and given the type of the patch it is hard to review. Hopefully I didn't miss anything crucial.
Make sure to land to try (debug builds too).

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a43ac75dfca
Add parent-to-child pipe streaming to IPCStream - part 1 - renaming PSendStream to PChildToParentStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce5031e6ac4
Add parent-to-child pipe streaming to IPCStream - part 2 - PParentToChild, r=smuag

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/9a43ac75dfca
https://hg.mozilla.org/mozilla-central/rev/cce5031e6ac4