1377055 - Disallow setting JSCLASS_SKIP_NURSERY_FINALIZE on proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er, I meant to needinfo jonco for the questions in comment 0.

Comment 2

[Jon Coppeard (:jonco)]

(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

[Jon Coppeard (:jonco)]

(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.

Comment 4

[Jon Coppeard (:jonco)]

(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

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, given that question 2 got spun out into bug 1377368, let's mutate this bug to remove the footgun.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Assert that JSCLASS_SKIP_NURSERY_FINALIZE is not used on proxies

Comment 7

[Jon Coppeard (:jonco)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/159610c3ad34
Assert that JSCLASS_SKIP_NURSERY_FINALIZE is not used on proxies.  r=jonco

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/159610c3ad34