920484 - Intl prototype initialisation requires empty object

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[André Bargull [:anba]]

test case:
---
Object.prototype.style = "percent";
Intl.NumberFormat.prototype.format("10")
---

Expected: returns "10"
Actual: returns "1.000\xA0%"

Should be an easy fix: In InitNumberFormatClass (builtin/Intl.cpp), initialise the `options` variable in l. 1191 with a fresh empty object which does not inherit from Object.prototype, so like `Object.create(null)`. And apply the same change to InitCollatorClass and InitDateTimeFormatClass.

The code uses `undefined` as spec'ed in ECMA-402, but due to lazy instantiation this is not the right choice here.

Comment 1

[Jeff Walden [:Waldo]]

Hum, nice catch.  So the issue is that the InitializeNumberFormat call in that method (and similar in the others) passes in |undefined| per spec, but it so happens that inside the Initialize* methods that value is converted into an empty object.  That's correct in general, but here, we need an empty object as-if before any user code executed.  Ergo, putting in a new object with null prototype instead of |undefined|.  Easy first bug for someone, recommend grabbing a copy of the 1.0 spec from here to be sure you understand how things fit together:

http://wiki.ecmascript.org/doku.php?id=globalization:specification_drafts

Comment 2

[Rodrigo Rodriguez]

I'll be more than happy to work on this bug.

Comment 3

[Till Schneidereit [:till]]

Great! Feel free to ask questions here or on IRC. Review requests should go to Waldo or me.

Comment 4

[Gordon P. Hemsley [:GPHemsley]]

Mass-moving existing Intl-related bugs to the new Core :: JavaScript: Internationalization API component.

If you think this bug has been moved in error, feel free to move it back to Core :: JavaScript Engine.

[Mass change filter: core-js-intl-api-move]

Comment 5

[Mike Hoye [:mhoye]]

This one needs a new mentor. Mr. Hemsley?

Comment 6

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Mike Hoye [:mhoye] from comment #5)
> This one needs a new mentor. Mr. Hemsley?

I'm flattered, but I don't think I'm the best fit for this.

Maybe Norbert is, or knows who would be?

Comment 7

[André Bargull [:anba]]

Attachment: empty-object.patch

Simple patch, not sure if further comments are needed in the source file to explain the changes...

Comment 8

[André Bargull [:anba]]

Attachment: empty-object.patch

Now even without trailing white space... :-/

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8443984 [details] [diff] [review]
empty-object.patch

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

Hmm, why can't we change the Initialize* functions in Intl.js to use std_Object_create instead of an object literal? And aren't `Collator`, `NumberFormat` and `DateTimeFormat` affected as well? They can pass in `UndefinedValue`, too, after all. I'd definitely prefer a solution that's localized instead of careful preparation at all callsites, if possible.

Additionally, it'd be good to have tests similar to the one in comment 0 covering all initializers.

::: js/src/builtin/Intl.cpp
@@ +702,5 @@
>  
> +    RootedObject options(cx, NewObjectWithGivenProto(cx, &JSObject::class_, nullptr, cx->global()));
> +    if (!options)
> +        return false;
> +    RootedValue optionsValue(cx, ObjectValue(*options));

Given that this is repeated three times,

Comment 10

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #9)
> Hmm, why can't we change the Initialize* functions in Intl.js to use
> std_Object_create instead of an object literal?

No, because that breaks ECMA-402 compatibility, cf. 10.1.1.1 step 4 and 11.1.1.1 step 4. 12.1.1.1 works a bit different because of ToDateTimeOptions. I've only applied the same changes to InitDateTimeFormatClass for consistency, but they shouldn't actually be needed (I haven't tested that, though).


> And aren't `Collator`, `NumberFormat` and `DateTimeFormat` affected as well? They can pass in
> `UndefinedValue`, too, after all. 

They are affected in so far that adding certain properties to `Object.prototype` breaks the Intl constructors, but we cannot change that because that's what ECMA-402 specifies. 


> I'd definitely prefer a solution that's localized instead of careful preparation at all callsites, if possible.

Careful preparation is needed here, because the Intl module is lazily initialized. If no user code was permitted to run before the Intl module was initialized, these changes weren't even necessary.


> Additionally, it'd be good to have tests similar to the one in comment 0 covering all initializers.

The test case in the patch is quite similar to the test in comment #0. Both assert that the Intl prototypes do not read an option value from `Object.prototype` during initialization. The test case in the patch is just more concise since the "localeMatcher" option is read for all Intl constructors. 


So I guess further comments are necessary to explain the peculiarities of these changes and Intl prototypes initialization?! :)

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8443984 [details] [diff] [review]
empty-object.patch

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

