Chris Peterson [:cpeterson] Reporter
	Description • 11 years ago

      No description provided.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 3 • 11 years ago

The basic idea here is to allow CPOWs in both directions (from the parent to the child and from the child to the parent). Currently we only allow parent->child CPOWs. The main use case for child->parent CPOWs is when an addon wants to attach an event listener to some object in the content process; in this case, it needs to pass a function object down to the child, and there's no way to do that now.

To do this, we'll make parent->child references weak and child->parent references strong. That way, the child is solely responsible for deciding what's garbage.

To make this work, I've factored most of the important code out of JavaScriptParent and JavaScript child into classes called WrapperOwner (which manages CPOWs) and WrapperAnswer (which answers CPOW queries). Then I made JavaScriptChild and JavaScriptParent both inherit from both of these classes. That way, all the functionality exists on both sides. The only stuff that's specialized is the tracing and finalization code, which works differently in each.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 4 • 11 years ago

This patch makes it possible to have multiple GC FinalizeCallbacks attached to a single runtime. We'll need that later on in the patch stack.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 5 • 11 years ago

We have a couple static methods that manage CPOWs. They're attached to JSParent right now, but pretty soon the distinction between JSParent and JSChild will mostly disappear. This patch just makes them functions.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 6 • 11 years ago

We have two map types that map from JSObject* to CPOW id and from CPOW id to JSObject*. This patch renames them to ObjectToIdMap and IdToObjectMap to clarify their purpose.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 7 • 11 years ago

We have a single function, findObject, that looks up "objects" based on their ID. In JSParent, it returns CPOWs. In JSChild, it returns normal JSObjects that are referenced by CPOWs. In the future, both sides will need both kinds of mappings. This patch renames findObject to findProxyById (for the JSParent-side usecase) and findObjectById (for the JSChild-side usecase). Eventually we'll have two separate maps that implement this functionality, but this patch just distinguishes the callsites and still uses the same map for both.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 8 • 11 years ago

Most conversion functions in JavaScript shared have names of the form toX/fromX. The exceptions are toVariant/toValue. This patch renames toValue to fromVariant.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 9 • 11 years ago

For some reason, the functions that convert between object IDs and JSObject*s are called makeId and unwrap. This patch renames them to toId and fromId.

Note that these functions still have different functionality in the parent and the child, so they're virtual. I'll fix that later on.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 10 • 11 years ago

This patch creates a separate IdToObjectMap proxies_. It's used by findProxyById. This way, findObjectById and findProxyById now use separate maps.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 11 • 11 years ago

This patch moves all the CPOW-related code from JSParent to a WrapperOwner class. Most of the patch is just code movement. The tricky part is how the IPC works. The CPOW code needs to call functions like CallGetProperty, which are generated by the IPDL codegen and belong to PJavaScriptParent. Eventually, I'm going to shift the IPDL code so that messages like GetProperty are marked "both:" rather than just "child:". Unfortunately, that means that PJavaScriptParent and PJavaScriptChild will have completely separate implementations of CallGetProperty. Somehow, WrapperOwner needs to know which one is the right one to call.

I sorted this out using virtual methods and some templating. Hopefully it's not too ugly. Please let me know if you can think of a better way.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 12 • 11 years ago

This patch moves the JSChild code to a different class, WrapperAnswer. It's pretty similar to the previous patch. It has the additional problem that I want JavaScriptBase to inherit from both WrapperAnswer and WrapperOwner. However, both of these inherit from JavaScriptShared. To solve this, I used virtual inheritance. It actually works kind of okay.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 13 • 11 years ago

This patch is more cleanup. Rather than pass raw uint64_ts around to denote object IDs, I created an ObjectVariant IPDL type. It's just a struct holding a uint64_t.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 14 • 11 years ago

This patch makes it possible to refer to two kinds of objects over IPDL. When you send a message, you can talk about "local" objects or "remote" objects. These refer to objects that are local and remote *to the receiver*. So if the parent sends a message to the child referring to a local object, it means an object that lives in the child. If it refers to a remote object, it means a CPOW in the child that refers to a parent object.

I also renamed toId/fromId to toObjectVariant/fromObjectVariant.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 15 • 11 years ago

This patch moves all the tables and memory management stuff so that it lives in JavaScriptShared. That way, both the parent and the child can manage CPOWs.

I also added trace methods to both JSParent and JSChild. This patch and the next patch make it so that all cross-process pointers are strong. A later patch will make parent->child pointers weak.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 16 • 11 years ago

This patch moves toObjectVariant/fromObjectVariant to WrapperOwner, where it is shared between JSParent and JSChild. This patch actually permits the child to have CPOWs, since it now has a way to create them and send them over the wire.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 17 • 11 years ago

This patch makes parent->child CPOWs use weak references.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 18 • 11 years ago

