1085566 - Make the preventExtensions hook return succeeded/failed rather than always indicate failure by reporting an error

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

This'll be necessary to implement Reflect.preventExtensions, eventually, and the current signature is misleading people as to how this hook should work.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8508143 [details] [diff] [review]
Patch

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

r=me with return value changed. Glad to see the stupidity in the SDPH method go away, and us get closer to spec.

::: dom/base/nsGlobalWindow.cpp
@@ +727,1 @@
>    return false;

This should now return true, no? This doesn't actually throw, just "recommends throwing to the caller", right? As written, this is likely to assert on a JS_ASSERT_IF(!ok, cx->pendingException) or some such, I would think

Comment 3

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e900e87b4bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f3364485a1

Second patch is a test for this.  It's mostly invisible, but if you try to do Object.preventExtensions on a cross-compartment wrapper to a proxy that sets the outparam to false but doesn't return false, the compartment of the throw exception differs, pre- and post-patch.

There are a few other little flourishes I want to do to the preventExtensions hooks (mostly make the ones that should flat-out throw, that I didn't think to make do it here, do so again).  I'll file a new bug with that patch.

Comment 4

[Carsten Book [:Tomcat]]

Hi Waldo,

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=accd28c1aaa3 since one of this changes/bugs caused perma failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3198627&repo=mozilla-inbound

Comment 5

[Jeff Walden [:Waldo]]

My suspicion is that IPC code requires outparams to be initialized even for failure cases.  Retrying:

https://tbpl.mozilla.org/?tree=Try&rev=cea02b701e36

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1f3364485a1

Comment 7

[Jeff Walden [:Waldo]]

Relanded the backed-out part, with the missing |*succeeded = false;| (or true, doesn't matter as long as it's one of the two :-\ ) added to WrapperAnswer::RecvPreventExtensions:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfc7cabc457

(The bit in comment 6 is a test that landed separately from this, that doesn't run on main tinderbox displays and so wasn't backed out when this was backed out.  ^-^ )

https://tbpl.mozilla.org/?tree=Try&rev=c179dacb0844 is a try run that was green wrt the one failure mentioned as responsible for the backout.  The dt oranges there are from the pe-fup.diff patch there, that was *not* landed at this time.  (Those are the "few other little flourishes" from comment 3, destined for another bug.)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bfc7cabc457

Comment 9

[Jeff Walden [:Waldo]]

Updated:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/38
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_PreventExtensions (which apparently didn't exist before, bad us!)
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference