Closed
Bug 870180
Opened 11 years ago
Closed 11 years ago
Implement CPOW support in MessageManager
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: dvander, Assigned: dvander)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 4 obsolete files)
84.57 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
84.85 KB,
patch
|
billm
:
review+
|
Details | Diff | 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 1•11 years ago
|
||
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 2•11 years ago
|
||
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-
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #747216 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #766150 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
should have tests on monday
Attachment #766177 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #766182 -
Attachment is obsolete: true
Attachment #767380 -
Flags: review?(bugs)
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/129da44ac469
Backed out for causing lots of failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=25149812&tree=Mozilla-Inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/e55829733ccf
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
w/ clobber: https://hg.mozilla.org/integration/mozilla-inbound/rev/58fe760f66d8
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58fe760f66d8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•11 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•