857138 - Give consumers a sane way to work with cross-compartment wrappers for Date objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We have a bunch of code that does JS_ObjectIsDate, then js_DateIsValid, and then things like js_DateGetMsecSinceEpoch.

I'm fixing JS_ObjectIsDate to return true for cross-compartment around Date objects, but of course js_DateIsValid and so forth still fail for them...

We should fix Date stuff to actually work even if you happen to be holding such a wrapper.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Also, this is blocking one of the DOM Q2 goals...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Though I suppose for bug 742206 I could work around in DOM code and do manual unwrapping and all that jazz...

Comment 3

[Bobby Holley (:bholley)]

Oh, I just realized that we can't use CallNonGenericMethod here because these aren't JSNatives.

The simplest thing to do is just to do a CheckedUnwrap at the beginning of these utility methods.

Luke, the basic problem here is that JS_ObjectIsDate uses ObjectClassIs, but the various utility methods here just check ->isDate(). Should we do a CheckedUnwrap, or something else?

Comment 4

[Luke Wagner [:luke]]

The last time we had this problem was for RegExps, and the result was regexp_toShared which generic extracts a RegExpShared from an object.  Is there a similar compartment-free representation of a Date that could be extracted?  On first glance, I only see uses of obj->getDataUTCTime() and GetCachedLocalTime(), so perhaps we could have a date_toLocalAndUTCTime that returns these two doubles?  I know it is pretty clunky to add a new proxy trap for each case, but (1) the number of such cases is inherently limited and (2) I like having these special cases documented in the proxy interface rather than scattered around as CheckedUnwraps.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is there a similar compartment-free representation of a Date that could be extracted?

Sure, the double inside.

How do we actually make some progress here?  This is biting people on the web, because it causes cross-iframe Date instances to fail in weird ways when used in various APIs...  I can hack around this bug on the DOM side if I have to, but it sure would be nicer if this were fixed in one central place instead of all the DOM callsites.

Comment 6

[Luke Wagner [:luke]]

Attachment: checked-unwrap-date

CheckedUnwrap seems to be what we do now for all these types of friendapi helpers.  Is this all we need here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508211 [details] [diff] [review]
checked-unwrap-date

No, that looks good at first glance.  Thanks!

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8508211 [details] [diff] [review]
checked-unwrap-date

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

Recently, CPOWs have given us a very good reason to prefer custom proxy traps over CheckedUnwrap - we can do the former cross-process, but not the latter.

I don't know how much these APIs are likely to be used with CPOWs, but I'd certainly rather we set things up so that they Just Work when they do. That has two parts:
(1) Adding a date_toTimestamp proxy trap that operates on things of ESClass_Date, and altering these API methods to use that.
(2) Adding support for the trap in CPOWProxyHandler.

If you wouldn't mind doing (1), we can file a followup bug to do (2). If you're really busy I'm OK with just landing this patch and moving on, but I'd rather do this right if we can.

Comment 9

[Luke Wagner [:luke]]

I haven't touched proxies in quite a while, so I'm probably not the right person to write that patch.

Comment 10

[Tom S [:evilpie]]

I agree with Bobby doing something like regexp_toShared seems cleaner. In this simple case Unbox would probably work as well.

Comment 11

[Bobby Holley (:bholley)]

Actually, yes - we don't even need another proxy trap - we could just piggyback on boxedValue_unbox unless that's not kosher for some reason.

Comment 12

[Bobby Holley (:bholley)]

I can fix this real quick.

Comment 13

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=22e4b525a65c

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove unused APIs. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add some cx parameters. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make Date operate with the boxedValue_unbox proxy trap. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove CheckedUnwrap in binding code. v1

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8509309 [details] [diff] [review]
Part 1 - Remove unused APIs. v1

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

::: js/src/jsdate.cpp
@@ +2663,2 @@
>              JS_snprintf(buf + (result_len - 2), (sizeof buf) - (result_len - 2),
> +                        "%d", YearFromTime(localtime));

I almost rubberstamped this on basis of "I love removing dead code"...

Comment 19

[Luke Wagner [:luke]]

Oops, the context doesn't show the "return" statement right before the JS_snprintf.  Also, could you add a testcase that would have caught this?

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8509310 [details] [diff] [review]
Part 2 - Add some cx parameters. v1

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

::: js/src/jsfriendapi.h
@@ +1282,3 @@
>  
>  extern JS_FRIEND_API(double)
> +js_DateGetMsecSinceEpoch(JSContext *cx, JSObject *obj);

If you are tweaking these signatures anyway, can you put them in namespace js?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8509310 [details] [diff] [review]
Part 2 - Add some cx parameters. v1

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8509312 [details] [diff] [review]
Part 4 - Remove CheckedUnwrap in binding code. v1

Can we no longer get a failure from js_DateGetMsecSinceEpoch if the thing is a security wrapper for a Date?

I guess not, given the previous patch; worst case we'll get 0.

Comment 23

[Boris Zbarsky [:bzbarsky]]

We should add tests for at least bug 857140.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #19)
> Also, could you add a testcase that would have caught this?

This is non-trivial because testing it requires having OS settings such that the default system date format uses 2-digit (rather than 4-digit) date strings. Figuring out how to test that case reliable is a rabbit hole I don't intend to go down.

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove unused APIs. v2

Comment 26

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> We should add tests for at least bug 857140.

Can you (or someone else) do that as part of resolving bug 857140?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Sure.

Comment 28

[Luke Wagner [:luke]]

Comment on attachment 8510153 [details] [diff] [review]
Part 1 - Remove unused APIs. v2

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

::: js/src/jsdate.cpp
@@ +2662,2 @@
>              JS_snprintf(buf + (result_len - 2), (sizeof buf) - (result_len - 2),
> +                        "%d", YearFromTime(localtime));

IIUC, in the IsNaN case, this will print 1970 (YearFromTime(0)).  The previous behavior was to print 0.  r+ assuming you do the obvious fix to preserve behavior.

Comment 29

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/afac219f612a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/907fbfcb0a62
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23f7f212a287
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0901d95848f

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/afac219f612a
https://hg.mozilla.org/mozilla-central/rev/907fbfcb0a62
https://hg.mozilla.org/mozilla-central/rev/23f7f212a287
https://hg.mozilla.org/mozilla-central/rev/c0901d95848f