Thanks for the explanation. With that, I agree that this is the right solution conceptually. I still don't like the code duplication, though. What do you think of adding an optional argument to IntlInitialize, along the lines of `bool createEmptyOptions = false`?

Comment 12

[Jeff Walden [:Waldo]]

I'm point man for Internationalization stuff these days, so needinfo and whatever should go to me.

IntlInitialize corresponds directly to a spec algorithm, so I'd rather not see it change that way.  If you want the steps centralized, a bool CreateDefaultOptions(MutableHandle<Value>) method seems better.

Comment 13

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> IntlInitialize corresponds directly to a spec algorithm, so I'd rather not
> see it change that way.  If you want the steps centralized, a bool
> CreateDefaultOptions(MutableHandle<Value>) method seems better.

Right, that would've been my alternative recommendation. It doesn't reduce duplication too much because you still have to have the error checking, but it at least make the pattern clear.

Comment 14

[André Bargull [:anba]]

Attachment: empty-object.patch

Is there any difference between:
  NewObjectWithGivenProto(cx, &JSObject::class_, nullptr, cx->global())
and
  NewObjectWithGivenProto(cx, &JSObject::class_, nullptr, nullptr)
?

I've seen both alternatives and I'm not sure which one to use in this patch.

Also: Multiple lines in Intl.cpp already exceed the maximum length of 100 characters, this patch will introduce two new style violations. Is this acceptable or do you insist on following the normal SpiderMonkey code styles rules?

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8444417 [details] [diff] [review]
empty-object.patch

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

The difference between nullptr and global is arcane.  At a skim (and going from a couple hours' memory ago, distracted by various other things), I think nullptr implies use the parent of the (possibly inferred!) proto.  And of course the identity of that parent is determined far distant in time, so its identity is very unclear.

That said, these days parent is so largely useless that I think it doesn't really matter usually what it is, so long as it chains to the compartment global.  Short-circuiting that chain is very good for helping us simplify and ultimately remove the concept.  So we should do that.

While 100ch mistakes in Intl.cpp should be fixed, it's not the end of the world if they exist.  A followup bug to fix them seems reasonable.  Don't much care if you add one or two more, if the whole file isn't totally right right now.

::: js/src/builtin/Intl.cpp
@@ +417,5 @@
>  
> +static bool
> +CreateDefaultOptions(JSContext *cx, MutableHandleValue defaultOptions)
> +{
> +    RootedObject options(cx, NewObjectWithGivenProto(cx, &JSObject::class_, nullptr, cx->global()));

For this totally-minimal lifetime, a raw JSObject* would also work fine.  But I tend to favor always doing the safe way, unless the unsafe thing is necessary for a demonstrated reason.  And in this case it's not necessary (run-thrice init code), nor demonstrably so.  So I prefer this (slightly, to be sure) to the raw pointer.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> that I think it doesn't really matter usually what it is

It can matter for performance, kinda, because shape includes parent, so if you end up with different parents you get different shapes.

Comment 17

[André Bargull [:anba]]

Attachment: empty-object.patch

Add correct bug number and reviewer to commit message.

Comment 18

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> While 100ch mistakes in Intl.cpp should be fixed, it's not the end of the
> world if they exist.  A followup bug to fix them seems reasonable.  Don't
> much care if you add one or two more, if the whole file isn't totally right
> right now.
> 

Created bug 1029483.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this by chance? :)

Comment 20

[Till Schneidereit [:till]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Is there a Try link handy for this by chance? :)

There is now: https://tbpl.mozilla.org/?tree=Try&rev=ad4dfe095270

André, you can either set the checkin-needed flag again once the try run looks green and complete enough, or wait for me to do it tomorrow.

Comment 21

[Till Schneidereit [:till]]

nice and green

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7834122e3262

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/7834122e3262