Closed Bug 870180 Opened 7 years ago Closed 7 years ago

Implement CPOW support in MessageManager

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
The message manager API has skeletal support for passing CPOWs over sendSyncMessage/sendAsyncMessage, but it's not implemented. This patch changes the API a little and implements the full functionality, the last piece needed to begin using CPOWs for electrolysis work on mozilla-central.

The original API allowed the user to pass an array of JavaScript objects. The new API is similar, except you can pass a dictionary. The pairs of keys and values are sent as part of the message. If a value is a non-object jsval, its actual value will be sent. If the value is a JSObject or JSFunction, it will become a CPOW if the message leaves the current process.

On the receiving side, the old "objects" property of the message has been replaced with a "remote" property. This contains the key/value pairs that were sent, except that any objects sent over a process boundary will actually be CPOWs.

Since this API is experimental - in that it's intended to be used for our electrolysis work - I added an assert that browser.tabs.remote is true when a user opts in to the new API.
Attachment #747216 - Flags: review?(bugs)
Comment on attachment 747216 [details] [diff] [review]
v1

.remote sounds a bit odd in case message manager runs in the same process.
The idea should be that the same API could be used whether or not the
mm is in content process.
Have you tested in-process message manager? And could we think of
better name than remote.
Comment on attachment 747216 [details] [diff] [review]
v1

># HG changeset patch
># Parent 18364c745bc710e2fa980577130fea7143617b11
>
>diff --git a/content/base/public/nsIMessageManager.idl b/content/base/public/nsIMessageManager.idl
>--- a/content/base/public/nsIMessageManager.idl
>+++ b/content/base/public/nsIMessageManager.idl
>@@ -157,19 +157,18 @@ interface nsIMessageListener : nsISuppor
>    *   {
>    *     target:  %the target of the message. Either an element owning
>    *               the message manager, or message manager itself if no
>    *               element owns it%
>    *     name:    %message name%,
>    *     sync:    %true or false%.
>    *     data:    %structured clone of the sent message data%,
>    *     json:    %same as .data, deprecated%,
>-   *     objects: %array of handles or null, always null if sync is false%
>+   *     remote:  %named table of jsvals/remote objects, or null%
>    *   }
I think objects is better name than remote, especially because message manager doesn't always run in a separate
process and we must support the same API in out-of-process and in-process cases.

