Implement Xrays to the Date object

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

4.99 KB, patch
peterv
: review+
Details | Diff | Splinter Review
9.30 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.14 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.75 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.38 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.63 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.64 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.17 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.86 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.03 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
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).
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.
Depends on: 975277
All the deps here have landed. Uploading patches and flagging Peter for review.
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.
Attachment #8380094 - Flags: review?(peterv)
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.
Attachment #8380095 - Flags: review?(peterv)
Attachment #8380097 - Flags: review?(peterv)
Attachment #8380098 - Flags: review?(peterv)
We try to make this test machinery reusable for future Xrayable JS objects.
Attachment #8380103 - Flags: review?(peterv)
Attachment #8380094 - Flags: review?(peterv) → review+
Attachment #8380095 - Flags: review?(peterv) → review+
Attachment #8380096 - Flags: review?(peterv) → review+
Attachment #8380097 - Flags: review?(peterv) → review+
Attachment #8380098 - Flags: review?(peterv) → review+
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.
Attachment #8380099 - Flags: review?(peterv) → review+
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));|?
Attachment #8380100 - Flags: review?(peterv) → review+
Attachment #8380101 - Flags: review?(peterv) → review+
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!
Attachment #8380102 - Flags: review?(peterv) → review+
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 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?
Attachment #8380103 - Flags: review?(peterv) → review+
(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.
Addressed all other comments. Thanks for the reviews Peter!

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=fe3ddbc857f7
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...
And the try run in comment 21 has that starred as bug 958689, but this seems like a different error from that one.
Flags: needinfo?(bobbyholley)
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
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.
Attachment #8395144 - Flags: review?(fabrice)
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() ?
Attachment #8395144 - Flags: review?(fabrice) → review+
(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
Depends on: 1072174
You need to log in before you can comment on or make changes to this bug.