Closed Bug 1366316 Opened 3 years ago Closed 3 years ago

Separate thread for IPCBlobInputStream

Categories

(Core :: DOM: File, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(6 files, 9 obsolete files)

3.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.79 KB, patch
smaug
: review+
kanru
: review+
Details | Diff | Splinter Review
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.
Attached patch part 1 - PIPCBlobCloning (obsolete) — Splinter Review
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.
Assignee: nobody → amarchesini
Attachment #8869484 - Flags: review?(bugs)
We don't need a workerHolder anymore.
Attachment #8869485 - Flags: review?(bugs)
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.
Attachment #8869488 - Flags: review?(bugs)
Attachment #8869489 - Flags: review?(bugs)
Attached patch part 5 - tests (obsolete) — Splinter Review
Attachment #8869490 - Flags: review?(bugs)
Attachment #8869488 - Attachment is obsolete: true
Attachment #8869488 - Flags: review?(bugs)
Attachment #8869758 - Flags: review?(bugs)
Attached patch part 5 - tests (obsolete) — Splinter Review
Attachment #8869490 - Attachment is obsolete: true
Attachment #8869490 - Flags: review?(bugs)
Attachment #8869762 - Flags: review?(bugs)
Attachment #8869762 - Flags: review?(bugs) → review+
Attachment #8869485 - Flags: review?(bugs) → review+
Comment on attachment 8869489 [details] [diff] [review]
part 4 - WorkerHolder for the cloning

This needs comments.
Attachment #8869489 - Flags: review?(bugs) → review-
Comment on attachment 8869484 [details] [diff] [review]
part 1 - PIPCBlobCloning

I have no idea what this is about. Why we need to clone anything?
Attachment #8869484 - Flags: review?(bugs)
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?
Attachment #8869758 - Flags: review?(bugs)
In other words, please explain why we do any cloning and actor replacement. Why can't we just start with the right actor?
Attachment #8869484 - Attachment is obsolete: true
Attachment #8869485 - Attachment is obsolete: true
Attachment #8869489 - Attachment is obsolete: true
Attachment #8869758 - Attachment is obsolete: true
Attachment #8869762 - Attachment is obsolete: true
Attached patch part 1 - migration (obsolete) — Splinter Review
Attached patch part 2 - callback storing (obsolete) — Splinter Review
Attached patch part 3 - testsSplinter Review
Attachment #8870715 - Attachment is obsolete: true
Attachment #8870713 - Attachment is obsolete: true
smaug, I would like you to review these patches.
Flags: needinfo?(bugs)
Attachment #8870711 - Flags: review?(bugs)
Attachment #8870716 - Flags: review?(bugs)
Attachment #8870718 - Flags: review?(bugs)
Attachment #8870719 - Flags: review?(bugs)
Attachment #8870720 - Flags: review?(bugs)
Attachment #8870746 - Flags: review?(bugs)
Comment on attachment 8870746 [details] [diff] [review]
part 1 - migration

Kanru, can you review the IPC part of this patch? Thanks.
Attachment #8870746 - Flags: review?(kchen)
Attachment #8870746 - Flags: review?(kchen) → review+
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.
Flags: needinfo?(bugs)
Attachment #8870711 - Flags: review?(bugs) → review+
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.
Attachment #8870746 - Flags: review?(bugs) → review+
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.
Attachment #8870718 - Flags: review?(bugs) → review+
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.
Attachment #8870719 - Flags: review?(bugs) → review+
Attachment #8870720 - Flags: review?(bugs) → review+
> >+  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.
Attachment #8870716 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.