>   void sendAsyncMessage([optional] in AString messageName,
>-                        [optional] in jsval obj);
>+                        [optional] in jsval obj,
>+                        [optional] in jsval remote);
objects, and similar also elsewhere.
>-nsFrameLoader::DoSendAsyncMessage(const nsAString& aMessage,
>-                                  const StructuredCloneData& aData)
>+nsFrameLoader::DoSendAsyncMessage(JSContext* aCx,
>+                                  const nsAString& aMessage,
>+                                  const StructuredCloneData& aData,
>+                                  JSObject* aCpows)
> {
>   PBrowserParent* tabParent = GetRemoteBrowser();
>   if (tabParent) {
>     ClonedMessageData data;
>     ContentParent* cp = static_cast<ContentParent*>(tabParent->Manager());
>     if (!BuildClonedMessageDataForParent(cp, aData, data)) {
>       return false;
>     }
>-    return tabParent->SendAsyncMessage(nsString(aMessage), data);
>+    InfallibleTArray<mozilla::jsipc::CpowEntry> cpows;
>+    if (aCpows && !cp->GetJavaScript()->Wrap(aCx, aCpows, &cpows)) {
GetJavaScript() is really odd name. 


>+  JSObject* remote = nullptr;
>+  if (aArgc >= 3 && aRemote.isObject()) {
>+    remote = JSVAL_TO_OBJECT(aRemote);
toObject()? Same also elsewhere.

>-        // To keep compatibility with e10s message manager,
>-        // define empty objects array.
>-        if (!objectsArray) {
>-          // Because we want JS messages to have always the same properties,
>-          // create array even if len == 0.
>-          objectsArray = JS_NewArrayObject(ctx, 0, nullptr);
>-          if (!objectsArray) {
>-            return NS_ERROR_OUT_OF_MEMORY;
>+        JS::RootedObject cpows(ctx);
JS::RootedObject shouldn't be used outside js/.
Use JS::Rooted<JSObject*>
Same thing with JS::RootedValue
>+  ~nsAsyncMessageToSameProcessParent()
>+  {
>+    if (mCpows)
>+      js_RemoveObjectRoot(mRuntime, &mCpows);
if (expr) {
  stmt;
}

>-  virtual bool DoSendSyncMessage(const nsAString& aMessage,
>+  virtual bool DoSendSyncMessage(JSContext* aCx,
>+                                 const nsAString& aMessage,
>                                  const mozilla::dom::StructuredCloneData& aData,
>+				 JSObject *aCpows,
You have some tabs here. Use spaces. And JSObject* aCpows


>+  virtual bool DoSendAsyncMessage(JSContext* aCx,
>+                                  const nsAString& aMessage,
>+                                  const mozilla::dom::StructuredCloneData& aData,
>+				  JSObject* aCpows)
Some tabs here too

>+class CpowHolder
>+{
>+  public:
>+    virtual bool ToObject(JSContext* cx, JSObject** objp) = 0;
aCx, aObjp

>+};
>+
>+class SameProcessCpowHolder : public CpowHolder
>+{
>+  public:
>+    SameProcessCpowHolder(JSObject* obj)
aObj

>+    bool ToObject(JSContext* cx, JSObject** objp);
aCx, aObjp

>+  ~nsAsyncMessageToParent()
>+  {
>+    if (mCpows)
>+        js_RemoveObjectRoot(mRuntime, &mCpows);
if (expr) {
  stmt;
}
> 
>     SendGetProcessAttributes(&mID, &mIsForApp, &mIsForBrowser);
> 
>+    GetJavaScript();
>+
I hope GetJavasScript() will be renamed, but this method call will still need some explanation.

>-ContentParent::DoSendAsyncMessage(const nsAString& aMessage,
>-                                  const mozilla::dom::StructuredCloneData& aData)
>+ContentParent::DoSendAsyncMessage(JSContext* aCx,
>+                                  const nsAString& aMessage,
>+                                  const mozilla::dom::StructuredCloneData& aData,
>+                                  JSObject *aCpows)
JSObject* aCpows


> {
>   ClonedMessageData data;
>   if (!BuildClonedMessageDataForParent(this, aData, data)) {
>     return false;
>   }
>-  return SendAsyncMessage(nsString(aMessage), data);
>+  InfallibleTArray<CpowEntry> cpows;
>+  if (!GetJavaScript()->Wrap(aCx, aCpows, &cpows)) {
>+      return false;
Use 2 spaces for indentation


>+    virtual bool DoSendAsyncMessage(JSContext* aCx,
>+                                    const nsAString& aMessage,
>+                                    const mozilla::dom::StructuredCloneData& aData,
>+                                    JSObject *cpows);
JSObject* aCpows

>+JavaScriptShared::toGecko(JSContext *cx, jsid id, nsString *to)
toGecko is a bit odd name. Why not call it toString?
Or could you perhaps just use nsDependentJSString from nsJSUtils ? (though perhaps including stuff
from dom/base isn't ok ?)

>+struct JSIID {
>+    uint32_t m0;
>+    uint16_t m1;
>+    uint16_t m2;
>+    uint8_t m3_0;
>+    uint8_t m3_1;
>+    uint8_t m3_2;
>+    uint8_t m3_3;
>+    uint8_t m3_4;
>+    uint8_t m3_5;
>+    uint8_t m3_6;
>+    uint8_t m3_7;
>+};
>+
>+union JSVariant {
>+    void_t;     /* |undefined| */
>+    nsString;   /* StringValue(x) */
>+    uint32_t;   /* JSObject */
>+    double;     /* NumberValue(x) */
>+    bool;       /* BooleanValue(x) */
>+    JSIID;      /* XPC nsIID */
>+};
>+
>+struct ReturnStatus {
>+    bool ok;        /* True for success, false for failure. */
>+    JSVariant exn;  /* Exception, if |ok| is false. */
>+};
>+
>+union JSParam {
>+    void_t;     /* value is strictly an xpc out param */
>+    JSVariant;  /* actual value to pass through */
>+};
>+
>+struct PPropertyDescriptor
>+{
>+    uint32_t    objId;
>+    uint32_t    attrs;
>+    uint32_t    shortid;
>+    JSVariant   value;
>+
>+    // How to interpret these values depends on whether JSPROP_GETTER/SETTER
>+    // are set. If set, the corresponding value is a CPOW or 0 for NULL.
>+    // Otherwise, the following table is used:
>+    //
>+    //  0 - NULL
>+    //  1 - Default getter or setter.
>+    //  2 - js_GetterOnlyPropertyStub (setter only)
>+    //  3 - Unknown
>+    uint32_t    getter;
>+    uint32_t    setter;
>+};
>+
>+struct CpowEntry {
>+  nsString name;
>+  JSVariant value;
>+};
You're mixing 2 and 4 space indentations in this file



Need some e10s and in-process tests for cpow + message manager.
Attachment #747216 - Flags: review?(bugs) → review-
Attached patch v1 rebased (obsolete) — Splinter Review
Attachment #747216 - Attachment is obsolete: true
Attached patch v2 - exact rooting (obsolete) — Splinter Review
Attachment #766150 - Attachment is obsolete: true
Attached patch v3 - nits picked (obsolete) — Splinter Review
should have tests on monday
Attachment #766177 - Attachment is obsolete: true
Attachment #766182 - Attachment is obsolete: true
Attachment #767380 - Flags: review?(bugs)
Comment on attachment 767380 [details] [diff] [review]
v4, nits picked + tests

> {
> public:
>-  nsAsyncMessageToChild(nsFrameLoader* aFrameLoader,
>+  nsAsyncMessageToChild(JSContext* aCx,
>+                        nsFrameLoader* aFrameLoader,
>                         const nsAString& aMessage,
>-                        const StructuredCloneData& aData)
>-    : mFrameLoader(aFrameLoader), mMessage(aMessage)
>+                        const StructuredCloneData& aData,
>+                        JS::HandleObject aCpows)
JS::Handle<JSObject*>
Same also elsewhere

>+  JSRuntime* mRuntime;
Do we really to have mRuntime? Can't we just access the runtime for the current thread or something?
This all runs in the main thread, so xpconnect should be able to give the runtime.

> nsFrameMessageManager::BroadcastAsyncMessage(const nsAString& aMessageName,
>-                                             const JS::Value& aObject,
>+                                             const JS::Value& aJSON,
>+                                             const jsval& aObjects,
JS::Value
(isn't jsval just a typedef)

>         JS_DefineProperty(ctx, param, "json", json, nullptr, nullptr, JSPROP_ENUMERATE); // deprecated
>         JS_DefineProperty(ctx, param, "data", json, nullptr, nullptr, JSPROP_ENUMERATE);
>-        JS_DefineProperty(ctx, param, "objects", objectsv, nullptr, nullptr, JSPROP_ENUMERATE);
>+        JS_DefineProperty(ctx, param, "objects", cpowsv, NULL, NULL, JSPROP_ENUMERATE);
s/NULL/nullptr/



>+++ b/content/base/test/chrome/Makefile.in
>@@ -53,11 +53,14 @@ MOCHITEST_CHROME_FILES = \
>     test_csp_bug773891.html \
>     test_bug816340.xul \
>     file_bug816340.xul \
>     test_domparsing.xul \
>     test_bug814638.xul \
>     host_bug814638.xul \
>     test_document_register.xul \
>     frame_bug814638.xul \
>+    test_cpows.xul \
>+    cpows_parent.xul \
>+    cpows_child.js \
>     $(NULL)
> 
> include $(topsrcdir)/config/rules.mk
>diff --git a/content/base/test/chrome/cpows_child.js b/content/base/test/chrome/cpows_child.js
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/chrome/cpows_child.js
>@@ -0,0 +1,54 @@
>+dump('loaded child cpow test\n');
>+
>+(function start() {
>+    sync_test();
>+    async_test();
>+    sendAsyncMessage("cpows:done", {});
>+  }
>+)();
>+
>+function ok(condition, message) {
>+  dump('condition: ' + condition  + ', ' + message + '\n');
>+  if (!condition) {
>+    sendAsyncMessage("cpows:fail", { message: message });
>+    throw 'failed check: ' + message;
>+  }
>+}
>+
>+var sync_obj;
>+var async_obj;
>+
>+function make_object()
>+{
>+  let o = { };
>+  o.i = 5;
>+  o.b = true;
>+  o.s = "hello";
>+  o.x = { i: 10 };
>+  o.f = function () { return 99; }
>+  return { "data": o };
>+}
>+
>+function make_json()
>+{
>+  return { check: "ok" };
>+}
>+
>+function sync_test()
>+{
>+  dump('beginning cpow sync test\n');
>+  sync_obj = make_object();
>+  sendSyncMessage("cpows:sync",
>+    make_json(),
>+    make_object());
>+}
>+
>+function async_test()
>+{
>+  dump('beginning cpow async test\n');
>+  async_obj = make_object();
>+  sendAsyncMessage("cpows:async",
>+    make_json(),
>+    async_obj);
>+}
>+
>diff --git a/content/base/test/chrome/cpows_parent.xul b/content/base/test/chrome/cpows_parent.xul
>new file mode 100644
>+    function testCpowMessage(message) {
>+      ok(message.json.check == "ok", "correct json");
>+
>+      let data = message.objects.data;
>+      ok(data.i === 5, "integer property");
>+      ok(data.b === true, "boolean property");
>+      ok(data.s === "hello", "string property");
>+      ok(data.x.i === 10, "nested property");
>+      ok(data.f() === 99, "function call");
>+
>+      data.i = 6;
>+      data.b = false;
>+      data.s = "bye";
>+      data.x = null;
>+      ok(data.i === 6, "integer property");
>+      ok(data.b === false, "boolean property");
>+      ok(data.s === "bye", "string property");
>+      ok(data.x === null, "nested property");

There should be a test for sending things like DOM nodes.
The child side could for example send .content.document.
If that is not yet supported, at least better to test that result is
null or some such

>+ContentParent::DoSendAsyncMessage(JSContext* aCx,
>+                                  const nsAString& aMessage,
>+                                  const mozilla::dom::StructuredCloneData& aData,
>+                                  JS::HandleObject aCpows)
> {
>   ClonedMessageData data;
>   if (!BuildClonedMessageDataForParent(this, aData, data)) {
>     return false;
>   }
>-  return SendAsyncMessage(nsString(aMessage), data);
>+  InfallibleTArray<CpowEntry> cpows;
>+  if (!GetCPOWManager()->Wrap(aCx, aCpows, &cpows)) {
>+      return false;
2 extra spaces

>+    virtual bool DoSendAsyncMessage(JSContext* aCx,
>+                                    const nsAString& aMessage,
>+                                    const mozilla::dom::StructuredCloneData& aData,
>+                                    JS::HandleObject cpows);
aCpows

>-    AsyncMessage(nsString aMessage, ClonedMessageData aData);
>+    AsyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] cpows);
aCpows

>-    sync SyncMessage(nsString aMessage, ClonedMessageData aData)
>+    sync SyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] cpows)
ditto

>-    sync SyncMessage(nsString aMessage, ClonedMessageData aData)
>+    sync SyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] cpows)
ditto, and elsewhere

>+++ b/js/ipc/JavaScriptShared.cpp
This needs js eng peer review
Attachment #767380 - Flags: review?(bugs) → review+
Bill could you look at the changes to js/*?
Attachment #770533 - Flags: review?(wmccloskey)
Comment on attachment 770533 [details] [diff] [review]
rebase, rooting support

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

The js/ stuff looks good.

::: js/ipc/JavaScriptShared.cpp
@@ +421,5 @@
> +        str = JS_NewUCStringCopyN(cx, name.BeginReading(), name.Length());
> +        if (!str)
> +            return false;
> +
> +        if (!JS_ValueToId(cx, STRING_TO_JSVAL(str), id.address()))

StringValue. Also, I think you might be able to shortcut some of this by using JS_DefineUCProperty instead of JS_DefinePropertyById.

::: js/src/jsgc.cpp
@@ +1082,5 @@
>      return AddRoot(cx, rp, name, JS_GC_ROOT_SCRIPT_PTR);
>  }
>  
> +extern JS_FRIEND_API(bool)
> +js_AddObjectRoot(JSRuntime *rt, JSObject **objp)

Can we just change the existing JS_AddObjectRoot to take a runtime rather than a context? There are only a few callers, and they could be changed to convert a cx to an rt via JS_GetRuntime. Then we don't need to add any extra API code.

@@ +1088,5 @@
> +    return AddRoot(rt, objp, NULL, JS_GC_ROOT_OBJECT_PTR);
> +}
> +
> +extern JS_FRIEND_API(void)
> +js_RemoveObjectRoot(JSRuntime *rt, JSObject **objp)

We already have JS_RemoveObjectRootRT, which should work here I think.
Attachment #770533 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) [away until July 1] from comment #9)
> > +extern JS_FRIEND_API(bool)
> > +js_AddObjectRoot(JSRuntime *rt, JSObject **objp)
> 
> Can we just change the existing JS_AddObjectRoot to take a runtime rather
> than a context? There are only a few callers, and they could be changed to
> convert a cx to an rt via JS_GetRuntime. Then we don't need to add any extra
> API code.

I started to do this but it turned out to be a lot more work and makes the rooting API inconsistent. I thought about cleaning up the API in a separate bug, but it doesn't seem salvageable. I'm hoping someday we just get auto rooters that replace it at all.
Since you caused every test on Mac debug (and no other platform) on your push to fail, but caused every test on Windows debug (and no other platform) on the next push to fail, I suspect you just needed a clobber, and just need to edit the /CLOBBER file and repush.
https://hg.mozilla.org/mozilla-central/rev/58fe760f66d8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.