Bug 956218 (PBackground)

Add a mechanism for communicating with a non-main I/O thread via thread and process links

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

({dev-doc-complete})

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

73.46 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.85 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.48 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
16.81 KB, patch
mrbkap
: review+
benjamin
: review+
Details | Diff | Splinter Review
6.36 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Posted patch Patch, v1 (obsolete) — Splinter Review
One of the things we need for indexedDB-in-workers is a mechanism to communicate with a non-main thread from both child processes and other parent-process worker threads. I'm calling this PBackground for now, mrbkap has at least given tacit approval to the name :)
Attachment #8355419 - Flags: review?(mrbkap)
Alias: PBackground
Posted patch Patch, v1 (obsolete) — Splinter Review
Rebased.
Attachment #8355419 - Attachment is obsolete: true
Attachment #8355419 - Flags: review?(mrbkap)
Attachment #8360735 - Flags: review?(mrbkap)

Updated

6 years ago
Blocks: 961049

Comment 2

6 years ago
Comment on attachment 8360735 [details] [diff] [review]
Patch, v1

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

I went through the patch to get a better understanding of PBackground, here are some nits.

::: dom/ipc/ContentChild.cpp
@@ +314,5 @@
> +
> +    virtual void
> +    ActorCreated(PBackgroundChild* aActor) MOZ_OVERRIDE
> +    {
> +        MOZ_ASSERT(aActor, "Failed to create a PackgroundChild actor!");

s/PackgroundChild/PBackgroundChild

@@ +320,5 @@
> +
> +    virtual void
> +    ActorFailed() MOZ_OVERRIDE
> +    {
> +        MOZ_CRASH("Failed to create a PackgroundChild actor!");

s/PackgroundChild/PBackgroundChild

::: dom/ipc/ContentParent.cpp
@@ +180,5 @@
> +
> +    virtual void
> +    ActorCreated(PBackgroundChild* aActor) MOZ_OVERRIDE
> +    {
> +        MOZ_RELEASE_ASSERT(aActor, "Failed to create a PackgroundChild actor!");

s/PackgroundChild/PBackgroundChild

@@ +191,5 @@
> +
> +    virtual void
> +    ActorFailed() MOZ_OVERRIDE
> +    {
> +        MOZ_CRASH("Failed to create a PackgroundChild actor!");

s/PackgroundChild/PBackgroundChild

::: ipc/glue/BackgroundImpl.cpp
@@ +158,5 @@
> +
> +  static void
> +  AssertIsOnBackgroundThread()
> +  {
> +    MOZ_ASSERT(IsOnBackgroundThread);

s/IsOnBackgroundThread/IsOnBackgroundThread()

@@ +1137,5 @@
> +  // This happens on the main thread but before XPCOM has started so we can't
> +  // assert that we're being called on the main thread here.
> +
> +  MOZ_ASSERT(sThreadLocalIndex == kBadThreadLocalIndex,
> +             "BackgroundChild::Init() called more than once!");

BackgroundChild::Init() -> BackgroundChild::Startup() ?

@@ +1230,5 @@
> +                                 nsIIPCBackgroundChildCreateCallback* aCallback)
> +{
> +  MOZ_ASSERT(aCallback);
> +  MOZ_ASSERT(sThreadLocalIndex != kBadThreadLocalIndex,
> +             "BackgroundChild::Init() was never called!");

BackgroundChild::Init() -> BackgroundChild::Startup() ?

Comment 4

6 years ago
Comment on attachment 8360735 [details] [diff] [review]
Patch, v1

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

::: ipc/glue/BackgroundImpl.cpp
@@ +1252,5 @@
> +    threadLocalInfo = newInfo.forget();
> +  }
> +
> +  if (threadLocalInfo->mActor) {
> +    nsCOMPtr<nsIRunnable> runnable = new CreateCallbackRunnable();

The callback runnable needs to be initialized with the actor, this works for me:
nsRefPtr<ChildImpl> actor = threadLocalInfo->mActor;
nsCOMPtr<nsIRunnable> runnable = new CreateCallbackRunnable(actor.forget());
Fixed janv's comments, updated test to cover that case.
Attachment #8360735 - Attachment is obsolete: true
Attachment #8360735 - Flags: review?(mrbkap)
Attachment #8364013 - Flags: review?(mrbkap)
Comment on attachment 8364013 [details] [diff] [review]
Part A: Main patch, v1.1

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

::: ipc/glue/BackgroundImpl.cpp
@@ +1000,5 @@
> +
> +NS_IMETHODIMP
> +ParentImpl::ShutdownObserver::Observe(nsISupports* aSubject,
> +                                      const char* aTopic,
> +                                      const PRUnichar* aData)

Just letting you know that you might need to s/PRUnichar/char16_t/ the next time you update your repo. M-c rev 165041 won't build without it (at least on my Mac 10.8).

Same for ChildImpl, and BackgroundTester::Observe in ContentParent.cpp.
Comment on attachment 8364013 [details] [diff] [review]
Part A: Main patch, v1.1

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

I'm not done yet, but here are a few small comments that I've noticed.

::: dom/ipc/ContentParent.cpp
@@ +188,5 @@
> +                           "Failed to create a PBackgroundChild actor!");
> +
> +        NS_NAMED_LITERAL_CSTRING(testStr, "0123456789");
> +
> +        MOZ_RELEASE_ASSERT(aActor->SendPBackgroundTestConstructor(testStr),

Here and below: I've been trying to get used to this, but can't. Having side-effects in the condition of *_ASSERT() parameters is too much of a jump to me. Can you use a temporary |bool ok = ...; MOZ_RELEASE_ASSERT(ok)|?

::: ipc/glue/BackgroundImpl.cpp
@@ +30,5 @@
> +#include "nsXULAppAPI.h"
> +#include "nsXPCOMPrivate.h"
> +#include "prthread.h"
> +
> +#define CRASH_IN_CHILD_PROCESS(_msg)                                           \

As discussed, this needs to at least assert in the parent process.

@@ +129,5 @@
> +
> +  // This is only modified on the main thread. It is a FIFO queue for callbacks
> +  // waiting for the background thread to be created.
> +  static StaticAutoPtr<nsTArray<nsRefPtr<CreateCallback>>>
> +    sPendingCallbacks;

Nit: seems like this fits on one line.

@@ +882,5 @@
> +    }
> +  }
> +
> +  if (sBackgroundThread) {
> +    nsCOMPtr<nsIThread> thread = sBackgroundThread.get();

Why is the .get() necessary? I guess you can't swap either because of the nsCOMPtr/nsRefPtr difference?

@@ +895,5 @@
> +    // If this is final shutdown then we need to spin the event loop while we
> +    // wait for all the actors to be cleaned up.
> +    if (sShutdownHasStarted && sLiveActorCount) {
> +      nsIThread* currentThread = NS_GetCurrentThread();
> +      MOZ_ASSERT(currentThread);

Can't you just get the main thread here?

@@ +928,5 @@
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT_IF(mIsOtherProcessActorDEBUG, mContent);
> +  MOZ_ASSERT_IF(!mIsOtherProcessActorDEBUG, !mContent);
> +  MOZ_ASSERT_IF(mIsOtherProcessActorDEBUG, mTransport);
> +  MOZ_ASSERT_IF(!mIsOtherProcessActorDEBUG, !mTransport);

This is one of those cases where a little boolean logic actually makes things a little clearer, IMO.

@@ +1207,5 @@
> +    MOZ_CRASH("Failed to dispatch OpenActorRunnable!");
> +  }
> +
> +  // This value is only checked against null to determine success/failure, so
> +  // there is no need to worry bout the reference count here.

"about"
Comment on attachment 8364013 [details] [diff] [review]
Part A: Main patch, v1.1

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

This actually looks really good. I have one small comment to address and then we need to get real implementations to really shake this stuff out ASAP.

Also, don't forget to add additional tests for GetOrCreateForCurrentThread as we discussed.

::: ipc/glue/BackgroundImpl.cpp
@@ +1261,5 @@
> +    return true;
> +  }
> +
> +  if (!created) {
> +    return true;

Please add a comment (I think here should be OK) explaining how this early return interacts with the while loops that Steve had to add.
Attachment #8364013 - Flags: review?(mrbkap) → review+
This patch is from sworkman, r=me.
Attachment #8371326 - Flags: review+
And http://hg.mozilla.org/integration/mozilla-inbound/rev/3ec82c51c8d5 for the silly StaticRefPtr thing.

P.S. Told 'ya so, mrbkap ;)
And back out. There's some include order thing that makes 'Preferences' visible without the 'mozilla::' prefix on most builds but not all.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #15)
> And back out. There's some include order thing that makes 'Preferences'
> visible without the 'mozilla::' prefix on most builds but not all.

That's the unified build stuff most likely.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> That's the unified build stuff most likely.

</3
So... I found two races in addition to one bad assertion. Interdiff attached.
Attachment #8372384 - Flags: review?(mrbkap)
Attachment #8372384 - Attachment description: changes2.patch → Fix test failures, v1
Attachment #8372384 - Flags: review?(mrbkap) → review+
This applies on top of all the other patches and makes us gracefully handle hung actors on shutdown off of a timeout. This is only necessary on Windows because of a problem in the implementation of the Windows IPC channel. I want to fix the underlying problem in another bug so this is just a temporary workaround.
Attachment #8377893 - Flags: review?(mrbkap)
Comment on attachment 8377893 [details] [diff] [review]
Handle hanging actors on shutdown, v1.1

bsmedberg, can you review the tiny change ipc/glue/MessageChannel.cpp? Our windows channels are getting stuck in the ChannelOpening state and it seems perfectly reasonable to forcefully close a channel in that state.
Attachment #8377893 - Flags: review?(benjamin)
Bug 974176 will fix the ipc implementation on windows for real.
This patch only has a change in MessageChannel.cpp, the stuff bsmedberg is looking at. mrbkap's part is unchanged.
Attachment #8377893 - Attachment is obsolete: true
Attachment #8377893 - Flags: review?(mrbkap)
Attachment #8377893 - Flags: review?(benjamin)
Attachment #8378391 - Flags: review?(mrbkap)
Attachment #8378391 - Flags: review?(benjamin)
Comment on attachment 8378391 [details] [diff] [review]
Part D: Handle hanging actors on shutdown, v1.2

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

Phew. Spinning the event loop to run your own timer callback.

A couple of questions, but I don't see any reason for this not to land.

::: ipc/glue/BackgroundImpl.cpp
@@ +898,5 @@
>                 "already begun!");
>      return false;
>    }
>  
> +  nsresult rv;

Can this declaration go inside the if (!sShutdownTimer) consequent?

@@ +1263,5 @@
> +
> +  AssertIsOnBackgroundThread();
> +
> +  if (!mActorArray->IsEmpty()) {
> +    nsAutoTArray<ParentImpl*, 50> actorsToClose(*mActorArray);

50?
Attachment #8378391 - Flags: review?(mrbkap) → review+
Attachment #8364013 - Attachment description: Patch, v1.1 → Part A: Main patch, v1.1
Attachment #8371326 - Attachment description: Steve's callback loop patch, v1 → Part B: Steve's callback loop patch, v1
Attachment #8372384 - Attachment description: Fix test failures, v1 → Part C: Fix test failures, v1
Attachment #8378391 - Attachment description: Handle hanging actors on shutdown, v1.2 → Part D: Handle hanging actors on shutdown, v1.2

Updated

5 years ago
Attachment #8378391 - Flags: review?(benjamin) → review+
Bah, Nuwa landed and now the emulator tests go orange with a pretty cryptic timeout...

While I was here I added the profiler hooks too.
Attachment #8379744 - Flags: review?(khuey)
Ok, found the problem. There's a deadlock hazard between 'opens' protocols and the Nuwa fork mechanism because Nuwa blocks the IO thread... We should probably figure out a way to make sure Nuwa never does that, but this is sufficient for now.
Attachment #8379744 - Attachment is obsolete: true
Attachment #8379744 - Flags: review?(khuey)
Attachment #8380359 - Flags: review?(khuey)
Did this cause Bug 976479?

Updated

5 years ago
No longer blocks: 1015466

Updated

5 years ago
Blocks: 1015466
I'm not sure if this is the proper way to ask for it, but an MDN page describing this protocol would be really appreciated.
Keywords: dev-doc-needed
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #32)
> I'm not sure if this is the proper way to ask for it, but an MDN page
> describing this protocol would be really appreciated.

khuey wrote something: http://blog.kylehuey.com/post/136679681701/what-is-pbackground

Chris, is Kyle's blog post something that can be "MDN-ified"? Somewhere related to https://developer.mozilla.org/en-US/docs/Mozilla/IPDL.
Flags: needinfo?(cmills)
(In reply to Andrew Overholt [:overholt] from comment #33)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #32)
> > I'm not sure if this is the proper way to ask for it, but an MDN page
> > describing this protocol would be really appreciated.
> 
> khuey wrote something:
> http://blog.kylehuey.com/post/136679681701/what-is-pbackground
> 
> Chris, is Kyle's blog post something that can be "MDN-ified"? Somewhere
> related to https://developer.mozilla.org/en-US/docs/Mozilla/IPDL.

I've done so, see

https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/PBackground

Do you think this gives enough detail? A concrete example might be nice. I link to the IndexedDB ActorsChild.cpp/ActorsParent.cpp files on DXR, so maybe some one could pull out (or point to) a few snippets in there for those who want to look at a specific usage example?
Flags: needinfo?(cmills)
I think that IndexedDB is a bit too complex to be used as example.
What about something simpler such as BroadcastChannel ?
You need to log in before you can comment on or make changes to this bug.