1366316 - Separate thread for IPCBlobInputStream

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement)

Description

[Andrea Marchesini [:baku]]

I and smaug discussed this issue on IRC. The reason why we need to have a separate thread for IPCBlob is that, any IPCBlobInputStream has an owning thread. But if this is a worker, and the inputStream is used elsewhere, we don't want to keep alive the worker.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - PIPCBlobCloning

This protocol is used for the cloning of a IPCBlobInputStream. Instead making the CTOR for client and parent, it's better and cleaner to have a separate protocol just for this.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - reverting the WorkerHolder usage

We don't need a workerHolder anymore.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlobInputStreamThread and replacement of the IPCBlobInputStream actor

The workflow is this:

1. a IPCBlobInputStream can be created in any thread. When this is done, we will have a IPCBlobInputStreamChild owned by that thread.

2. if the thread is not a IPCBlobInputStreamThread, IPCBlobInputStreamChild registers itself into IPCBlobInputStreamThread, in order to be cloned into the right thread.

3. When the cloning is completed on the IPCBlobInputStreamThread, any IPCBlobInputStream objects, managed by the original IPCBlobInputStreamChild will be moved to the new actor.

4. If there were pending StreamNeeded() operations, they will be processed.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - WorkerHolder for the cloning

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlobInputStreamThread and replacement of the IPCBlobInputStream actor

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 8

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

Comment on attachment 8869489 [details] [diff] [review]
part 4 - WorkerHolder for the cloning

This needs comments.

Comment 9

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

Comment on attachment 8869484 [details] [diff] [review]
part 1 - PIPCBlobCloning

I have no idea what this is about. Why we need to clone anything?

Comment 10

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

Comment on attachment 8869758 [details] [diff] [review]
part 3 - IPCBlobInputStreamThread and replacement of the IPCBlobInputStream actor

I don't understand this either. What is this ReplaceActor stuff?

Comment 11

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

In other words, please explain why we do any cloning and actor replacement. Why can't we just start with the right actor?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 0 - comments

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - migration

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - callback storing

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - callback storing

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 4 - monitoring child processes

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 5 - workerHolder

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 1 - migration

Comment 20

[Andrea Marchesini [:baku]]

smaug, I would like you to review these patches.

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8870746 [details] [diff] [review]
part 1 - migration

Kanru, can you review the IPC part of this patch? Thanks.

Comment 22

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

Comment on attachment 8870711 [details] [diff] [review]
part 0 - comments

It might be good to clarify what happens when migration is tried to be done while reading from a blob.

Comment 23

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

Comment on attachment 8870746 [details] [diff] [review]
part 1 - migration

> IPCBlobInputStreamChild::CreateStream()
> {
>-  MutexAutoLock lock(mMutex);
>+  bool toBeDeleted = false;
Could we use some other name for this variable, since it doesn't mean this particular object should be deleted.
Perhaps bool shouldMigrate = false; ?

> public:
>+  enum ActorState {
Nit, { goes to its own line.

>+    // The actor is connected via IPDL to the parent.
>+    eActive,
>+
>+    // The actor is disconnected.
>+    eInactive,
>+
>+    // The actor is waiting to be disconnected. When it will be disconnected, it
>+    // will be reactivated on the DOM-File thread.
Perhaps:
Once it has been disconnected, it will be reactivated on the DOM-File thread.

>+  ActorCreated(mozilla::ipc::PBackgroundChild* aActor) override
>+  {
>+    MOZ_ASSERT(mActor->State() == IPCBlobInputStreamChild::eInactiveMigrating);
>+
>+    if (aActor->SendPIPCBlobInputStreamConstructor(mActor, mActor->ID(),
>+                                                   mActor->Size())) {
>+      // We need manually to increase the reference for this actor because the
>+      // IPC allocator method is not triggered.
>+      mActor->AddRef();
Who will call Release? Please add a comment





>+  // __delete__ can be called by parent and by child for 2 reasons:
>+  // - parent->child: This happens after a Close(). The child wants to inform
>+  //                  the parent that no other messages will be dispatched and
>+  //                  that the channel can be interrupted.
This looks backwards. Parent sending a message to child that the child wants to inform parent?



Could we perhaps ask QA folks to do some manual testing - writing evil testcases or so? This is unusual and complicated stuff.

Comment 24

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

Comment on attachment 8870718 [details] [diff] [review]
part 2 - callback storing

> IPCBlobInputStreamStorage::GetStream(const nsID& aID,
>                                      nsIInputStream** aInputStream)
> {
>   mozilla::StaticMutexAutoLock lock(gMutex);
>-  nsCOMPtr<nsIInputStream> stream = mStorage.Get(aID);
>-  if (!stream) {
>+  Data* data =mStorage.Get(aID);
Missing space after =

>+  already_AddRefed<IPCBlobInputStreamParentCallback>
>+  StealCallback(const nsID& aID);
Perhaps TakeCallback ? Take is used more commonly than Steal


>-  nsInterfaceHashtable<nsIDHashKey, nsIInputStream> mStorage;
>+  struct Data
Could you call this something more descriptive. Even though this is inside another class, 
Data is super generic.

Comment 25

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

Comment on attachment 8870719 [details] [diff] [review]
part 4 - monitoring child processes

> 
> template<typename M>
> /* static */ IPCBlobInputStreamParent*
> IPCBlobInputStreamParent::Create(nsIInputStream* aInputStream, uint64_t aSize,
>-                                 nsresult* aRv, M* aManager)
>+                                 uint64_t aChildID, nsresult* aRv, M* aManager)
What is aChildID? It is content child ID, so at least add some comment what it is supposed to be.

>+  MOZ_ASSERT(!strcmp(aTopic, "ipc:content-shutdown"));
Do we get ipc:content-shutdown also when child process crashes?
Have you ensured this all work as expected when a child process crashes?


> private:
>   IPCBlobInputStreamStorage();
>   ~IPCBlobInputStreamStorage();
> 
>   struct Data
>   {
>     nsCOMPtr<nsIInputStream> mInputStream;
>     RefPtr<IPCBlobInputStreamParentCallback> mCallback;
>+    uint64_t mChildID;
Please document what this member variable is.

Comment 26

[Andrea Marchesini [:baku]]

> >+  MOZ_ASSERT(!strcmp(aTopic, "ipc:content-shutdown"));
> Do we get ipc:content-shutdown also when child process crashes?
> Have you ensured this all work as expected when a child process crashes?

Yes, I checked. But I don't know if it testable with a mochitest.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2698925268
Separate thread for IPCBlobInputStream actors - part 0 - comments, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c54a18bf95
Separate thread for IPCBlobInputStream actors - part 1 - actor migration, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/432077ba9b7f
Separate thread for IPCBlobInputStream actors - part 2 - callback stored, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b48496a0f3
Separate thread for IPCBlobInputStream actors - part 3 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3965a5c72ea3
Separate thread for IPCBlobInputStream actors - part 4 - monitoring child processes, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd22c45d4ada
Separate thread for IPCBlobInputStream actors - part 5 - WorkerHolder, r=smaug

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c2698925268
https://hg.mozilla.org/mozilla-central/rev/59c54a18bf95
https://hg.mozilla.org/mozilla-central/rev/432077ba9b7f
https://hg.mozilla.org/mozilla-central/rev/c1b48496a0f3
https://hg.mozilla.org/mozilla-central/rev/3965a5c72ea3
https://hg.mozilla.org/mozilla-central/rev/fd22c45d4ada