975277 - Miscellaneous dependencies for Date object Xrays

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I have a handful of patches for bug 975042 that can be reviewed by various people and land separately. Splitting them out into a separate bug.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v3 r=bholley

This patch was originally written by peterv, but I made some changes to it. I
think efaust can review.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Pass the real receiver when using HasPrototype. v1 r=bholley

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rewrite Proxy::set logic. v1

The current logic ends up invoking BaseProxyHandler::set in various cases that
will cause it to invoke handler->getPropertyDescriptor, which is verboten for
mHasPrototype proxies.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Clean up the XPCWN XrayHolder a bit. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Move the wrappedJSObject getter out of getPropertyDescriptor. v1

The current setup is kinda wrong, and doesn't work with HasPrototype Xrays.
This change requires us to manually munge the holder, but that's probably ok
for now.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Introduce a mechanism to identify instances of standard classes. v1

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8379496 [details] [diff] [review]
Part 6 - Introduce a mechanism to identify instances of standard classes. v1

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

Unless you have planned used cases that actually depend on a dynamic value of IdentifyKind (I couldn't see any in this patch), I think it'd be cleaner to just have three different JSAPIs, one for each IdentifyKind.  This code duplication looks minimal and it'll make it easier to read.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8379493 [details] [diff] [review]
Part 3 - Rewrite Proxy::set logic. v1

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

This is much cleaner. r=me

::: js/src/jsproxy.cpp
@@ +2550,5 @@
>      AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true);
>      if (!policy.allowed())
>          return policy.returnValue();
> +
> +    // If we don't have a prototype, we can sink to the |set| trap.

Can we change the wording of "don't have a prototype"? This flag is more or less unrelated to the value returned by JSObject::getProto() on the proxy, as I understand it.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Introduce a mechanism to identify instances of standard classes. v2

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8379890 [details] [diff] [review]
Part 6 - Introduce a mechanism to identify instances of standard classes. v2

Great, thanks.  Can you inject "StandardClass" into each of those names (e.g., IdentifyStandardClassInstance)?

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8379491 [details] [diff] [review]
Part 1 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v3 r=bholley

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

r=me. I hadn't seen EnableIf before. That's amazingly cool.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1876,5 @@
>      RootedObject expando(cx, Traits::singleton.getExpandoObject(cx, target, wrapper));
>  
> +    // We want to keep the Xray's prototype distinct from that of content, but
> +    // only if there's been a set. If there's not an expando, or the expando
> +    // slot is |undefined|, hand back content's proto, appropriately wrapped.

"hand back content's proto" is not quite true in the case where hasPrototype is set.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8379494 [details] [diff] [review]
Part 4 - Clean up the XPCWN XrayHolder a bit. v1

Looks like this doesn't compile on MSVC.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 4 - Clean up the XPCWN XrayHolder a bit. v2

This should do it.

Comment 14

[Bobby Holley (:bholley)]

Thanks for the reviews everyone.

All-platform try push:
https://tbpl.mozilla.org/?tree=Try&rev=27d44fcc0ff9

Comment 15

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89a7246d2bdd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65192746bdee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5db44a9eece2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd04a8b8ccc4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/02dffb9d2748
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/84904662e2d5

Comment 16

[Bobby Holley (:bholley)]

Followup bustage fix for the removal of shortid:

https://hg.mozilla.org/integration/mozilla-inbound/rev/924690f9d81b

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/89a7246d2bdd
https://hg.mozilla.org/mozilla-central/rev/65192746bdee
https://hg.mozilla.org/mozilla-central/rev/5db44a9eece2
https://hg.mozilla.org/mozilla-central/rev/fd04a8b8ccc4
https://hg.mozilla.org/mozilla-central/rev/02dffb9d2748
https://hg.mozilla.org/mozilla-central/rev/84904662e2d5
https://hg.mozilla.org/mozilla-central/rev/924690f9d81b