Closed
Bug 1377368
Opened 7 years ago
Closed 7 years ago
ProxyObject::New won't create foreground finalized proxies in the nursery
Categories
(Core :: JavaScript: GC, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file, 1 obsolete file)
2.52 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Steve, what do you think?
Attachment #8882473 -
Flags: review?(sphink)
Comment 2•7 years ago
|
||
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•7 years ago
|
||
> But I think this is about: Note bug 1377190 from yesterday.
Assignee | ||
Comment 4•7 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)
Assignee | ||
Comment 5•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd1d5238ceaeec3a31e3f7b2bd9399f6ab9b55b8
Updated•7 years ago
|
Attachment #8882685 -
Flags: review?(sphink) → review+
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a9ed22b1a51
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•