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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

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 :)

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Rebased.

Comment 2

[Jan Varga [:janv]]

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 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks!

Comment 4

[Jan Varga [:janv]]

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());

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part A: Main patch, v1.1

Fixed janv's comments, updated test to cover that case.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

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 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks!

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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 9

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part B: Steve's callback loop patch, v1

This patch is from sworkman, r=me.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/246619a27992
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e377eee59a

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And http://hg.mozilla.org/integration/mozilla-inbound/rev/3ec82c51c8d5 for the silly StaticRefPtr thing.

P.S. Told 'ya so, mrbkap ;)

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, backed out for a missing 'inline' problem :-/

https://hg.mozilla.org/integration/mozilla-inbound/rev/115c1eeb8247

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And relanded with that fixed and a special check to disable the test in XPCShell.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3530d73e8fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/42d91a4ff0b0

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And back out. There's some include order thing that makes 'Preferences' visible without the 'mozilla::' prefix on most builds but not all.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d013b7f7e03

Comment 17

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

(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.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

</3

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part C: Fix test failures, v1

So... I found two races in addition to one bad assertion. Interdiff attached.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Handle hanging actors on shutdown, v1.1

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.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 974176 will fix the ipc implementation on windows for real.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part D: Handle hanging actors on shutdown, v1.2

This patch only has a change in MessageChannel.cpp, the stuff bsmedberg is looking at. mrbkap's part is unchanged.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part E: Add support for Nuwa and the gecko profiler, v1

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.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part E: Add support for Nuwa and the gecko profiler, v2

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.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Part A,C,D,E: https://hg.mozilla.org/integration/mozilla-inbound/rev/e75583a0cbbb
Part B: https://hg.mozilla.org/integration/mozilla-inbound/rev/639a17c98839

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

One bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/653d606f540c

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e75583a0cbbb
https://hg.mozilla.org/mozilla-central/rev/639a17c98839
https://hg.mozilla.org/mozilla-central/rev/653d606f540c

Comment 30

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

Did this cause Bug 976479?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

(In reply to Olli Pettay [:smaug] from comment #30)
> Did this cause Bug 976479?

Yes.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=f0ce508ce329&tochange=e75583a0cbbb&jobname=Rev4%20MacOSX%20Snow%20Leopard%2010.6%20mozilla-inbound%20opt%20test%20mochitest-2

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 33

[Andrew Overholt [:overholt]]

(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.

Comment 34

[Chris Mills [: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?

Comment 35

[Andrea Marchesini [:baku]]

I think that IndexedDB is a bit too complex to be used as example.
What about something simpler such as BroadcastChannel ?