1377368 - ProxyObject::New won't create foreground finalized proxies in the nursery

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

As brought up in bug 1377055, ProxyObject::New tenures proxy objects if |!handler->finalizeInBackground(priv)|.  This doesn't really make sense: finalizers will not be called for nursery objects regardless of whether the finalizer should run in the foreground or background.

This was added when we enabled nursery allocation of cross compartment wrappers in bug 1237058.  I originally thought it might be something to do with ensuring the proxy didn't have a finalizer, but actually all proxies have finalizers.  If we are to allocate proxy objects in the nursery finalization must be handled by some other means.

I think it's fine to remove this check and rely on the |handler->canNurseryAllocate()| check alone.

This is blocking allocating DOM proxies in the nursery.

A try push here was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93910fbf53242dcb9d6e59f28ab36880d583ea4&group_state=expanded

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1377368-proxy-nursery-alloc

Steve, what do you think?

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8882473 [details] [diff] [review]
bug1377368-proxy-nursery-alloc

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

::: js/src/vm/ProxyObject.cpp
@@ -72,5 @@
>          MOZ_ASSERT(priv.isNull() || (priv.isGCThing() && priv.toGCThing()->isTenured()));
>          newKind = SingletonObject;
>      } else if ((priv.isGCThing() && priv.toGCThing()->isTenured()) ||
> -               !handler->canNurseryAllocate() ||
> -               !handler->finalizeInBackground(priv))

At first look, this just seemed like it should have been a MOZ_ASSERT:

  MOZ_ASSERT(!handler->finalizeInBackground(priv), "nursery finalization only implemented for foreground-finalized wrappers"); // remove this if you implement nursery finalization for background-finalized things

...which we would now be removing. But I think this is about:

bool
Wrapper::finalizeInBackground(const Value& priv) const
{
    if (!priv.isObject())
        return true;

    /*
     * Make the 'background-finalized-ness' of the wrapper the same as the
     * wrapped object, to allow transplanting between them.
     *
     * If the wrapped object is in the nursery then we know it doesn't have a
     * finalizer, and so background finalization is ok.
     */
    if (IsInsideNursery(&priv.toObject()))
        return true;
    return IsBackgroundFinalized(priv.toObject().asTenured().getAllocKind());
}

So these assumptions no longer hold. I need to think about it more, but I'll discuss it IRL.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> But I think this is about:

Note bug 1377190 from yesterday.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1377368-proxy-nursery-alloc v2

Ah right, so we need to update that to make that work with nursery allocated wrappees which may end up being finalized in the foreground.

Comment 5

[Jon Coppeard (:jonco)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd1d5238ceaeec3a31e3f7b2bd9399f6ab9b55b8

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9ed22b1a51
Support allocating foreground finalized proxies in the nursery r=sfink

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6a9ed22b1a51