The ES6 spec now clearly states that Date.prototype should be (in ES5 terms) of [[Class]] "Object" rather than [[Class]] "Date" and should not have the internal properties expected of Date instances. Doing so would also remove the overhead from the SES-on-FF implementation imposed by http://code.google.com/p/google-caja/issues/detail?id=1362 See also bug 656828

Status?

Till, can you explain why ClassSpecs make this more difficult? Naively, you'd just need to replace the GenericCreatePrototype<&DateObject::class_> with a custom function (similar to the ones created in part 7 of bug 992958).

(In reply to Bobby Holley (:bholley) from comment #2) > Till, can you explain why ClassSpecs make this more difficult? Naively, > you'd just need to replace the GenericCreatePrototype<&DateObject::class_> > with a custom function (similar to the ones created in part 7 of bug 992958). It doesn't actually make it difficult, it just threw me off, particularly in combination with an assert in GlobalObject->createBlankPrototype. I'll attach a patch that works almost completely, with one test failing in a way that I don't understand.

This passes all tests except for two that I changed because they didn't make sense anymore and one that fails without the patch applied, too (tests/js1_8_5/extensions/findReferences-02.js). Jason, I think we can land this and keep it in Nightly for a few days to see if problems show up. If this try server run turns up green, that is: https://tbpl.mozilla.org/?tree=Try&rev=c779e44267c7

Comment on attachment 8428245 [details] [diff] [review] Make Date.prototype not be a Date object Review of attachment 8428245 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +3011,5 @@ > return true; > } > > +static JSObject * > +GetJSObjectClass(JSContext *cx, JSProtoKey key) Please call this CreateDatePrototype, so that the naming aligns with that from bug 992958. @@ +3013,5 @@ > > +static JSObject * > +GetJSObjectClass(JSContext *cx, JSProtoKey key) > +{ > + return cx->global()->createBlankPrototype(cx, &JSObject::class_); Add a comment indicating the relevant part of the spec?

Second try run with silly mistake fixed still causes errors in some devtools tests: https://tbpl.mozilla.org/?tree=Try&rev=489bb2b8f8b4 I don't understand what those tests do and haven't yet been successful in getting a devtools peer to explain them to me. Will update the patch once I do. Leaving the r? because I'm pretty sure that this will just require some test changes.

Comment on attachment 8428245 [details] [diff] [review] Make Date.prototype not be a Date object (In reply to Till Schneidereit [:till] from comment #6) > Second try run with silly mistake fixed still causes errors in some devtools > tests: > https://tbpl.mozilla.org/?tree=Try&rev=489bb2b8f8b4 Oh, those xrayToJS failures are real. The issue is that XrayToJS needs to be able to pull the proper JSProtoKey and ClassSpec off of both instances _and_ prototypes. So the proto will just need a custom JSClass here. I'm doing something similar for the typed array instance/proto split in bug 987163.

(In reply to Bobby Holley (:bholley) from comment #7) > Oh, those xrayToJS failures are real. The issue is that XrayToJS needs to be > able to pull the proper JSProtoKey and ClassSpec off of both instances _and_ > prototypes. So the proto will just need a custom JSClass here. I'm doing > something similar for the typed array instance/proto split in bug 987163. Ah, thanks for weighing in - I was quite puzzled by these failures. I'll keep an eye on bug 987163 and amend the patch here once I have a template for how to do so.

I realized that the spec currently has a problem here: `Date.prototype.toString` throws when called on `Date.prototype` itself. I doubt this is intended, and would guess that it's not web-compatible. Sent mail to es-discuss proposing a way to handle it: https://mail.mozilla.org/pipermail/es-discuss/2014-June/037630.html

This passes almost all tests[1], but there are two open questions: - What's the right behavior for test_bug809652.js? In lines 43 and 44, calling `toString` on wrapped objects is tested. That now returns `Invalid Date` instead of throwing. Should the test be changed, or should we still throw for wrapped objects? If the latter, how do I do that? - I don't really understand the devtools-related failures[2]. Are they caused by Xray stuff, too, or are they about something completely different? (In which case I'd talk to devtools people, of course.) [1] https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1e658a9e5831 [2] https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2331371&repo=try

Comment on attachment 8502714 [details] [diff] [review] Make Date.prototype not be a Date object.v2 Review of attachment 8502714 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Till Schneidereit [:till] from comment #10) > - What's the right behavior for test_bug809652.js? In lines 43 and 44, > calling `toString` on wrapped objects is tested. That now returns `Invalid > Date` instead of throwing. Should the test be changed, or should we still > throw for wrapped objects? If the latter, how do I do that? This should be fixed by continuing to use CallNonGenericMethod per my suggestion. > - I don't really understand the devtools-related failures[2]. Are they > caused by Xray stuff, too, or are they about something completely different? > (In which case I'd talk to devtools people, of course.) My guess is that it's Xray-related. You'll need to debug it, though going back to CallNonGenericMethod may just fix it. ::: js/src/jsdate.cpp @@ -2851,5 @@ > static bool > date_toString(JSContext *cx, unsigned argc, Value *vp) > { > CallArgs args = CallArgsFromVp(argc, vp); > - return CallNonGenericMethod<IsDate, date_toString_impl>(cx, args); We should still use CallNonGenericMethod here, because it does a lot of important security and transparency stuff via the nativeCall proxy hook. The correct solution is to pass IsDateOrDatePrototype, and have date_toString_impl handle both cases. @@ +3024,5 @@ > JS_PropertyStub, JS_StrictPropertyStub, 0); > } > > +const Class DateObject::protoClass_ = { > + js_Object_str, Please add a comment explaining what this is about. @@ +3045,5 @@ > + date_static_methods, > + date_methods, > + nullptr, > + FinishDateClassInit > + } Er, what? You definitely don't want the ClassSpec {} block here. Please remove it.

stealing, as suggested in IRC :) Changes are following: [browser/devtools/webconsole/test/browser_webconsole_output_05.js] Date.prototype is no more Date object, so console output is changed. [js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js] Date.prototype.toString is not generic, right? ES6 spec says that "Unless explicitly defined otherwise, the methods of the Date prototype object defined below are not generic", and I don't see any mention about generic in Date.prototype.toString. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-date.prototype.tostring So, I reverted the related changes in tests, and jsdate.cpp. [js/src/jsdate.cpp] Added IsDatePrototype and IsDateOrDatePrototype. IsDateOrDatePrototype is used by date_toString (in CallNonGenericMethod). IsDatePrototype is used by the assertion in date_toString_impl, and of course by IsDateOrDatePrototype. About DateObject::protoClass_, I think it needs ClassSpec, for 2 reasons. both of them happen in JSXrayTraits::resolveOwnProperty and JSXrayTraits::enumerateNames. 1. ClassSpec::createConstructor should have non-null value, to return true from js::ClassSpec::defined() 2. ClassSpec::prototypeProperties should have same value as DateObject::class_, to enumerate properties for Date.prototype Is there any other better way? (maybe using DateObject::class_ instead if clasp is DateObject::protoClass_ ?) [toolkit/devtools/server/actors/script.js] Date.prototype is no more Date object, so it's no more required to filter it out in the function for Date object. Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07241b1b6ff1

Actually the way Date.prototype.toString is defined, it turns out to be generic. Any object that is not a date object won't have the [[DateValue]] branding, and thus will end up in Step 3.a. I believe we could simply rewrite date_toString like this: tv = NaN if (ObjectClassIsDate(obj, ESClass_Date, cx) Unbox(obj, ..) date_format(...tv...)

Sorry, any object without the [[DateValue]] branding will pass the check in Step 2. and end up in Step 2.a., with tv set to NaN.

Comment on attachment 8602123 [details] [diff] [review] Make Date.prototype not be a Date object. So "explicitly defined" means "steps are explicitly defined"? I thought it means "explicitly defined as generic" (so, only toJSON is generic). okay, then I'll fix the patch shortly.

Actually, I might be wrong after-all! "Let O be this Date object." What does that mean? Only handle Date objects, but otherwise do what? Not so obvious, huh.

Spec bug: https://bugs.ecmascript.org/show_bug.cgi?id=4355

Updated Date.prototype.toString as a generic function. Almost green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d39634ce4c28

Comment on attachment 8602897 [details] [diff] [review] Make Date.prototype not be a Date object. Review of attachment 8602897 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/test/browser_webconsole_output_05.js @@ +63,5 @@ > > // 7 > { > input: "Date.prototype", > + output: "Object { , 49 more\u2026 }", Is this really how we want the test to be? Please check the owner of these tests to see if they object to hard-coding \u2026 etc. ::: js/src/jsdate.cpp @@ +3178,5 @@ > + nullptr, /* hasInstance */ > + nullptr, /* construct */ > + nullptr, /* trace */ > + { > + GenericCreateConstructor<DateConstructor, 7, JSFunction::FinalizeKind>, So, it looks like this trick is basically the same thing we do for TypedArray prototypes. I think it's really confusing and error-prone though, and we should get rid of it. So instead of doing this, let's do the following: * add a new flag (next to DontDefineConstructor) called "IsDelegatedClassSpec". * Move all of the ClassSpec fields other than |flags| into a struct inside a union. The other union member is a ClassSpec* pointing to the canonical/delegated ClassSpec. * Give the union an unfriendly name like u_, and make all of the fields accessible by accessors that properly traverse to the underlying delegate. Does that sounds sensible? I'm open to other ways to solve this problem, but I want to keep this stuff understandable and avoid duplication.

Thank you for reviewing :) (In reply to Bobby Holley (:bholley) from comment #19) > ::: js/src/jsdate.cpp > @@ +3178,5 @@ > > + nullptr, /* hasInstance */ > > + nullptr, /* construct */ > > + nullptr, /* trace */ > > + { > > + GenericCreateConstructor<DateConstructor, 7, JSFunction::FinalizeKind>, > > So, it looks like this trick is basically the same thing we do for > TypedArray prototypes. I think it's really confusing and error-prone though, > and we should get rid of it. > > So instead of doing this, let's do the following: > * add a new flag (next to DontDefineConstructor) called > "IsDelegatedClassSpec". > * Move all of the ClassSpec fields other than |flags| into a struct inside a > union. The other union member is a ClassSpec* pointing to the > canonical/delegated ClassSpec. > * Give the union an unfriendly name like u_, and make all of the fields > accessible by accessors that properly traverse to the underlying delegate. > > Does that sounds sensible? I'm open to other ways to solve this problem, but > I want to keep this stuff understandable and avoid duplication. I agree with that idea :) there are some issues, almost related to notation. * adding union and struct requires extra braces in initializer. e.g. { { { GenericCreateConstructor<...>, ... } } } * there seems no way to initialize second member of union (designated initializer isn't available) fortunatelly 1st member of former struct is also a pointer, so we can initialize ClassSpec* by casting it I thought those could be solved by (variadic) macro like following, > JS_CLASS_SPEC( > GenericCreateConstructor<SavedFrame::construct, 0, JSFunction::FinalizeKind>, > GenericCreatePrototype, > SavedFrame::staticFunctions, > nullptr, > SavedFrame::protoFunctions, > SavedFrame::protoAccessors, > SavedFrame::finishSavedFrameInit, > ClassSpec::DontDefineConstructor > ) > > JS_DELEGATED_CLASS_SPEC(DateObject::class_.spec) but macro interacts badly with the comma in template parameter, especially for former case, and it requires parenthesizing that member. > JS_CLASS_SPEC( > (GenericCreateConstructor<SavedFrame::construct, 0, JSFunction::FinalizeKind>), > GenericCreatePrototype, > SavedFrame::staticFunctions, > nullptr, > SavedFrame::protoFunctions, > SavedFrame::protoAccessors, > SavedFrame::finishSavedFrameInit, > ClassSpec::DontDefineConstructor > ) it could be a pitfall, and error message is hard to read, if I forgot the parentheses (of course it won't pass the compile). How do you think about putting 2 extra braces around ClassSpec initializers? Is it okay? > { > {{ > GenericCreateConstructor<SavedFrame::construct, 0, JSFunction::FinalizeKind>, > GenericCreatePrototype, > SavedFrame::staticFunctions, > nullptr, > SavedFrame::protoFunctions, > SavedFrame::protoAccessors, > SavedFrame::finishSavedFrameInit, > }}, > ClassSpec::DontDefineConstructor > }

(In reply to Tooru Fujisawa [:arai] from comment #20) > I agree with that idea :) \o/ > there are some issues, almost related to notation. > * adding union and struct requires extra braces in initializer. > e.g. { { { GenericCreateConstructor<...>, ... } } } > * there seems no way to initialize second member of union (designated > initializer isn't available) > fortunatelly 1st member of former struct is also a pointer, so we can > initialize ClassSpec* by casting it Clever idea. Let's add a static_assert to make sure that this trick continues to work. > How do you think about putting 2 extra braces around ClassSpec initializers? > Is it okay? That's certainly a decent approach. The other approach would be to skip the union and just manually cast the first field, since we're going to have to do that anyway. So it would just be. constexpr ClassObjectCreationOp DELEGATED_CLASSSPEC(ClassSpec* spec) { return reinterpret_cast<ClassObjectCreationOp>(spec); } { DELEGATED_CLASSSPEC(someOtherSpec), nullptr, nullptr, ..., ClassSpec::DelegatedSpec } Thoughts?

(We could also just tag the low bit of the pointer in DELEGATED_CLASSSPEC instead of a separate flag, which might be less error-prone).

Thanks again :) (In reply to Bobby Holley (:bholley) from comment #22) > (We could also just tag the low bit of the pointer in DELEGATED_CLASSSPEC > instead of a separate flag, which might be less error-prone). Yeah, that would be best here. Added relevant methods/functions, updated GlobalObject/Xray to call them, and changed TypedArray classes to use delegated ClassSpec. DELEGATED_CLASSSPEC cannot be a constexpr, since reinterpret_cast is not supported there.

bgrins, can you give me a feedback on the change in browser/devtools/webconsole/test/browser_webconsole_output_05.js ? the output for "Date.prototype" is now "Object { , 49 more\u2026 }". is passing it as 'output' property correct way to check the result? Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf98bdfe774

Comment on attachment 8603701 [details] [diff] [review] Part 0: Make ClassSpec be able to delegate to another ClassSpec. Review of attachment 8603701 [details] [diff] [review]: ----------------------------------------------------------------- This looks awesome, thank you! ::: js/public/Class.h @@ +453,5 @@ > const size_t JSCLASS_CACHED_PROTO_WIDTH = 6; > > struct ClassSpec > { > + // All properties except flags should be accessed through get* accessor. I think this is too error-prone. Can you postfix all of these members with a |_|? It's too bad we can't make POD members private. @@ +470,5 @@ > static const uintptr_t DontDefineConstructor = 1 << ParentKeyWidth; > > + static const uintptr_t DelegatedTag = 1; > + > + bool defined() const { return !!getCreateConstructor(); } It's a minor point, but I think it would be clearer to return |!!createConstructor_| so that we don't trigger the delegated-walking-code. If this member is non-zero, it tells us all we need to know. @@ +491,5 @@ > MOZ_ASSERT(defined()); > return !(flags & DontDefineConstructor); > } > + > + const ClassSpec* getDelegatedClassSpec() const { Naming nit: delegatedClassSpec(), since this is infallible. @@ +497,5 @@ > + return reinterpret_cast<ClassSpec*>(reinterpret_cast<uintptr_t>(createConstructor) & > + ~DelegatedTag); > + } > + > + ClassObjectCreationOp getCreateConstructor() const { I don't have a strong opinion here, but might be slightly more idiomatic to name the hook-returning getters |fooHook()| and drop the |get| on the non-hook-returning getters. The |get| prefix is supposed to mean "this may fail", which isn't totally accurate here. So this would be one way to name them - feel free to take it or keep it as it currently is - your choice. createConstructorHook() createPrototypeHook() constructorFunctions() constructorProperties() prototypeFunctions() prototypeProperties() finishInitHook() ::: js/src/vm/TypedArrayObject.cpp @@ +1830,5 @@ > // @@toStringTag) a custom class. The third requirement mandates that each > // prototype's class have the relevant typed array's cached JSProtoKey in them. > // Thus we need one class with cached prototype per kind of typed array, with a > +// delegated ClassSpec. > +#define IMPL_TYPED_ARRAY_PROTO_CLASS(typedArray,i) \ nit: space after the comma.

Comment on attachment 8603702 [details] [diff] [review] Part 1: Make Date.prototype not be a Date object. Review of attachment 8603702 [details] [diff] [review]: ----------------------------------------------------------------- ClassSpec stuff looks great, thanks.

Comment on attachment 8603702 [details] [diff] [review] Part 1: Make Date.prototype not be a Date object. Review of attachment 8603702 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/test/browser_webconsole_output_05.js @@ +63,5 @@ > > // 7 > { > input: "Date.prototype", > + output: "Object { , 49 more\u2026 }", For background, this condition was added in Bug 1077857 due to executing `Object.getPrototypeOf(new Date())` breaking the console. My feeling is that the actual number of properties (49) is unimportant for the devtools test. If so, you can change the check to use a regex so the test won't break if/when that number changes: output: /Object {.*}/

https://hg.mozilla.org/integration/mozilla-inbound/rev/3841e37b0e2f https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a4931b119c

sorry had to back this out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9776445&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce4fc16203d https://hg.mozilla.org/integration/mozilla-inbound/rev/4071627f11d3

jandem told me that lowest bit of function pointer is used as a indicator for ARM or Thumb code, on ARM. It should be better to use ClassSpec.flags field now. Switched to add ClassSpec::IsDelegated constant for flags field, and use it in ClassSpec definition and ClassSpec::delegated(). No other change. At least no regression happens: https://treeherder.mozilla.org/#/jobs?repo=try&revision=228d269e685c (M(p) is bug 1164432)

Just changed the definition of DateObject::protoClass_ to set ClassSpec::IsDelegated flag.

Comment on attachment 8605258 [details] [diff] [review] Part 0: Make ClassSpec be able to delegate to another ClassSpec. r=bholley Review of attachment 8605258 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Class.h @@ +453,5 @@ > const size_t JSCLASS_CACHED_PROTO_WIDTH = 6; > > struct ClassSpec > { > + // All properties except flags should be accessed through get* accessor. This comment should be updated since |get| isn't in the name anymore, right?

Comment on attachment 8605259 [details] [diff] [review] Part 1: Make Date.prototype not be a Date object. r=bholley Review of attachment 8605259 [details] [diff] [review]: ----------------------------------------------------------------- I only looked at the DateObject::protoClass_ stuff - I'm assuming nothing else changed?

(In reply to Tooru Fujisawa [:arai] from comment #31) > Created attachment 8605258 [details] [diff] [review] > Part 0: Make ClassSpec be able to delegate to another ClassSpec. r=bholley > > jandem told me that lowest bit of function pointer is used as a indicator > for ARM or Thumb code, on ARM. It should be better to use ClassSpec.flags > field now. Hm yeah, I'd heard that, but it's not clear to me why it's a problem, since we were masking out the bit before using it. Can you explain, for my education?

Thank you again :) (In reply to Bobby Holley (:bholley) from comment #33) > Comment on attachment 8605258 [details] [diff] [review] > Part 0: Make ClassSpec be able to delegate to another ClassSpec. r=bholley > > Review of attachment 8605258 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/public/Class.h > @@ +453,5 @@ > > const size_t JSCLASS_CACHED_PROTO_WIDTH = 6; > > > > struct ClassSpec > > { > > + // All properties except flags should be accessed through get* accessor. > > This comment should be updated since |get| isn't in the name anymore, right? Indeed. Removed 'get*'. (In reply to Bobby Holley (:bholley) from comment #34) > Comment on attachment 8605259 [details] [diff] [review] > Part 1: Make Date.prototype not be a Date object. r=bholley > > Review of attachment 8605259 [details] [diff] [review]: > ----------------------------------------------------------------- > > I only looked at the DateObject::protoClass_ stuff - I'm assuming nothing > else changed? Yes, change to Part 1 is only DateObject::protoClass_ definition. (In reply to Bobby Holley (:bholley) from comment #35) > (In reply to Tooru Fujisawa [:arai] from comment #31) > > Created attachment 8605258 [details] [diff] [review] > > Part 0: Make ClassSpec be able to delegate to another ClassSpec. r=bholley > > > > jandem told me that lowest bit of function pointer is used as a indicator > > for ARM or Thumb code, on ARM. It should be better to use ClassSpec.flags > > field now. > > Hm yeah, I'd heard that, but it's not clear to me why it's a problem, since > we were masking out the bit before using it. Can you explain, for my > education? Oh, good point. First of all, the reason of the bustage was following. We used the LSB of `ClassObjectCreationOp createConstructor_` as a tag, so, if it's 1, we assumed that it's a delegated ClassSpec. But it can be set to 1 even if we didn't set it through DELEGATED_CLASSSPEC() on ARM, if the function is Thumb code, and ClassSpec::delegated() returns wrong value in that case (so, function pointer to thumb code is treated as a delegated ClassSpec). Then, here are some test results: (1) https://treeherder.mozilla.org/#/jobs?repo=try&revision=243348eb0ffa use IsDelegated flag only (almost same as current patches) this PASSED (2) https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7e95c708414 use both DelegatedTag and IsDelegated flag use IsDelegated flag in ClassSpec::delegated() this also PASSED (3) https://treeherder.mozilla.org/#/jobs?repo=try&revision=9109a1bf0878 use both DelegatedTag and IsDelegated flag use IsDelegated flag in ClassSpec::delegated() assert ClassSpec::delegated()'s return values are same for DelegatedTag and IsDelegated flag this assertion FAILED (the tag was set, but the flag was not set) (1) and (3) tell us that at least one of ClassObjectCreationOp functions are in Thumb code, and current patch works and is safer. So, the question is that why (2) passed without crashing, I didn't notice about this, thank you for pointing that out :) Actually, most (or possibly all?) of libxul.so seems to be compiled as Thumb code, and it means performing bitwise-OR with ClassSpec::DelegatedTag does nothing, because the bit is already set, and returning that pointer from ClassSpec::createConstructorHook as function pointer is totally okay. What (2) tells is this, all ClassObjectCreationOp's are in Thumb code. It might be possible to use LSB=0 as a delegated ClassSpec on ARM, but I'm not totally sure it's safe in all case (including other archs). so I'd like to use ClassSpec.flags field instead.

Ah, makes sense - thanks for the detailed explanation!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ad5584dc91 https://hg.mozilla.org/integration/mozilla-inbound/rev/37b8a0496394

https://hg.mozilla.org/mozilla-central/rev/d3ad5584dc91 https://hg.mozilla.org/mozilla-central/rev/37b8a0496394

Updated following documents: https://developer.mozilla.org/en-US/Firefox/Releases/41 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/prototype https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toString https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla

Thanks for the doc updates, :arai! Very nicely done!