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

Rebased.

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

Thanks!

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.

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.

Thanks!

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.

This patch is from sworkman, r=me.

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

And http://hg.mozilla.org/integration/mozilla-inbound/rev/3ec82c51c8d5 for the silly StaticRefPtr thing. P.S. Told 'ya so, mrbkap ;)

Ok, backed out for a missing 'inline' problem :-/ https://hg.mozilla.org/integration/mozilla-inbound/rev/115c1eeb8247

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

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.

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

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.

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?

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.

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.

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

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

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

Did this cause Bug 976479?

(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

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.

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

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

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