Disallow setting JSCLASS_SKIP_NURSERY_FINALIZE on proxies

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug)

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

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

We don't seem to support this at the moment.

I tried to implement this, but ran into two problems:

1)  Why does js::GetInitialHeap assert that NurseryAllocatedProxy objects don't have this flag set?

2)  In ProxyObject::New, why do we disallow nursery allocation if |handler->finalizeInBackground(priv)| returns false?  What does background finalization have to do with whether the proxy can be allocated in the nursery?  Especially in the case when JSCLASS_SKIP_NURSERY_FINALIZE is set, so the finalizer won't even be called while in the nursery?  As a local hack, I've made that condition:

               (!handler->finalizeInBackground(priv) &&
                !CanNurseryAllocateFinalizedClass(clasp))

but I'm not sure whether that's right.

Also, it's a bit odd that we have handler->canNurseryAllocate() which can be set independently of JSCLASS_SKIP_NURSERY_FINALIZE.

Note to self: proxy_Finalize does things other than calling the handler's finalize() hook.  Need to check whether the freeing of the value array it does is ok.
Er, I meant to needinfo jonco for the questions in comment 0.
Flags: needinfo?(jcoppeard)

Comment 2

11 months ago
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0)

> 1)  Why does js::GetInitialHeap assert that NurseryAllocatedProxy objects
> don't have this flag set?

Passing NurseryAllocatedProxy is a way to circumvent the default behaviour for a class with a finalizer 
and without the JSCLASS_SKIP_NURSERY_FINALIZE flag set which is that it gets allocated in the tenured heap (the last if statement in this function).  So if this flag was set you wouldn't need to use NurseryAllocatedProxy.

Comment 3

11 months ago
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0)
> 2)  In ProxyObject::New, why do we disallow nursery allocation if
> |handler->finalizeInBackground(priv)| returns false? 

I don't know why we do this.  Possibly it's something to do with making sure that the background/foreground finalization state of proxies ends up the same as that of their target, but I can't see why right now.  I'll try taking the check out and do some testing.
Flags: needinfo?(jcoppeard)

Comment 4

11 months ago
(In reply to Jon Coppeard (:jonco) from comment #3)
> I'll try taking the check out and do some testing.

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

Updated

11 months ago
Depends on: 1377368
OK, given that question 2 got spun out into bug 1377368, let's mutate this bug to remove the footgun.
Summary: Support JSCLASS_SKIP_NURSERY_FINALIZE proxies → Disallow setting JSCLASS_SKIP_NURSERY_FINALIZE on proxies
Created attachment 8882586 [details] [diff] [review]
Assert that JSCLASS_SKIP_NURSERY_FINALIZE is not used on proxies
Attachment #8882586 - Flags: review?(jcoppeard)

Comment 7

11 months ago
Comment on attachment 8882586 [details] [diff] [review]
Assert that JSCLASS_SKIP_NURSERY_FINALIZE is not used on proxies

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

Great, thanks the the patch.
Attachment #8882586 - Flags: review?(jcoppeard) → review+

Comment 8

11 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/159610c3ad34
Assert that JSCLASS_SKIP_NURSERY_FINALIZE is not used on proxies.  r=jonco

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/159610c3ad34
Status: NEW → RESOLVED
Last Resolved: 11 months 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.