Closed
Bug 861219
Opened 12 years ago
Closed 10 years ago
Date.prototype should be a non-Date Object
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: erights, Assigned: arai)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 6 obsolete files)
14.74 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
21.83 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•11 years ago
|
||
Status?
Comment 2•11 years ago
|
||
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).
Flags: needinfo?(till)
Comment 3•11 years ago
|
||
(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.
Flags: needinfo?(till)
Comment 4•11 years ago
|
||
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
Attachment #8428245 -
Flags: review?(jorendorff)
Updated•11 years ago
|
Assignee: general → till
Status: NEW → ASSIGNED
Comment 5•11 years ago
|
||
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?
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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.
Attachment #8428245 -
Flags: review?(jorendorff) → review-
Comment 8•11 years ago
|
||
(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.
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 9•11 years ago
|
||
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
Comment 10•10 years ago
|
||
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
Attachment #8502714 -
Flags: feedback?(bobbyholley)
Updated•10 years ago
|
Attachment #8428245 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
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.
Attachment #8502714 -
Flags: feedback?(bobbyholley) → feedback+
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee: till → arai.unmht
Attachment #8502714 -
Attachment is obsolete: true
Attachment #8602123 -
Flags: review?(bobbyholley)
Comment 13•10 years ago
|
||
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...)
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
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.
Attachment #8602123 -
Flags: review?(bobbyholley)
Comment 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
Updated Date.prototype.toString as a generic function.
Almost green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d39634ce4c28
Attachment #8602123 -
Attachment is obsolete: true
Attachment #8602897 -
Flags: review?(bobbyholley)
Comment 19•10 years ago
|
||
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.
Attachment #8602897 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 20•10 years ago
|
||
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
> }
Flags: needinfo?(bobbyholley)
Comment 21•10 years ago
|
||
(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?
Flags: needinfo?(bobbyholley)
Comment 22•10 years ago
|
||
(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).
Assignee | ||
Comment 23•10 years ago
|
||
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.
Attachment #8603701 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 24•10 years ago
|
||
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
Attachment #8602897 -
Attachment is obsolete: true
Attachment #8603702 -
Flags: feedback?(bgrinstead)
Comment 25•10 years ago
|
||
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.
Attachment #8603701 -
Flags: review?(bobbyholley) → review+
Comment 26•10 years ago
|
||
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.
Attachment #8603702 -
Flags: review+
Comment 27•10 years ago
|
||
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 {.*}/
Attachment #8603702 -
Flags: feedback?(bgrinstead) → feedback+
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
sorry had to back this out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9776445&repo=mozilla-inbound
Comment 30•10 years ago
|
||
Assignee | ||
Comment 31•10 years ago
|
||
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)
Attachment #8603701 -
Attachment is obsolete: true
Attachment #8605258 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 32•10 years ago
|
||
Just changed the definition of DateObject::protoClass_ to set ClassSpec::IsDelegated flag.
Attachment #8603702 -
Attachment is obsolete: true
Attachment #8605259 -
Flags: review?(bobbyholley)
Comment 33•10 years ago
|
||
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?
Attachment #8605258 -
Flags: review?(bobbyholley) → review+
Comment 34•10 years ago
|
||
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?
Attachment #8605259 -
Flags: review?(bobbyholley) → review+
Comment 35•10 years ago
|
||
(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?
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 36•10 years ago
|
||
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.
Flags: needinfo?(arai.unmht)
Comment 37•10 years ago
|
||
Ah, makes sense - thanks for the detailed explanation!
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d3ad5584dc91
https://hg.mozilla.org/mozilla-central/rev/37b8a0496394
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 40•10 years ago
|
||
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
Comment 41•10 years ago
|
||
Thanks for the doc updates, :arai! Very nicely done!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•