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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

55 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Steve, what do you think?
Attachment #8882473 - Flags: review?(sphink)
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.
> But I think this is about:

Note bug 1377190 from yesterday.
(Assignee)

Comment 4

2 years ago
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+

Comment 6

2 years ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9ed22b1a51
Support allocating foreground finalized proxies in the nursery r=sfink

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a9ed22b1a51
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.