Closed Bug 1377368 Opened 3 years ago Closed 3 years ago

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


(Core :: JavaScript: GC, enhancement)

55 Branch
Not set



Tracking Status
firefox56 --- fixed


(Reporter: jonco, Assigned: jonco)




(1 file, 1 obsolete file)

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:
Attached patch bug1377368-proxy-nursery-alloc (obsolete) — Splinter Review
Steve, what do you think?
Attachment #8882473 - Flags: review?(sphink)
Comment on attachment 8882473 [details] [diff] [review]

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:

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.
> But I think this is about:

Note bug 1377190 from yesterday.
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.
Attachment #8882473 - Attachment is obsolete: true
Attachment #8882473 - Flags: review?(sphink)
Attachment #8882685 - Flags: review?(sphink)
Attachment #8882685 - Flags: review?(sphink) → review+
Pushed by
Support allocating foreground finalized proxies in the nursery r=sfink
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.