ecma402
ecma402 copied to clipboard
tc39
Intl.DateTimeFormat calls getters on options objects twice
We have recently been notified by our friends at Google that the following code crashes Chakra:
const options = {};
Object.defineProperty(options, "year", {
get() {
Object.defineProperty(options, "year", {
get() {
return undefined;
},
configurable: true
});
return "numeric";
},
configurable: true
});
print(new Intl.DateTimeFormat("en-US", options).format());
The problem here is that by spec, we call Get(options, "year") twice -- once in ToDateTimeOptions and another time in step 22.b of InitializeDateTimeFormat. As a result, we get "numeric" on the first call and undefined on the second, which is tripping us up. V8 and SpiderMonkey both print the empty string here, since they are most likely generating empty pattern strings when they format the date according to the UDatePatternGenerator. While Chakra could fall back on that, the fact that we call the getter twice seems like a possible spec bug.
As a workaround, in ToDateTimeOptions, we already recreate the options object as options = ObjectCreate(options) early on. Could we change the spec text to CreateDataPropertyOrThrow in the !needsDefaults case for each property we find? That way, when we get properties off the options object, we will be getting safe ones defined in ToDateTimeOptions and not the possibly-mutated ones provided to us.
thank you so much for reporting that! it does ineed seem like a spec suboptimal behavior which should be optimized away.
Could we change the spec text to CreateDataPropertyOrThrow in the !needsDefaults case for each property we find?
I don't understand how this'll prevent the assertion error in Chakra. Can you elaborate?
For example even if CreateDataPropertyOrThrow is used in ToDateTimeOptions, the following test case should still assert with Failure: (*returnLength > 0) in Chakra.
const options = {};
Object.defineProperty(options, "timeZone", {
get() {
// |this| is not |options|, but a new object inheriting from |options|.
for (var p of Object.getOwnPropertyNames(this))
delete this[p];
}
});
print(new Intl.DateTimeFormat("en-US", options).format());
(Chakra definitely has work to do here, not disputing that at all :) I just want to try to fix this once instead of multiple times )
In that scenario, perhaps we could create a new options object/record entirely instead of setting the provided one as the prototype? Then the iteration happening in step 22.b would never call getters of the original object.
Hmm, I don't see an easy way to untangle this, because ToDateTimeOptions is also used in Date.prototype.toLocale[Date,Time]String() to pre-process the options argument. Short of changing Date.prototype.toLocale[Date,Time]String() to skip the constructor call and instead directly call into InitializeDateTimeFormat plus some additional changes for InitializeDateTimeFormat, for example passing the "required" and "defaults" arguments to InitializeDateTimeFormat etc. etc. Bah, this is probably too vague to be understandable, let's create a rough sketch → https://github.com/anba/ecma402/commit/82f9cb68e8c119e2ea90cabe5e7e4c8a5b853d05
I just want to try to fix this once instead of multiple times
I want to suggest that we actually hold off on these fixes in favor of a larger change to move towards WebIDL for these sorts of conversions. That will be another observable change, but it will "pay for itself" by creating a cleaner separation between conversions and behavior, as well as consistency with Web APIs.
@ljharb The idea would be to eventually move both. I don't have an opinion on ordering, and didn't mean to imply that they wouldn't happen together; however, this is a huge work item so I won't be able to do either immediately.
this fix is user-observable, so it seems like it’d make sense to prioritize this change over an unobservable spec rewrite.
@ljharb The problem is that the upgrade to WebIDL will likely also be observable. I'd rather avoid two observable spec changes.
If we can’t upgrade to web IDL without observable changes, that’s a pretty big argument against doing it.
Either way, i think this is off topic for this issue, and i don’t think the two should be coupled.
@ljharb I think switching to WebIDL would be a comprehensive way to fix all of these sorts of issues. It would result in a fixed, shared pattern for how all of the options are accessed. The change would be that first, all the Get calls happen, and then all the data is used. This would change the ordering of a number of exceptions, for one. I think it will be worth it, as we've had a number of issues relating to things being observable in options processing, and this would solve them all.
Another example from duplicate ticket #663:
As a result [of property values read by ToDateTimeOptions not being captured], the value of a property can change between Get in ToDateTimeOptions and the Get in InitializeDateTimeFormat, with surprising results such as empty output when the former overrides needsDefaults but the latter wouldn't:
const formatter = new Intl.DateTimeFormat(undefined, { _getCount: 0, get dateStyle(){ return this._getCount++ < 1 ? "invalid" : undefined }, }); formatter.format(new Date("2022-03-21")); // => ""
