Added [jetpack] to the whiteboard area; if Jetpack is to work on electrolysis, JPW's will be integral to keeping Jetpack's API developer-friendly (not to mention backwards-compatible). bsmedberg or mrbkap, please correct me if I'm totally wrong about this.

I've been thinking about this, and I think we should consider using JPW semi-immediately for jetpack, on a separate thread in the single Firefox process, and do inter-thread communication using IPDL. Each jetpack could have its own JSRuntime/heap, and any long-running computations wouldn't necessary block the browser.

(In reply to comment #2) > Each jetpack could have > its own JSRuntime/heap, and any long-running computations wouldn't necessary > block the browser. If we do this, then we're going to have to write *another* wrapper assuming that these jetpacks want to deal with non-primitive (i.e. object) values, which I'm assuming that we want because every object-tagged value exposed to a script has to be from that script's runtime...

Blake, I don't understand what the other wrapper is for. Wouldn't it deal exclusively in JPW?

IPC Skeleton code. It compiles, but I have no means of testing it yet. I've included a partial implementation of JPW_NewResolve, but I still need to implement the other JPW_* hooks. Please comment on the design and/or request further explanation. I've resorted to rpc semantics for the time being, since synchronous parent-to-child messages are forbidden. Guidance on that issue would be very much appreciated. I'm inclined to let PIFrameEmbedding manage PContentScriptProxy, since there should be a 1:1 mapping between the two, but that means propagating up the rpc semantics of JPWs. Thoughts?

The JPW is going to be completely RPC, except for the constructor/destructor which should be async so that we can pass references to remote JS objects in asynchronous calls. I think you should add a .contentWindow getter to PIFrameEmbedding as the primary way of reaching a JPW, at least for now. Yes, PIFrameEmbedding should manager PScriptProxy... I wouldn't call it PContentScriptProxy because with jetpack it may allow different kinds of access. The JPW is a crutch for extension compatibility (and perhaps Firefox UI compatibility as we port code), so yes, we'll have to propagate up the RPC semantics.

(In reply to comment #6) > The JPW is a crutch for extension compatibility (and perhaps Firefox UI > compatibility as we port code), so yes, we'll have to propagate up the RPC > semantics. If I understand the plans correctly, when jetpack has its own process, it will have its own IPDL protocol tree, so we can de-propagate RPC for ContentProcess.

Maybe... we have non-jetpack extension compatibility to worry about also.

Oh, and Ben the best way to test this will almost certainly be to hook this up with the IPC xpcshell testing harness PTestShell protocol. Either have ~PTestShell() hand back a JPW instead of a string, or invent a new command/response protocol with similar behavior.

Pay no attention to PTestScriptProxy for now. Instead, launch the test.xul browser and click the button labelled "JPW". This will execute the following chrome-process code: var child = window.jaypeedoubleyoo; alert(child.location.href); child.location = prompt("Where to?"); setTimeout(function() { alert(child.location.href); }, 2000); The window.jaypeedoubleyoo property is a JPW that proxies operations for the global window object of the child process. This code reads a subproperty (.href) of the child window's .location property, sets the .location property (which triggers a setter in the child process), and again reads the .location.href property (modified by the setter). The existence of the window.jaypeedoubleyoo property is a temporary hack, obviously inadequate in the presence of multiple child windows.

Awesome. I'm presuming that we want browser.contentWindow to be the JPW.

(In reply to comment #11) > Awesome. I'm presuming that we want browser.contentWindow to be the JPW. Yep, that's the plan.

This patch adds a .crossProcessObjectWrapper property to nsIFrameLoaderOwner and a function getChildGlobalObject() to the global scope of the XPCShell parent process. Locking the child process across multiple CPOW_* operations is not yet implemented, since I anticipate that depending on how JetPack ends up using CPOWs. As a general rule, my implementation simply forwards all JSClass-hooked operations to the JSObject in the child process. This probably misses some edge cases, such as setting the correct (chrome) principal for SJOWs. I'll work on writing tests for that, but I wanted to post this implementation before waiting any longer. See the tests included in my next patch for a demonstration of what's currently possible with CPOWs.

This patch adds a suite of XPCShell unit tests for CPOWs. Also included is a XUL file similar to dom/ipc/test.xul that allows ad-hoc testing in a realistic environment. To launch the XUL file, execute dist/bin/firefox -P e10s -chrome chrome://global/content/cpow/test.xul from the object directory and click the "Run tests" button in the XUL window that appears. To run the XPCShell tests, execute make SOLO_FILE="test_cpow.js" -C js/src/ipc/tests check-interactive from the object directory and type _execute_test() at the prompt.

Compilation error found during a full rebuild (oops). I should mention that this patch has a couple of dependencies. To resolve any confusion about file versions and patch order, please refer to my patch queue repository: http://github.com/benjamn/e10s

js/src/ipc/tests/Makefile.ipc contains hardcoded paths to /home/ben/dev, for the record.

(In reply to comment #16) > js/src/ipc/tests/Makefile.ipc contains hardcoded paths to /home/ben/dev, for > the record. Thanks for catching that, Josh.

Until feedback is in, this will be the last change to the CPOW patch. The substantive change introduced with this version of the patch might be lost in the superficial renaming, so let me draw your attention to it: ObjectWrapperChild no longer stores its own JSContext* data member, but instead consults its manager, the now-more-aptly-named ContextWrapper, to determine which JSContext it should use. Why did I bother with the renaming, except to save characters? First of all, Liaison had to go. Second, I anticipate a follow-up patch in which PContextWrapper supports BeginRequest and EndRequest methods (just like JS_{Begin,End}Request, which operate on JSContext*s), so that the child process can be locked across multiple PObjectWrapper operations. Third, a renaming like this would be a pain to review as a separate patch, and it would bitrot very quickly if I posted it in a follow-up bug.

Comment on attachment 421978 [details] [diff] [review] Renaming ObjectWrapperLiaison to ContextWrapper. >+/* Although PContentProcess manages PContextWrapper, >+ * PContextWrapper instances are always created by (or on behalf of) >+ * a particular PIFrameEmbedding or PTestShell, since each instance of these >+ * subprotocols has its own JSRuntime (and distinct PContextWrappers >+ * are needed for distinct JSRuntimes). I don't quite understand this comment. Why PIFrameEmbedding needs to have its own JSRuntime? I'd thought that JSRuntime is per content process. Or does JSRuntime mean here something else than usually?

(In reply to comment #19) > (From update of attachment 421978 [details] [diff] [review]) > >+/* Although PContentProcess manages PContextWrapper, > >+ * PContextWrapper instances are always created by (or on behalf of) > >+ * a particular PIFrameEmbedding or PTestShell, since each instance of these > >+ * subprotocols has its own JSRuntime (and distinct PContextWrappers > >+ * are needed for distinct JSRuntimes). > I don't quite understand this comment. > Why PIFrameEmbedding needs to have its own JSRuntime? > I'd thought that JSRuntime is per content process. > Or does JSRuntime mean here something else than usually? I should really have renamed JSRuntimeOwner to ContextWrapperOwner, if only because that terminology would make the PContextWrapper constructor message a lot more understandable: parent: PContextWrapper(ContextWrapperOwner cwo); To answer your second question, while it might be true that there's a 1:1 correspondence between PContentProcesses and JSRuntimes, I have no idea how to get a JSRuntime* or a JSContext* from a ContentProcessChild, so it would be inconvenient for ContentProcessChild to allocate/manage ContextWrapperChildren (currently I need at least a JSContext* to create a ContextWrapperChild). However, I do know how to get a JSContext* from a PIFrameEmbedding (via TabChild::mWebNav) or a PTestShell (via TestShellChild::mXPCShell), so it's more convenient to let each PIFrameEmbedding or PTestShell "manage" its own ContextWrapperChild instances (I put "manage" in quotes because PContentProcess is the manager technically). The ContextWrapperOwner union type is necessary so that bool ContentProcessParent:: RecvPContextWrapperConstructor(PContextWrapperParent*, const ContextWrapperOwner&) can tell the appropriate TabParent or TestShellParent about the newly constructed PContextWrapperParent. Fortunately, all this complexity will go away as soon as IPDL has support for multiple manager syntax (bug 540111), for then PContextWrappers can be allocated/managed directly by either PTestShells or PIFrameEmbeddings, rather than by PContentProcesses.

(In reply to comment #20) > it's > more convenient to let each PIFrameEmbedding or PTestShell "manage" its own > ContextWrapperChild instances (I put "manage" in quotes because PContentProcess > is the manager technically). By the way, I chose the name ContextWrapperOwner rather than ContextWrapperManager to emphasize just this distinction, that PContextWrapper is conceptually (though not technically) managed by PIFrameEmbedding/PTestShell. So, where I wrote "'manage'," it might have been better to write "own," just to stay consistent with my own terminology.

As explained in comment 20.

The testing instructions from comment 14 are still valid, as is the previously-attached testing patch. Note that bug 541589 depended on IPDL support for multiple managers, which landed very recently, so you'll need to be up-to-date with the latest electrolysis revisions.

(In reply to comment #6) > The JPW is going to be completely RPC, except for the constructor/destructor > which should be async so that we can pass references to remote JS objects in > asynchronous calls. Is the comment about async ctor/dtor still valid? When handling events in content process, the message handler may need to pass CPOW synchronously to chrome. (Actually, so far I've been thinking to allow CPOW objects only in sync messages) Though, after playing a bit with the current content process listeners patch, I've been thinking (or at least hoping) that the need for CPOWs in event handling could be pretty minimal. Checking what is the target etc in content process and then just sending some json message to chrome could be enough in many cases.

Comment on attachment 423914 [details] [diff] [review] Folding changes from bug 541589 into the main CPOW patch. >@@ -112,16 +113,18 @@ native alreadyAddRefed_nsFrameLoader(alr > interface nsIFrameLoaderOwner : nsISupports > { > /** > * The frame loader owned by this nsIFrameLoaderOwner > */ > readonly attribute nsIFrameLoader frameLoader; > [noscript, notxpcom] alreadyAddRefed_nsFrameLoader GetFrameLoader(); > >+ readonly attribute nsIVariant crossProcessObjectWrapper; >+ To which object does crossProcessObjectWrapper point? To the current inner window in the tab? Does crossProcessObjectWrapper get updated when a new page is loaded? Or does it always point to the outer window, so it doesn't need to be updated?

(In reply to comment #26) > To which object does crossProcessObjectWrapper point? > To the current inner window in the tab? Does crossProcessObjectWrapper > get updated when a new page is loaded? Or does it always point to the > outer window, so it doesn't need to be updated? It's the .globalObject property of the JSContext* derived through the following process: http://github.com/benjamn/e10s/blob/cf9adcb67c0163b59e56e6e530342a72c9a8d253/cpow.diff#L432-453 I have to admit I don't know which window that corresponds to (that's related to the implementation of nsIWebNavigation), or whether it might change (that would seem to depend on where JS_SetGlobalObject is called). If you have knowledge of these things, please advise.

(In reply to comment #25) > (In reply to comment #6) > > The JPW is going to be completely RPC, except for the constructor/destructor > > which should be async so that we can pass references to remote JS objects in > > asynchronous calls. > Is the comment about async ctor/dtor still valid? > When handling events in content process, the message handler may need to > pass CPOW synchronously to chrome. (Actually, so far I've been thinking > to allow CPOW objects only in sync messages) The presence or absence of the sync qualifier for the PObjectWrapper constructor actually makes no difference for current functionality, so I'd be happy to let it be async if that has any benefit: http://github.com/benjamn/e10s/blob/cf9adcb67c0163b59e56e6e530342a72c9a8d253/cpow.diff#L2544-2551 > Though, after playing a bit with the current content process listeners patch, > I've been thinking (or at least hoping) that the need for CPOWs in event > handling could be pretty minimal. Checking what is the target etc in > content process and then just sending some json message to chrome > could be enough in many cases. That sounds more complicated, frankly.

I would very much like the PObjectWrapper constructor/__delete__ to be async messages.

Well, does crossProcessObjectWrapper work properly after you have navigated to a new page? Can you access .document from it and check .documentURI or something like that. nsGlobalWindow::GetContext() does seem to forward the call from inner window to outer, so perhaps things just work. Btw, you should be able to get nsIScriptGlobalObject from webNav. No need to get document. http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#841

(In reply to comment #29) > I would very much like the PObjectWrapper constructor/__delete__ to be async > messages. If chrome process needs CPOW of the event target, it probably needs it synchronously.

No it doesn't. You can send the constructor async and then immediately send your other message using RPC semantics.

(In reply to comment #28) > > Though, after playing a bit with the current content process listeners patch, > > I've been thinking (or at least hoping) that the need for CPOWs in event > > handling could be pretty minimal. Checking what is the target etc in > > content process and then just sending some json message to chrome > > could be enough in many cases. > > That sounds more complicated, frankly. Well, the idea is that we could filter out most of the unnecessary events already in content process (by listening events there) and only handle some events in chrome process.

(In reply to comment #32) > No it doesn't. You can send the constructor async and then immediately send > your other message using RPC semantics. If that gives the same (synchronous-like) behavior on content process, then that is good enough.

(In reply to comment #30) > Well, does crossProcessObjectWrapper work properly after you have > navigated to a new page? Can you access .document from it and check > .documentURI or something like that. Yep, judging from recent experiments, that sort of thing works just fine. Even when navigating back and forth via cpow.history.

Comment on attachment 423914 [details] [diff] [review] Folding changes from bug 541589 into the main CPOW patch. @@ -112,16 +113,18 @@ native alreadyAddRefed_nsFrameLoader(alr interface nsIFrameLoaderOwner : nsISupports { /** * The frame loader owned by this nsIFrameLoaderOwner */ readonly attribute nsIFrameLoader frameLoader; [noscript, notxpcom] alreadyAddRefed_nsFrameLoader GetFrameLoader(); + readonly attribute nsIVariant crossProcessObjectWrapper; Need to bump the iid of this interface. diff --git a/content/base/src/nsFrameLoader.cpp @@ -1513,16 +1517,34 @@ nsFrameLoader::ActivateFrameEvent(const if (mChildProcess) { mChildProcess->SendactivateFrameEvent(nsString(aType), aCapture); return NS_OK; } #endif return NS_ERROR_FAILURE; } +NS_IMETHODIMP +nsFrameLoader::GetCrossProcessObjectWrapper(nsIVariant** cpow) +{ + Uber-nit: Nuke this line. + if ((xpc = nsContentUtils::XPConnect()) && + (stack = nsContentUtils::ThreadJSContextStack()) && + NS_SUCCEEDED(stack->Peek(&cx)) && cx && + mChildProcess->GetGlobalJSObject(cx, &global)) + return xpc->JSToVariant(cx, OBJECT_TO_JSVAL(global), cpow); Nit: Please over-brace if statements like this one. + Uber-nit: nuke trailing whitepace here. diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp --- a/dom/ipc/ContentProcessParent.cpp +++ b/dom/ipc/ContentProcessParent.cpp @@ -48,16 +48,17 @@ #include "nsAutoPtr.h" #include "nsCOMPtr.h" #include "nsServiceManagerUtils.h" #include "nsThreadUtils.h" using namespace mozilla::ipc; using namespace mozilla::net; + Nit: might as well undo this (and get rid of the trailing whitespace at the same time). diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl @@ -53,12 +53,13 @@ rpc protocol PContentProcess child: PIFrameEmbedding(); PTestShell(); parent: PNecko(); + Another unnecessary change? diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp @@ -329,16 +338,20 @@ TabChild::RecvloadURL(const nsCString& u printf("loading %s, %d\n", uri.get(), NS_IsMainThread()); nsresult rv = mWebNav->LoadURI(NS_ConvertUTF8toUTF16(uri).get(), nsIWebNavigation::LOAD_FLAGS_NONE, NULL, NULL, NULL); if (NS_FAILED(rv)) { NS_WARNING("mWebNav->LoadURI failed. Eating exception, what else can I do?"); } + + SendPContextWrapperConstructor() + ->SendPObjectWrapperConstructor(true); No reason to wrap this to two lines. + Uber-nit: nuke the trailing whitespace. @@ -371,16 +384,46 @@ TabChild::RecvsendMouseEvent(const nsStr +static JSContext* +GetJSContext(nsIWebNavigation* webNav) +{ + nsCOMPtr<nsIDOMDocument> domDocument; + nsCOMPtr<nsIDocument> document; + nsCOMPtr<nsIScriptGlobalObject> global; + nsCOMPtr<nsIScriptContext> context; + + if (NS_SUCCEEDED(webNav->GetDocument(getter_AddRefs(domDocument))) && + (document = do_QueryInterface(domDocument)) && + (global = do_QueryInterface(document->GetScriptGlobalObject())) && + (context = do_QueryInterface(global->GetContext()))) + return static_cast<JSContext*>(context->GetNativeContext()); Please over-brace this if statement, as well. diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp @@ -152,16 +155,42 @@ TabParent::AllocPDocumentRenderer(const +bool +TabParent::GetGlobalJSObject(JSContext* cx, JSObject** globalp) +{ + // TODO Unify this code with TestShellParent::GetGlobalJSObject. + nsTArray<PContextWrapperParent*> cwps(1); + ManagedPContextWrapperParent(cwps); + if (cwps.Length() < 1) + return false; + NS_ASSERTION(cwps.Length() == 1, "More than one PContextWrapper?"); + return (static_cast<ContextWrapperParent*>(cwps[0]) + ->GetGlobalJSObject(cx, globalp)); IMO it's worth sticking the cast in a single-use temporary so this fits on one line. diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h @@ -77,16 +85,22 @@ public: const PRInt32& x, const PRInt32& y, const PRInt32& w, const PRInt32& h, const nsString& bgcolor, const PRUint32& flags, const bool& flush); virtual bool DeallocPDocumentRenderer(PDocumentRendererParent* actor); + + virtual PContextWrapperParent* AllocPContextWrapper(); + virtual bool DeallocPContextWrapper(PContextWrapperParent* actor); + More trailing whitespace here... I suspect there's going to be a bunch more. Do a search and replace at least on the added lines? diff --git a/ipc/testshell/TestShellChild.cpp @@ -80,8 +83,25 @@ TestShellChild::RecvPTestShellCommandCon nsString response; if (!mXPCShell->EvaluateString(aCommand, &response)) { return false; } return PTestShellCommandChild::Send__delete__(aActor, response); } + +PContextWrapperChild* +TestShellChild::AllocPContextWrapper() +{ + JSContext* cx; + if (mXPCShell && + (cx = mXPCShell->GetContext())) + return new ContextWrapperChild(cx); No reason to split this over two lines. Also, please over-brace the consequent. diff --git a/js/src/ipc/CPOWTypes.h b/js/src/ipc/CPOWTypes.h +// We already seem to know how to serialize JSBools, huh. +// template <> struct ParamTraits<JSBool> : public CPOWConvertible<JSBool, bool> {}; Probably because we know how to serialize PRBools which are typedef'd to the same thing. diff --git a/js/src/ipc/ContextWrapperChild.h + PObjectWrapperChild* GetOrCreateWrapper(JSObject* obj, + bool makeGlobal = false) + { + if (!obj) // Don't wrap nothin'! + return NULL; + PObjectWrapperChild* wrapper; + while (!mResidentObjectTable.Get(obj, &wrapper)) + mResidentObjectTable.Put(obj, SendPObjectWrapperConstructor(AllocPObjectWrapper(obj), + makeGlobal)); Does this do the right thing if the parent can't construct the wrapper for whatever reason (OOM)? + PObjectWrapperChild* GetOrCreateGlobalWrapper() { + return GetOrCreateWrapper(JS_GetGlobalObject(mContext), true); I don't see where this is used. diff --git a/js/src/ipc/ObjectWrapperChild.cpp + JS_AddNamedRoot(cx, (void*)&mObj, + "mozilla::jsipc::ObjectWrapperChild-rooted JSObject*"); This *can* fail (OOM only, but still). I don't know how you want to deal with that... +ObjectWrapperChild::jsval_to_JSVariant(JSContext* cx, jsval from, JSVariant* to) Maybe you've dealt with this in a later patch, but I'm not clear on who throws an exception on the originating context from this function. +static bool +jsid_from_nsString(JSContext* cx, const nsString& from, jsid* to) +{ + JSString* str = JS_NewUCStringCopyN(cx, from.BeginReading(), from.Length()); This can fail (due to OOM). +ObjectWrapperChild::AnswerAddProperty(const nsString& id) + JS_DefinePropertyById(cx, mObj, interned_id, JSVAL_VOID, NULL, NULL, 0); + return true; This can fail (OOM at the very least, but for other reasons too) so, you'll need to propagate error here. + // Since we fully expect this call to jsval_to_JSVariant to return + // true, we can't just leave vp uninitialized when JS_GetPropertyById + // returns JS_FALSE. This pitfall could be avoided in general if IPDL + // ensured that outparams were pre-initialized to some default value + // (XXXfixme cjones?). Bug on file? +static const PRUint32 sIdArraySlot = 0, sNextIdIndexSlot = 1; +static const PRUint32 sNumNewEnumerateStateSlots = 2; You told me this stuff doesn't work, so I'm not going to review it for now. +ObjectWrapperChild::AnswerNewResolve(const nsString& id, const int& flags, + JSBool* ok, PObjectWrapperChild** obj2) +{ + JSPropertyDescriptor desc; + if (!JS_GetPropertyDescriptorById(cx, mObj, interned_id, flags, &desc)) + return true; + + *ok = JS_TRUE; + + if (desc.obj) + *obj2 = Manager()->GetOrCreateWrapper(desc.obj); This is fine, but I think it might be just as good to return yourself here. It might confuse things like hasOwnProperty, but I'm OK with that for this wrapper. Basically, what we'd be saying is "yes, this property exists, just ask us about it" and then when we get the getProperty call, we'd actually go and fetch the value. +ObjectWrapperChild::AnswerCall(PObjectWrapperChild* receiver, const nsTArray<JSVariant>& argv, + JSBool* ok, JSVariant* rval) +bool +ObjectWrapperChild::AnswerConstruct(const nsTArray<JSVariant>& argv, + JSBool* ok, PObjectWrapperChild** rval) Construct is subtlely incorrect (there isn't a JSAPI for it and Call doesn't set all of the correct bits). So, I'd leave these out until they're needed (AFAIK we only wrap non-callable objects at this point). diff --git a/js/src/ipc/ObjectWrapperParent.cpp --- /dev/null +++ b/js/src/ipc/ObjectWrapperParent.cpp @@ -0,0 +1,614 @@ +#include "mozilla/jsipc/ObjectWrapperParent.h" +#include "mozilla/jsipc/ContextWrapperParent.h" +#include "mozilla/jsipc/CPOWTypes.h" + +#include "jsobj.h" +#include "jsfun.h" +#include "jsutil.h" + +using namespace mozilla::jsipc; + +namespace { + + // This shorthand saves a LOT of typing. + typedef ObjectWrapperParent _; + + static const uintN sFlagsSlot = 0, sWrappedObjSlot = 1; sWrappedObjSlot doesn't wrap a GC thing, so IMO it would be better to go with a private slot + a reserved slot, so that you can use JS_(Get|Set)Private instead of JS_GetReservedSlot + JSVAL_TO_PRIVATE, etc. + static const uintN sNumSlots = 2; + static const uintN CPOW_FLAG_RESOLVING = 1 << 0; + + class AutoResolveFlag + { + JSContext* mContext; + JSObject* mObj; + uintN mOldFlags; + JS_DECL_USE_GUARD_OBJECT_NOTIFIER; Nice! + + static uintN GetFlags(JSContext* cx, JSObject* obj) { + jsval v; + if (JS_GetReservedSlot(cx, obj, sFlagsSlot, &v)) In general, this is not the way that you want to use JSAPI functions. The problem is that if a function fails, the contract specifies that it sets an exception, so, in that case, this pattern could cause an exception to get stranded. However, in this case, we happen to "know" that this JS_GetReservedSlot call can't fail. So I'd take the if statement out altogether. +JSObject* +ObjectWrapperParent::GetJSObject(JSContext* cx) const +{ + JSClass* clasp = const_cast<JSClass*>(&_::sCPOW_JSClass.base); + if (!mObj) { + mObj = JS_NewObject(cx, clasp, NULL, NULL); I know that out of memory isn't a concern for other electrolysis code, but I believe the JS engine will continue to return NULL, so you should probably acknowledge that in some way. +/*static*/ bool +ObjectWrapperParent::jsval_from_JSVariant(JSContext* cx, const JSVariant& from, + jsval* to) +{ + switch (from.type()) { + case JSVariant::Tvoid_t: + *to = JSVAL_VOID; + return true; + case JSVariant::TPObjectWrapperParent: + return jsval_from_PObjectWrapperParent(cx, from.get_PObjectWrapperParent(), to); + case JSVariant::TnsString: + *to = STRING_TO_JSVAL(JS_NewUCStringCopyZ(cx, from.get_nsString().BeginReading())); Another OOM to care about here. +/*static*/ bool +ObjectWrapperParent:: +JSObject_from_PObjectWrapperParent(JSContext* cx, + const PObjectWrapperParent* from, + JSObject** to) +{ + const ObjectWrapperParent* owp = + static_cast<const ObjectWrapperParent*>(from); + *to = owp + ? owp->GetJSObject(cx) + : JSVAL_TO_OBJECT(JSVAL_NULL); AKA "NULL" (or nsnull). +jsid_from_nsString(JSContext* cx, const nsString& from, jsid* to) +{ + JSString* str = JS_NewUCStringCopyZ(cx, from.BeginReading()); Need to deal with OOM. +#define JSVAL_TO_CSTR(CX, V) \ + NS_ConvertUTF16toUTF8(nsString(JS_GetStringChars(JS_ValueToString(CX, V)))).get() This should be #ifdef LOGGING on. This, as written, is pretty dangerous to use in any other usecase (i.e. taking a reference to the result would crash). +/*static*/ JSBool +ObjectWrapperParent::CPOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, + jsval *vp) +{ + return self->CallAddProperty(in_id); We're going to have to document that __define[GS]etter__ won't work on these. But IMO that's OK.

(In reply to comment #36) > interface nsIFrameLoaderOwner : nsISupports > Need to bump the iid of this interface. Done. > +NS_IMETHODIMP > +nsFrameLoader::GetCrossProcessObjectWrapper(nsIVariant** cpow) > +{ > + > > Uber-nit: Nuke this line. Uber-done. > + if ((xpc = nsContentUtils::XPConnect()) && > + (stack = nsContentUtils::ThreadJSContextStack()) && > + NS_SUCCEEDED(stack->Peek(&cx)) && cx && > + mChildProcess->GetGlobalJSObject(cx, &global)) > + return xpc->JSToVariant(cx, OBJECT_TO_JSVAL(global), cpow); > > Nit: Please over-brace if statements like this one. An orthodontist told my parents something similar when I was 11. > + > > Uber-nit: nuke trailing whitepace here. Fixed in a follow-up patch. In general, trying to fix trailing whitespace in this patch will just invite errors when bit-unrotting subsequent patches, so I'm going to save all your whitespace nits for later. No offense. > diff --git a/dom/ipc/ContentProcessParent.cpp > b/dom/ipc/ContentProcessParent.cpp > --- a/dom/ipc/ContentProcessParent.cpp > +++ b/dom/ipc/ContentProcessParent.cpp > @@ -48,16 +48,17 @@ > > #include "nsAutoPtr.h" > #include "nsCOMPtr.h" > #include "nsServiceManagerUtils.h" > #include "nsThreadUtils.h" > > using namespace mozilla::ipc; > using namespace mozilla::net; > + Already fixed in my local version. > parent: > PNecko(); > + > > Another unnecessary change? Already fixed in my local version. > diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp > @@ -329,16 +338,20 @@ TabChild::RecvloadURL(const nsCString& u > printf("loading %s, %d\n", uri.get(), NS_IsMainThread()); > > nsresult rv = mWebNav->LoadURI(NS_ConvertUTF8toUTF16(uri).get(), > nsIWebNavigation::LOAD_FLAGS_NONE, > NULL, NULL, NULL); > if (NS_FAILED(rv)) { > NS_WARNING("mWebNav->LoadURI failed. Eating exception, what else can I > do?"); > } > + > + SendPContextWrapperConstructor() > + ->SendPObjectWrapperConstructor(true); > > No reason to wrap this to two lines. Done. > + > > Uber-nit: nuke the trailing whitespace. Pro-patch-tinating on this, as justified above. > + if (NS_SUCCEEDED(webNav->GetDocument(getter_AddRefs(domDocument))) && > + (document = do_QueryInterface(domDocument)) && > + (global = do_QueryInterface(document->GetScriptGlobalObject())) && > + (context = do_QueryInterface(global->GetContext()))) > + return static_cast<JSContext*>(context->GetNativeContext()); > > Please over-brace this if statement, as well. Done. > diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp > @@ -152,16 +155,42 @@ TabParent::AllocPDocumentRenderer(const > +bool > +TabParent::GetGlobalJSObject(JSContext* cx, JSObject** globalp) > +{ > + // TODO Unify this code with TestShellParent::GetGlobalJSObject. > + nsTArray<PContextWrapperParent*> cwps(1); > + ManagedPContextWrapperParent(cwps); > + if (cwps.Length() < 1) > + return false; > + NS_ASSERTION(cwps.Length() == 1, "More than one PContextWrapper?"); > + return (static_cast<ContextWrapperParent*>(cwps[0]) > + ->GetGlobalJSObject(cx, globalp)); > > IMO it's worth sticking the cast in a single-use temporary so this fits on one > line. Ok. > diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h > @@ -77,16 +85,22 @@ public: > const PRInt32& x, > const PRInt32& y, > const PRInt32& w, > const PRInt32& h, > const nsString& bgcolor, > const PRUint32& flags, > const bool& flush); > virtual bool DeallocPDocumentRenderer(PDocumentRendererParent* actor); > + > + virtual PContextWrapperParent* AllocPContextWrapper(); > + virtual bool DeallocPContextWrapper(PContextWrapperParent* actor); > + > > More trailing whitespace here... I suspect there's going to be a bunch more. > Do a search and replace at least on the added lines? I promise to M-x delete-trailing-whitespace before this lands. > +PContextWrapperChild* > +TestShellChild::AllocPContextWrapper() > +{ > + JSContext* cx; > + if (mXPCShell && > + (cx = mXPCShell->GetContext())) > + return new ContextWrapperChild(cx); > > No reason to split this over two lines. Also, please over-brace the consequent. Done. > diff --git a/js/src/ipc/CPOWTypes.h b/js/src/ipc/CPOWTypes.h > +// We already seem to know how to serialize JSBools, huh. > +// template <> struct ParamTraits<JSBool> : public CPOWConvertible<JSBool, > bool> {}; > > Probably because we know how to serialize PRBools which are typedef'd to the > same thing. Right. Annoying that a JSBool* can't be auto-converted to a PRBool*... > diff --git a/js/src/ipc/ContextWrapperChild.h > + PObjectWrapperChild* GetOrCreateWrapper(JSObject* obj, > + bool makeGlobal = false) > + { > + if (!obj) // Don't wrap nothin'! > + return NULL; > + PObjectWrapperChild* wrapper; > + while (!mResidentObjectTable.Get(obj, &wrapper)) > + mResidentObjectTable.Put(obj, > SendPObjectWrapperConstructor(AllocPObjectWrapper(obj), > + makeGlobal)); > > Does this do the right thing if the parent can't construct the wrapper for > whatever reason (OOM)? I'll change it to avoid putting NULL into the table. > + PObjectWrapperChild* GetOrCreateGlobalWrapper() { > + return GetOrCreateWrapper(JS_GetGlobalObject(mContext), true); > > I don't see where this is used. So it isn't! Nuked. > diff --git a/js/src/ipc/ObjectWrapperChild.cpp > + JS_AddNamedRoot(cx, (void*)&mObj, > + "mozilla::jsipc::ObjectWrapperChild-rooted JSObject*"); > > This *can* fail (OOM only, but still). I don't know how you want to deal with > that... I shall add an assertion. > +ObjectWrapperChild::jsval_to_JSVariant(JSContext* cx, jsval from, JSVariant* > to) > > Maybe you've dealt with this in a later patch, I'mma let you finish, but this later patch to which you're referring is the best patch of all time. Of all time. > but I'm not clear on who throws > an exception on the originating context from this function. AutoCheckOperation. Bug 541003. It's a thing of beauty. > +static bool > +jsid_from_nsString(JSContext* cx, const nsString& from, jsid* to) > +{ > + JSString* str = JS_NewUCStringCopyN(cx, from.BeginReading(), > from.Length()); > > This can fail (due to OOM). Now returning false, which will probably crash the child process. > +ObjectWrapperChild::AnswerAddProperty(const nsString& id) > + JS_DefinePropertyById(cx, mObj, interned_id, JSVAL_VOID, NULL, NULL, 0); > + return true; > > This can fail (OOM at the very least, but for other reasons too) so, you'll > need to propagate error here. Done. > + // Since we fully expect this call to jsval_to_JSVariant to return > + // true, we can't just leave vp uninitialized when JS_GetPropertyById > + // returns JS_FALSE. This pitfall could be avoided in general if IPDL > + // ensured that outparams were pre-initialized to some default value > + // (XXXfixme cjones?). > > Bug on file? Not yet. TODO. > +ObjectWrapperChild::AnswerNewResolve(const nsString& id, const int& flags, > + JSBool* ok, PObjectWrapperChild** obj2) > +{ > + JSPropertyDescriptor desc; > + if (!JS_GetPropertyDescriptorById(cx, mObj, interned_id, flags, &desc)) > + return true; > + > + *ok = JS_TRUE; > + > + if (desc.obj) > + *obj2 = Manager()->GetOrCreateWrapper(desc.obj); > > This is fine, but I think it might be just as good to return yourself here. It > might confuse things like hasOwnProperty, but I'm OK with that for this > wrapper. Basically, what we'd be saying is "yes, this property exists, just ask > us about it" and then when we get the getProperty call, we'd actually go and > fetch the value. Unless you have a more compelling case for breaking hasOwnProperty, I'll leave it be. > +ObjectWrapperChild::AnswerCall(PObjectWrapperChild* receiver, const > nsTArray<JSVariant>& argv, > + JSBool* ok, JSVariant* rval) > +bool > +ObjectWrapperChild::AnswerConstruct(const nsTArray<JSVariant>& argv, > + JSBool* ok, PObjectWrapperChild** rval) > > Construct is subtlely incorrect (there isn't a JSAPI for it and Call doesn't > set all of the correct bits). So, I'd leave these out until they're needed > (AFAIK we only wrap non-callable objects at this point). Can you say more about this? I use CPOW_Construct and CPOW_Call rather extensively in my tests. > diff --git a/js/src/ipc/ObjectWrapperParent.cpp > --- /dev/null > +++ b/js/src/ipc/ObjectWrapperParent.cpp > @@ -0,0 +1,614 @@ > +#include "mozilla/jsipc/ObjectWrapperParent.h" > +#include "mozilla/jsipc/ContextWrapperParent.h" > +#include "mozilla/jsipc/CPOWTypes.h" > + > +#include "jsobj.h" > +#include "jsfun.h" > +#include "jsutil.h" > + > +using namespace mozilla::jsipc; > + > +namespace { > + > + // This shorthand saves a LOT of typing. > + typedef ObjectWrapperParent _; > + > + static const uintN sFlagsSlot = 0, sWrappedObjSlot = 1; > > sWrappedObjSlot doesn't wrap a GC thing, so IMO it would be better to go with a > private slot + a reserved slot, so that you can use JS_(Get|Set)Private instead > of JS_GetReservedSlot + JSVAL_TO_PRIVATE, etc. Didn't know about that. Done. > + static const uintN sNumSlots = 2; > + static const uintN CPOW_FLAG_RESOLVING = 1 << 0; > + > + class AutoResolveFlag > + { > + JSContext* mContext; > + JSObject* mObj; > + uintN mOldFlags; > + JS_DECL_USE_GUARD_OBJECT_NOTIFIER; > > Nice! Glad you like it. More to come! > + > + static uintN GetFlags(JSContext* cx, JSObject* obj) { > + jsval v; > + if (JS_GetReservedSlot(cx, obj, sFlagsSlot, &v)) > > In general, this is not the way that you want to use JSAPI functions. The > problem is that if a function fails, the contract specifies that it sets an > exception, so, in that case, this pattern could cause an exception to get > stranded. Point taken. This is solved in my exceptions patch. > However, in this case, we happen to "know" that this JS_GetReservedSlot call > can't fail. So I'd take the if statement out altogether. Since the only motivation for this suggestion is to be explicit about "knowing" the call is infallible, I've added an assertion to that effect, to be even more explicit. > +JSObject* > +ObjectWrapperParent::GetJSObject(JSContext* cx) const > +{ > + JSClass* clasp = const_cast<JSClass*>(&_::sCPOW_JSClass.base); > + if (!mObj) { > + mObj = JS_NewObject(cx, clasp, NULL, NULL); > > I know that out of memory isn't a concern for other electrolysis code, but I > believe the JS engine will continue to return NULL, so you should probably > acknowledge that in some way. I'll just return NULL. > +/*static*/ bool > +ObjectWrapperParent::jsval_from_JSVariant(JSContext* cx, const JSVariant& > from, > + jsval* to) > +{ > + switch (from.type()) { > + case JSVariant::Tvoid_t: > + *to = JSVAL_VOID; > + return true; > + case JSVariant::TPObjectWrapperParent: > + return jsval_from_PObjectWrapperParent(cx, > from.get_PObjectWrapperParent(), to); > + case JSVariant::TnsString: > + *to = STRING_TO_JSVAL(JS_NewUCStringCopyZ(cx, > from.get_nsString().BeginReading())); > > Another OOM to care about here. Returning false. > +/*static*/ bool > +ObjectWrapperParent:: > +JSObject_from_PObjectWrapperParent(JSContext* cx, > + const PObjectWrapperParent* from, > + JSObject** to) > +{ > + const ObjectWrapperParent* owp = > + static_cast<const ObjectWrapperParent*>(from); > + *to = owp > + ? owp->GetJSObject(cx) > + : JSVAL_TO_OBJECT(JSVAL_NULL); > > AKA "NULL" (or nsnull). Or the literal 0. I prefer the symmetry of the JSVAL_ prefixes. Is JSVAL_NULL going away sometime soon? > +jsid_from_nsString(JSContext* cx, const nsString& from, jsid* to) > +{ > + JSString* str = JS_NewUCStringCopyZ(cx, from.BeginReading()); > > Need to deal with OOM. If allocation fails, is an exception necessarily set, or should I be making up an exception in that case? > +#define JSVAL_TO_CSTR(CX, V) \ > + NS_ConvertUTF16toUTF8(nsString(JS_GetStringChars(JS_ValueToString(CX, > V)))).get() > > This should be #ifdef LOGGING on. This, as written, is pretty dangerous to use > in any other usecase (i.e. taking a reference to the result would crash). Moved to CPOWTypes.h, with CPOW_LOG (which is now #ifdef LOGGING). > +/*static*/ JSBool > +ObjectWrapperParent::CPOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, > + jsval *vp) > +{ > + return self->CallAddProperty(in_id); > > We're going to have to document that __define[GS]etter__ won't work on these. > But IMO that's OK. Getters and setters could be implemented, no?

Have yet to rewrite ObjectWrapperChild::AnswerConstruct so that it doesn't call AnswerCall, and I'm deferring the whitespace cleanup until I can land more than one of the patches in my queue. bent, I'm not sure if jst's promise to review replaces yours or is in addition to yours. So, if you thought you were off the hook for this review, you were probably right.

Now passing the following Date-construction tests: var cd = new cpow.Date, tolerance = 10; // ms do_check_eq(Math.max(Math.abs(cd.getTime() - new Date), tolerance), tolerance); do_check_true(cd instanceof cpow.Date); After adding the missing CHECK_REQUEST(cx) to jorendorff's patch for bug 480850, I found JS_New to work just fine.

Comment on attachment 429048 [details] [diff] [review] Revised patch. In ObjectWrapperChild::jsval_to_JSVariant(): + case JSTYPE_STRING: + *to = nsString(JS_GetStringChars(JSVAL_TO_STRING(from))); This should use nsDependentJSString() instead of nsString, or at the very least it should take the length of the JS string into account to properly deal with embedded '\0's. - In jsid_to_nsString(): + *to = JS_GetStringChars(JSVAL_TO_STRING(v)); Same here. -In js/src/ipc/ObjectWrapperParent.cpp +namespace { + + // This shorthand saves a LOT of typing. + typedef ObjectWrapperParent _; I only see this used in ~10 places, so I'd say this adds more confusion than it saves on typing :) - In ObjectWrapperParent::GetJSObject(JSContext* cx) const: + *mObjSlotContents = this; // See ActorDestroy, Unwrap. I don't get what the mObjSlotContents pointer pointer is needed for. Seems like simply making the private be |this| would do what's needed here. r=jst with that and mrbkap's stuff fixed.

(In reply to comment #40) > (From update of attachment 429048 [details] [diff] [review]) > In ObjectWrapperChild::jsval_to_JSVariant(): > > + case JSTYPE_STRING: > + *to = nsString(JS_GetStringChars(JSVAL_TO_STRING(from))); > > This should use nsDependentJSString() instead of nsString, or at the very least > it should take the length of the JS string into account to properly deal with > embedded '\0's. > > - In jsid_to_nsString(): > > + *to = JS_GetStringChars(JSVAL_TO_STRING(v)); > > Same here. Great idea, but complicated by the fact that nsDependentJSString lives in dom/base/nsJSUtils.h. Filed followup bug 552568. > -In js/src/ipc/ObjectWrapperParent.cpp > > +namespace { > + > + // This shorthand saves a LOT of typing. > + typedef ObjectWrapperParent _; > > I only see this used in ~10 places, so I'd say this adds more confusion than it > saves on typing :) I agree! > - In ObjectWrapperParent::GetJSObject(JSContext* cx) const: > > + *mObjSlotContents = this; // See ActorDestroy, Unwrap. > > I don't get what the mObjSlotContents pointer pointer is needed for. Seems like > simply making the private be |this| would do what's needed here. The issue is that ObjectWrapperParent::ActorDestroy can be called by the IPDL system, when there's not necessarily a JSContext* on the stack (or at least none readily available). A JSContext* is needed to call JS_{Get,Set}Private. I considered storing a JSContext* member in the manager (which is a ContextWrapperParent, after all), so that ActorDestroy could just call Manager()->GetContext(), but that would require figuring out the context in TabParent::AllocPContextWrapper, and using the double-pointer technique seemed simpler.

(In reply to comment #41) > The issue is that ObjectWrapperParent::ActorDestroy can be called by the IPDL > system, when there's not necessarily a JSContext* on the stack (or at least > none readily available). A JSContext* is needed to call JS_{Get,Set}Private. A workaround is to include jsobj.h and use obj->getPrivate() and obj->setPrivate(private). That do not need cx.

Comment on attachment 429048 [details] [diff] [review] Revised patch. >-protocol PTestShell >+rpc protocol PTestShell Is this necessary? >+ObjectWrapperChild::AnswerNewEnumerateInit Looks like |state| isn't properly rooted in this function. Also, JS_Enumerate doesn't root the id array it returns, that's up to the caller. I'd recommend using JSAutoIdArray every time you use JS_Enumerate. >+ JSVariant in_v; >+ >+ if (!jsval_to_nsString(cx, id, &in_id) || >+ !self->jsval_to_JSVariant(cx, *vp, &in_v)) >+ return JS_FALSE; > ... >+ return (self->CallSetProperty(in_id, in_v, >+ &out_ok, &out_v) && This pattern is hazardous, used several times. JSVariant doesn't do any rooting by itself (IPDL unions have no such intelligence) and any time we make an RPC call we could potentially reenter and GC. You should use JSAutoTempValueRooter to protect against that. You might also want to go back and look at all the places you call jsval_to_JSVariant and make sure everything is rooted properly. CPOW_Call and CPOW_Construct have arrays of these guys that need to be protected, too. Also, I'd suggest running whatever tests you have with gcZeal cranked up, see if you can find some other GC bugs.

(In reply to comment #42) > A workaround is to include jsobj.h and use obj->getPrivate() and > obj->setPrivate(private). That do not need cx. Brilliant, that makes things much cleaner.

(In reply to comment #43) > >-protocol PTestShell > >+rpc protocol PTestShell > > Is this necessary? Unfortunately yes, since PTestShell manages PContextWrapper and PContextWrapper manages PObjectWrapper. > >+ObjectWrapperChild::AnswerNewEnumerateInit > > Looks like |state| isn't properly rooted in this function. JSObject_to_JSVariant should wrap it with an ObjectWrapperChild, whose constructor calls JS_AddNamedRoot. Given that the GC is blocked for the duration of the method by the JSAutoRequest, I think I'm safe? > Also, JS_Enumerate doesn't root the id array it returns, that's up to the > caller. I'd recommend using JSAutoIdArray every time you use JS_Enumerate. Agreed. > >+ JSVariant in_v; > >+ > >+ if (!jsval_to_nsString(cx, id, &in_id) || > >+ !self->jsval_to_JSVariant(cx, *vp, &in_v)) > >+ return JS_FALSE; > > ... > >+ return (self->CallSetProperty(in_id, in_v, > >+ &out_ok, &out_v) && > > This pattern is hazardous, used several times. JSVariant doesn't do any rooting > by itself (IPDL unions have no such intelligence) and any time we make an RPC > call we could potentially reenter and GC. You should use JSAutoTempValueRooter > to protect against that. You might also want to go back and look at all the > places you call jsval_to_JSVariant and make sure everything is rooted properly. > CPOW_Call and CPOW_Construct have arrays of these guys that need to be > protected, too. This doesn't matter for JSString*s and double jsvals, or even JSObjects that are not CPOWs (jsval_to_JSVariant fails for those), but you're definitely right about needing to root CPOW JSObjects. I shall audit this case and all the other cases. > Also, I'd suggest running whatever tests you have with gcZeal cranked up, see > if you can find some other GC bugs. Will do.

(In reply to comment #45) > JSObject_to_JSVariant should wrap it with an ObjectWrapperChild, whose > constructor calls JS_AddNamedRoot. Er... Hm? I don't see where that wrapping is happening in AnswerNewEnumerateInit for |state|. > Given that the GC is blocked for the > duration of the method by the JSAutoRequest, I think I'm safe? Not really, no. The request blocks other threads from entering GC but not your own. Anything that can allocate can potentially GC if all the open requests are on the same thread. > This doesn't matter for JSString*s and double jsvals, or even JSObjects that > are not CPOWs (jsval_to_JSVariant fails for those), but you're definitely right > about needing to root CPOW JSObjects. Actually, if you start using nsDependentString then JSString* will need to be rooted as well. The specific cases I quoted may actually be ok since inparams and argv are supposed to be properly rooted before they're passed in. Hm.

jst's three suggestions can be found in comment #40

bent's suggestions can be found in comment #43 All 97 tests in js/src/ipc/tests/unit/test_cpow.js now pass with gcZeal cranked up to 11.

I had a brain-dead bug that caused a segfault when null was passed to the child process as part of a CPOW operation (function argument, property value). This inspired the simplification that dominates this patch: I've removed the null_t field from JSVariant in preference to representing null with the PObjectWrapper field (which is nullable). The only other substantial change is that I made the out-parameter of PObjectWrapper::Construct nullable, so that just in case JS_New fails (due to security restrictions or OOM) the child process can return null and throw an exception to the parent, instead of crashing.

Comment on attachment 433459 [details] [diff] [review] Incremental patch implementing jst's suggestions. Looks good!

Comment on attachment 433464 [details] [diff] [review] Incremental patch addressing jdm's feedback. The changes fix my crash and look sensible to my untrained eye. Remove the commented out null_t references and it passes my rigorous review.

Found a bug that was causing windows builds to fail. I was using a variable-length array of jsvals in ObjectWrapperChild.cpp: jsval jsargs[argv.Length()]; which was convenient because of the automatic (de)allocation, but alas that's a GCC specific feature. I've replaced that idiom with nsTArray<jsval> args(argv.Length()); jsval* jsargs = args.Elements(); as you can see in this patch: http://github.com/benjamn/e10s/blob/66a8861e2479fb0071b14fc774c9f74eceaf71cf/constant-length.diff Is that an acceptable substitute?

Seems reasonable to me.

If you want, you could create an nsAutoTArray<jsval, 5> to try to avoid hitting malloc most of the time.

Comment on attachment 429901 [details] [diff] [review] Rewriting ObjectWrapperChild::AnswerConstruct in terms of JS_New. >diff --git a/js/src/ipc/ObjectWrapperChild.cpp b/js/src/ipc/ObjectWrapperChild.cpp >+ *ok = !!obj; >+ *rval = Manager()->GetOrCreateWrapper(obj); Is it ok to call GetOrCreateWrapper on a null obj? If so, looks good.

Comment on attachment 429048 [details] [diff] [review] Revised patch. Looks good to me. Just make sure you get all of the unfinished TODOs and FIXMEs filed.

(In reply to comment #54) > If you want, you could create an nsAutoTArray<jsval, 5> to try to avoid hitting > malloc most of the time. Lovely: http://github.com/benjamn/e10s/blob/4f2ec4e4b392b04310b911301cf72ffe313ac2d8/constant-length.diff

Comment on attachment 429048 [details] [diff] [review] Revised patch. r- for this one, i'll review the fixes.

Comment on attachment 433460 [details] [diff] [review] Incremental patch implementing bent's suggestions. > ObjectWrapperChild::AnswerNewEnumerateNext(const JSVariant& in_state, > ... > *idp = EmptyString(); You want |idp->Truncate()| instead, though it shouldn't be necessary as IPDL will send in an empty string. >+ if (i >= strIds->Length()) { It should be impossible for i to be greater than strIds->Length() here, right? Please assert. > ObjectWrapperChild::RecvNewEnumerateDestroy(const JSVariant& in_state) You could conceivably move the string deletion code to a new function and share with CPOW_NewEnumerateState_Finalize if you want. > jsval rv, jsargs[argv.Length()]; >+ JSAutoTempValueRooter tvr(cx, argv.Length(), jsargs); So you fixed this in another patch, but remember that nsTArray<T>::AppendElements() can return null on OOM still. r=me!

(In reply to comment #59) > (From update of attachment 433460 [details] [diff] [review]) > > ObjectWrapperChild::AnswerNewEnumerateNext(const JSVariant& in_state, > > ... > > *idp = EmptyString(); > > You want |idp->Truncate()| instead, though it shouldn't be necessary as IPDL > will send in an empty string. Switched to ->Truncate(). > >+ if (i >= strIds->Length()) { > > It should be impossible for i to be greater than strIds->Length() here, right? > Please assert. Done. > > ObjectWrapperChild::RecvNewEnumerateDestroy(const JSVariant& in_state) > > You could conceivably move the string deletion code to a new function and share > with CPOW_NewEnumerateState_Finalize if you want. Fair enough, fixed. > > jsval rv, jsargs[argv.Length()]; > >+ JSAutoTempValueRooter tvr(cx, argv.Length(), jsargs); > > So you fixed this in another patch, but remember that > nsTArray<T>::AppendElements() can return null on OOM still. Returning false in that case. > r=me! Much indebted. Updated patch, for posterity: http://github.com/benjamn/e10s/blob/4383f8846ddc2fd7c09910f5c9f9894d22ca9d88/bent-cpow-comments.diff

Comment on attachment 421655 [details] [diff] [review] Emending js/src/ipc/tests/Makefile.in. See https://bugzilla.mozilla.org/show_bug.cgi?id=541003#c13 for complete CPOW tests.

Pushed to projects/electrolysis: http://hg.mozilla.org/projects/electrolysis/rev/e483a5868df5 Will mark resolved when tests cycle.

Pushed to projects/electrolysis: http://hg.mozilla.org/projects/electrolysis/rev/55b6530567dd Will mark resolved when tests cycle.

Disregard comment #63. That was intended to be https://bugzilla.mozilla.org/show_bug.cgi?id=540126#c9

For anyone looking for documentation on how CPOWs work, the latest can be found here: https://wiki.mozilla.org/Electrolysis/CPOW

And https://wiki.mozilla.org/Content_Process_Event_Handlers has been updated to include some documentation about using CPOWs in sync TabChildGlobal messages.