One problem with weak references is that it forces the child to handle the case where the parent references an object ID of an object that was just GCed. In that case, we want to have the parent throw an exception (similar to using a nuked object). This patch makes that happen (before we just crashed intentionally).

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 19 • 11 years ago

This patch is a little unrelated. I was having problems where the parent would get two different CPOWs for the same object. The problem was that we would send up a CPOW for the object, and then later send up a CPOW for the same object, but wrapped in a different compartment. I talked about this a little with Bobby, and he liked the idea of using the TabChildGlobal as a canonical "home" for CPOWs. So we always try to wrap the object in that global (if it belongs to a tab at all) before sending it up as a CPOW.

This isn't the greatest solution, and I guess it doesn't work for parent objects. I don't expect that to be as much of a problem though, but maybe I'm wrong.

The other possibility is to unwrap the object before sending it to the parent. However, I like the fact that CPOWs are always wrapping xrays. It seems a lot safer that way. Bobby and I also talked about implementing the xray stuff in the parent, but that would be a lot more work and I'm not entirely sure it's the right way to go.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 20 • 11 years ago

This patch adds some new tests for bidirectional CPOWs and for the lifetime issues.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 21 • 11 years ago

This is a folded patch, based on top of e2db1c06a933.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 22 • 11 years ago

Comment on attachment 8409225 [details] [diff] [review]
finalize-callback

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

Great, looks like a good tidyup too.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 23 • 11 years ago

Comment on attachment 8409228 [details] [diff] [review]
rename-find-object

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

::: js/ipc/JavaScriptParent.cpp
@@ +42,5 @@
>      Value v = GetProxyExtra(obj, 1);
>      MOZ_ASSERT(v.isDouble());
>  
>      ObjectId objId = BitwiseCast<uint64_t>(v.toDouble());
> +    MOZ_ASSERT(findProxyById(objId) == obj);

Just a note that we decided to replace Proxy with CPOW.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 24 • 11 years ago

Comment on attachment 8409239 [details] [diff] [review]
move-wrapper-code

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

Per our in-person meeting, virtual functions were the way to go.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 25 • 11 years ago

Comment on attachment 8409251 [details] [diff] [review]
tables-both-sides

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

::: js/ipc/JavaScriptChild.cpp
@@ +60,5 @@
> +    ObjectId objId = idOf(obj);
> +
> +    proxies_.remove(objId);
> +    if (active() && !SendDropObject(objId))
> +        (void)0;

if (active())
    SendDropObject(objId);
?

