554941 - [E10s] CPOW for synchronous TabChildGlobal messages

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Attachment: patch

This adds support for sending JS objects to TabChildGlobal's sendSyncMessage.

To not make nsFrameMessageManager to depend on IPC classes, 
TabParent::ReceiveMessage converts nsTArray<PObjectWrapperParent*>
to JS array so that message listeners can access objects using
message.objects array.

Note, I added a temporary hack to TabParent::GetGlobalJSObject. I'll file a followup to sort out what we want to do there.

All the message manager tests are still in test.xul. I'll mochitestify
those all soon.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Incremental patch containing my suggestions.

The important parts of the patch look good.  As discussed on IRC, I had some misgivings about the RequestRunToCompletion modifications.  Since it seemed easier to talk about those changes in terms of code, I'm posting an incremental patch that contains my suggestions.

See comment #2 for further explanation.

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

>+void
>+ContentProcessParent::ReportChildAlreadyBlocked()
>+{
>+    if (!mRunToCompletionDepth) {
>+#ifdef DEBUG
>+        printf("Running to completion...\n");
>+#endif
>+        mRunToCompletionDepth = 1;
>+        mShouldCallUnblockChild = false;
>+    }
>+}

There were a couple of things I didn't like about RunningToCompletion(bool).

First, the name was a little off.  We're really only saying the child is blocked right now, and thus the parent should not call BlockChild(), so I changed the name to ReportChildAlreadyBlocked() to reflect that intent.

Second, it seemed a little less than ideal that we had to call RunningToCompletion(bool) twice, taking care to call it with aRunning=false after all CPOW operations had finished.  This new version is used more like RequestRunToCompletion(): call it once before any CPOW operations happen, and the cleanup automatically happens later (specifically, resetting mRunToCompletionDepth without calling UnblockChild()).

Here's where that happens:

> ContentProcessParent::AfterProcessNextEvent(nsIThreadInternal *thread,
>                                             PRUint32 recursionDepth)
> {
>     if (mRunToCompletionDepth &&
>         !--mRunToCompletionDepth) {
> #ifdef DEBUG
>             printf("... ran to completion.\n");
> #endif
>-            UnblockChild();
>+            if (mShouldCallUnblockChild) {
>+                mShouldCallUnblockChild = false;
>+                UnblockChild();
>+            }
>     }

It seems a little easier to understand the intent of calling ReportChildAlreadyBlocked() if we do it in AnswersendSyncMessageToParent():

> bool
> TabParent::AnswersendSyncMessageToParent(const nsString& aMessage,
>                                          const nsString& aJSON,
>                                          const nsTArray<PObjectWrapperParent*>& aObjects,
>                                          nsTArray<nsString>* aJSONRetVal)
> {
>+  static_cast<ContentProcessParent*>(Manager())->ReportChildAlreadyBlocked();
>   return ReceiveMessage(aMessage, PR_TRUE, aJSON, &aObjects, aJSONRetVal);
> }

... instead of conditioning it on aSync in ReceiveMessage():

>-      // Because of rpc, child is already blocked if aSync.
>-      if (aSync) {
>-        ContentProcessParent::GetSingleton()->RunningToCompletition(true);
>-      }
>       manager->ReceiveMessage(mFrameElement,
>                               aMessage,
>                               aSync,
>                               aJSON,
>                               objectsArray,
>                               aJSONRetVal);
>-      if (aSync) {
>-        ContentProcessParent::GetSingleton()->RunningToCompletition(false);
>-      }
>     }
>   }
>   return true;
> }

Finally, what's with these &amp;'s in your event handler functions?

>-          if (m.objects.length == 3 &amp;&amp;
>-              m.objects[0].target.href == m.json.href &amp;&amp;
>-              m.objects[1] == 123 &amp;&amp;
>+          if (m.objects.length == 3 &&
>+              m.objects[0].target.href == m.json.href &&
>+              m.objects[1] == 123 &&

Comment 3

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

(In reply to comment #2)
> It seems a little easier to understand the intent of calling
> ReportChildAlreadyBlocked() if we do it in AnswersendSyncMessageToParent()
Ok.


> Finally, what's with these &'s in your event handler functions?
That script isn't in CDATA, so && can't be used, or at least my editor (jEdit) complained about using &&.

Comment 4

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

And indeed after the & -> & test.xul doesn't work at all.

Comment 5

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

Attachment: patches merged, without & -> & change

I just changed & back to &.
I will rewrite all those testcases for mochitest, and I'll add
then the <![CDATA[ ... ]]>

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 435188 [details] [diff] [review]
patches merged, without & -> & change

Looks good to me.

(In reply to comment #4)
> And indeed after the & -> & test.xul doesn't work at all.

Whoa, crazy.  I knew there must have been a reason!

Comment 7

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

http://hg.mozilla.org/projects/electrolysis/rev/731c8bfd876e

Comment 8

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

Bizarre error on Windows.

e:/builds/moz2_slave/electrolysis-win32-debug/build/obj-firefox/ipc/ipdl/PIFrameEmbeddingChild.cpp(329) : error C2872: 'PContextWrapperChild' : ambiguous symbol
        could be 'e:/builds/moz2_slave/electrolysis-win32-debug/build/obj-firefox/ipc/ipdl/PIFrameEmbeddingChild.cpp(22) : mozilla::jsipc::PContextWrapperChild PContextWrapperChild'
        or 'e:\builds\moz2_slave\electrolysis-win32-debug\build\obj-firefox\ipc\ipdl\_ipdlheaders\mozilla/jsipc/PContextWrapperChild.h(59) : mozilla::jsipc::PContextWrapperChild'
e:/builds/moz2_slave/electrolysis-win32-debug/build/obj-firefox/ipc/ipdl/PIFrameEmbeddingChild.cpp(335) : error C2872: 'PContextWrapperChild' : ambiguous symbol
        could be 'e:/builds/moz2_slave/electrolysis-win32-debug/build/obj-firefox/ipc/ipdl/PIFrameEmbeddingChild.cpp(22) : mozilla::jsipc::PContextWrapperChild PContextWrapperChild'
        or 'e:\builds\moz2_slave\electrolysis-win32-debug\build\obj-firefox\ipc\ipdl\_ipdlheaders\mozilla/jsipc/PContextWrapperChild.h(59) : mozilla::jsipc::PContextWrapperChild'


I think this patch caused the problem. Backing out for now and fixing tomorrow.
http://hg.mozilla.org/projects/electrolysis/rev/497cb03bf371

Comment 9

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

Attachment: guess fix for the windows compilation problem

Re-ordering includes might help here.
http://hg.mozilla.org/projects/electrolysis/rev/7e5cfca4bf12

Comment 10

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

I'll reopen if the fix doesn't work after all.

Comment 11

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

Still no luck. The same bizarre error is there on Windows builds

Comment 12

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

http://hg.mozilla.org/projects/electrolysis/rev/1b9ac85bf4fd