975042 - Implement Xrays to the Date object

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I realize that we need another bug in the dependency chain here. Date is the first step for Xrays-to-JS, but won't include all the pieces necessary for other kinds of Xrays we want to do (Arrays, Errors, etc).

This bug tracks the work needed to implement Xrays to the Date object (a 2014 Q1 DOM goal).

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Green! I'm going to split some of the patches into a separate bug, do an all-platform try push, and start uploading patches for review.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

All the deps here have landed. Uploading patches and flagging Peter for review.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove the special COW support for Date. v1

As soon as Date is on Xrays, this stuff won't work anyway. Henceforth, content
access to chrome Date objects is forbidden, and APIs should use something like
|new contentWindow.Date()| for any Date object they wish to expose to content.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Basic Xray infrastructure and boilerplate. v2

All of this machinery asserts if it actually get used. But it won't be used at
present, because we have an empty whitelist of JSProtoKeys.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement the easy trait hooks. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement createHolder. v3

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement getPrototypeOf. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make enumerateNames trap virtual. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 7 - Implement resolveOwnProperty and enumerateNames trap. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 8 - Enable JS Xray for the Date object. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 9 - Update expando sharing tests to test the Xray-to-JS case. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 10 - Tests. v1

We try to make this test machinery reusable for future Xrayable JS objects.

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 8380099 [details] [diff] [review]
Part 6 - Make enumerateNames trap virtual. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +200,1 @@
>                                 AutoIdVector &props);

Fix up the whitespace.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 8380100 [details] [diff] [review]
Part 7 - Implement resolveOwnProperty and enumerateNames trap. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +401,5 @@
> +        desc.object().set(wrapper);
> +        return true;
> +    }
> +
> +    // Grab the JSClass. We require all Xrayble classes to have a ClassSpec.

Xrayable?

@@ +476,5 @@
> +    // Non-prototypes don't have anything on them yet.
> +    if (!isPrototype(holder))
> +        return true;
> +
> +    // Grab the JSClass. We require all Xrayble classes to have a ClassSpec.

Xrayable?

@@ +503,5 @@
> +    // Add the 'constructor' property.
> +    if (!props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_CONSTRUCTOR)))
> +        return false;
> +
> +    return true;

Just |return props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_CONSTRUCTOR));|?

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 8380102 [details] [diff] [review]
Part 9 - Update expando sharing tests to test the Xray-to-JS case. v1

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

::: js/xpconnect/tests/mochitest/file_expandosharing.html
@@ -3,5 @@
>  <head>
>  <script type="application/javascript">
>    function setup() {
>      // Set up different target objects for expandos, one for each binding type.
> -    window.targetWN = document.body.firstChild;

Heh!

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8380100 [details] [diff] [review]
Part 7 - Implement resolveOwnProperty and enumerateNames trap. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +456,5 @@
> +        return false;
> +
> +    // The generic Xray machinery only defines non-own properties on the holder.
> +    // This is broken, and will be fixed at some point, but for now we need to
> +    // cache the value explicitly. See the corresponding function at the top of

Sorry, missed this: "corresponding call to JS_GetPropertyDescriptorById"?

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 8380103 [details] [diff] [review]
Part 10 - Tests. v1

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

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +155,5 @@
> +  }
> +
> +  function testDate() {
> +    // toGMTString is handled oddly in the engine. We don't bother to support
> +    // it over Xrays.

Hmm, isn't it just an alias of toUTCString? What happens that we can't support it over Xrays?

Comment 20

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #19)
> Hmm, isn't it just an alias of toUTCString? What happens that we can't
> support it over Xrays?

The basic issue is that the function isn't in the JSFunctionSpec, but copied as a special case (because ES actually requires that the function objects compare equal). It didn't seem worth paying the (miniscule) memory/gc cost to add it to the JSFunctionSpec for all scopes, nor did it seem worth adding a special case to the Xray code.

I'm totally open to do either of the above in a followup, if you think it's appropriate - toGMTString isn't even used in our tree outside of testing.

Comment 21

[Bobby Holley (:bholley)]

Addressed all other comments. Thanks for the reviews Peter!

Final all-platform try push:

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

Comment 22

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f787c0fa465e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/351371062c26
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/449ade4078c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59a38e0e27bb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac7fa583164
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c25a4bfd449
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d97dc26289
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b62c8f1c211
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/16da792be5f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/767ce92ebaf1

Comment 23

[Boris Zbarsky [:bzbarsky]]

Starting with this push, there is a permaorange on "B2G ICS Emulator Opt" M7:

282 INFO TEST-UNEXPECTED-FAIL | /tests/dom/network/tests/test_networkstats_basics.html | uncaught exception - Error: Permission denied to access property 'getTime' at http://mochi.test:8888/tests/dom/network/tests/test_networkstats_basics.html:215

Seems like it might be related to this push, since that's a Date object...

Comment 24

[Boris Zbarsky [:bzbarsky]]

And the try run in comment 21 has that starred as bug 958689, but this seems like a different error from that one.

Comment 25

[Carsten Book [:Tomcat]]

Hi bholley, sorry had to backout this checkins for the permaorange in B2g ICS Emulator like https://tbpl.mozilla.org/php/getParsedLog.php?id=36489301&tree=Mozilla-Inbound

Comment 26

[Bobby Holley (:bholley)]

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

Comment 27

[Bobby Holley (:bholley)]

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

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 11 - Create NetworkStats dates in the window's scope. v2

This stuff should really move to WebIDL. But in the mean time, this patch
should be enough to unblock this bug. Note that ES standard classes are now
resolved on Xrayed globals, so we can do |new aWindow.Date()| safely.

Comment 29

[[:fabrice] Fabrice Desré]

Comment on attachment 8395144 [details] [diff] [review]
Part 11 - Create NetworkStats dates in the window's scope. v2

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

For my own education, is that different than using Cu.createDateIn() ?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> For my own education, is that different than using Cu.createDateIn() ?

That function is obsolete now that we resolve ES constructors on Xrayed globals. :-)

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=dec3d13bd706

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c6d6fcd028fd
https://hg.mozilla.org/mozilla-central/rev/cef075dd64f8
https://hg.mozilla.org/mozilla-central/rev/8110b9082e9e
https://hg.mozilla.org/mozilla-central/rev/aaf613996e21
https://hg.mozilla.org/mozilla-central/rev/25308759dec0
https://hg.mozilla.org/mozilla-central/rev/2bbc5469a75d
https://hg.mozilla.org/mozilla-central/rev/65a69614c90d
https://hg.mozilla.org/mozilla-central/rev/7ad0ea657273
https://hg.mozilla.org/mozilla-central/rev/7bcdc31e9356
https://hg.mozilla.org/mozilla-central/rev/d554eeb756cc
https://hg.mozilla.org/mozilla-central/rev/dec3d13bd706