::: js/ipc/JavaScriptShared.cpp
@@ +153,5 @@
> +JavaScriptShared::decref()
> +{
> +    refcount_--;
> +    if (!refcount_)
> +        delete this;

Should we stabilize the refcount during destruction?

::: js/src/jsapi.cpp
@@ -1597,5 @@
>  
>  JS_PUBLIC_API(void)
>  JS_RemoveExtraGCRootsTracer(JSRuntime *rt, JSTraceDataOp traceOp, void *data)
>  {
> -    AssertHeapIsIdle(rt);

Why remove this? Could we assert idle or collecting? It seems like it'd be bad to remove a roots tracer during tracing?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 26 • 11 years ago

Comment on attachment 8409257 [details] [diff] [review]
tabchild-owns-cpows

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

We should also make sure we either preserve DOM wrappers or make sure that we don't have to.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 27 • 11 years ago

Comment on attachment 8409257 [details] [diff] [review]
tabchild-owns-cpows

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

(Or investigate using the junk scope here)

	Blake Kaplan (:mrbkap) (inactive)
	Comment 28 • 11 years ago

Comment on attachment 8409258 [details] [diff] [review]
new-tests

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

::: content/base/test/chrome/cpows_child.js
@@ +8,3 @@
>  (function start() {
> +    [is_remote] = sendSyncMessage("cpows:is_remote");
> +    dump("IS REMOTE: " + is_remote + "\n");

Should probably nuke all of the dumps in these tests.

@@ +15,5 @@
>      nested_sync_test();
>      // The sync-ness of this call is important, because otherwise
>      // we tear down the child's document while we are
>      // still in the async test in the parent.
> +    lifetime_test(function() {

Please comment here and below that these two tests "race" to be the final test.

::: content/base/test/chrome/cpows_parent.xul
@@ +172,5 @@
> +        savedWilldieObj.f;
> +      } catch (e) {
> +        threw = true;
> +      }
> +      ok(threw, "limited-lifetime CPOW stopped working");

Worth checking to make sure we got the right exception?

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 29 • 11 years ago

I ran into some further problems with reachability. Blake already pointed out that we need to call PreserveWrapper on anything we create a CPOW to. However, there's a further problem, which is that we have to avoid creating CPOWs to CCWs. It's possible for the CCW to be collected while the target is still alive. In that case, using the CPOW would report a dead object, even though it's still alive.

So I settled on a setup where we always unwrap an object before creating a CPOW to it. However, when handling a CPOW request, we wrap all the objects we're operating on in either the TabChildGlobal or the junk scope. That way we always use Xrays.

I also added a PreserveWrapper call when creating a CPOW to an object. Unfortunately, I don't see how to do PreserveWrapper on XPCWrappedNatives. So right now it just works on DOM objects. We really need it for WNs though, so I'm hoping you can tell me how to fix this, Blake :-).

I'll write a test for the reachability behavior once we figure out this PreserveWrapper issue.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 30 • 11 years ago

Just talked to Bobby and apparently we have no way to PreserveWrapper a WN. The one place where I saw we needed this so far was an nsSHistory object. That one is pretty easy to work around. I'll try to see if there are any other places; it's hard to tell how big of an issue this might be.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 31 • 11 years ago

One idea to fix this generally is to handle WNs differently than other objects. They would be stored in the local object map using a strong reference but they would always be cleared out at the end of each event loop turn. That would at least give more predictable behavior.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 32 • 11 years ago

Hi Felipe. Just as some background, the main goal of this patch is to allow CPOWs to point both from the parent to the child and from the child to the parent. In order to avoid problems with uncollectable cycles between the two, we've decided to make the parent->child references be weak. That turns out to cause a few issues with our use of CPOWs in the browser. Specifically, our CPOWs are getting collected too soon.

There were two problems. The first one is with the CPOW handler we use for getting the focus element. Normally it should live as long as the content script it lives in, so there should be no problem. However, it turns out the name "SyncHandler" is used in two places: 

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#14
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#172

Because bug 673569 still isn't fixed, one overwrites the other and so the focus handler gets collected before we expect it to. I've fixed the problem by renaming the object.

The other problem is with a CPOW to an nsSHistory object. Normally we would expect that object to stay alive because it's pointed to by a live docshell. However, we're *actually* creating a CPOW to the JS wrapper around the nsSHistory object. The JS object is allowed to die before the nsSHistory object dies. (We normally have a scheme called "wrapper preservation" that can prevent the JS object from dying before the nsSHistory object, but that doesn't work here because nsSHistory is an old-fashioned XPCOM object that apparently no one cares about. For the same reason, you can't use an nsSHistory object as a key in a WeakMap.) Anyway, I fixed this problem by strongly rooting the JS object from the content script.

The latter problem might be an issue for add-ons, but maybe not. I wouldn't expect add-ons to use too many old-fashioned XPCOM objects in the content process. Usually they'll just be using regular DOM objects, which don't have the wrapper preservation problem. If it does turn out to be an issue, I think we can probably work around it on a case by case basis.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 33 • 11 years ago

> Should we stabilize the refcount during destruction?

I forgot to ask: what does this mean?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 34 • 11 years ago

Comment on attachment 8419842 [details] [diff] [review]
tabchild-owns-cpows v2

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

::: js/ipc/JavaScriptShared.cpp
@@ +366,5 @@
> +        return nullptr;
> +    }
> +
> +    // Objects are stored in objects_ unwrapped. We want to wrap the object
> +    // before returning it so that all operations happen on Xray wrappers. If

Do we have tests anywhere that we actually do get Xray behavior for CPOWs? I can't find any.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 35 • 11 years ago

This tests the preserve wrapper behavior for DOM objects as well as the Xray behavior. I verified that disabling the preserve wrapper code and the extra TabChildGlobal/junk scope wrapping behavior makes these tests fail.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 36 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=4f0a7452db01

	Phil Ringnalda (:philor)
	Comment 37 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/cb9571f1400b
https://hg.mozilla.org/mozilla-central/rev/48a84f76656d
https://hg.mozilla.org/mozilla-central/rev/737d5beb60c4
https://hg.mozilla.org/mozilla-central/rev/bb17661ca3bc
https://hg.mozilla.org/mozilla-central/rev/9631faa6ed67
https://hg.mozilla.org/mozilla-central/rev/4b7c1b82d2aa
https://hg.mozilla.org/mozilla-central/rev/85e6824afa65
https://hg.mozilla.org/mozilla-central/rev/15ecb06251c8
https://hg.mozilla.org/mozilla-central/rev/05fa4a69cb98
https://hg.mozilla.org/mozilla-central/rev/1e495c6dba19
https://hg.mozilla.org/mozilla-central/rev/990ea6bcfa07
https://hg.mozilla.org/mozilla-central/rev/9bc4f004e7f0
https://hg.mozilla.org/mozilla-central/rev/9055e91c6154
https://hg.mozilla.org/mozilla-central/rev/a141144b6c99
https://hg.mozilla.org/mozilla-central/rev/4f0a7452db01