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.

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

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

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?

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.

> 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.

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

Comment on attachment 8508211 [details] [diff] [review] checked-unwrap-date No, that looks good at first glance. Thanks!

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.

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

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

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.

I can fix this real quick.

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"...

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 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 on attachment 8509310 [details] [diff] [review] Part 2 - Add some cx parameters. v1 r=me

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.

We should add tests for at least bug 857140.

(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.

(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?

Sure.

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.

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

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