This fixes a bug -- being able to initialize an object as an Intl.Collator, or whatever, once per global object, versus once ever. It also slightly cuts memory usage by having one WeakMap for everyone, not one per global that happens to trigger Intl stuff. This is the first use of MakeWrappable, and I will admit that it's been a few months since we discussed MakeWrappable's implementation and concept. So it's possible I'm misremembering something, or misusing it. Please double-check I'm not doing something totally stupid here!

Comment on attachment 782826 [details] [diff] [review] Patch Review of attachment 782826 [details] [diff] [review]: ----------------------------------------------------------------- After long IRC discussions have convinced me that it doesn't open any side channels (because of the TypeError on non-[[Extensible]] receivers for the initialization functions), I'm good with this. You can, if you want, simplify the code a bit, like this: IntlInternalsMap = new WeakMap(); function getInternals(o) { return IntlInternalsMap.get(o); } MakeWrappable(getInternals); function setInternals(o, internals) { IntlInternalsMap.set(o, internals); } MakeWrappable(setInternals); At least to me, that appears somewhat cleaner, but it's up to you. r=me if the try run is green.

Nope. http://www.youtube.com/watch?v=gvdf5n-zI14 Investigating.

I got as far yesterday as -- in the testOOM jsapi-test -- seeing that we were hitting the JS_CHECK_CHROME_RECURSION(cx, return false) in JSCompartment::wrap upon the isInitializedIntlObject function. That fails because GetNativeStackLimit returns 0, because cx->runtime_->nativeStackQuota == 0. So some of the fix likely involves calling JS_SetNativeStackQuota, or something. There might be more than that, of course. I'll investigate after I figure out the intermittent test failure on the Intl landing, which is definitely higher priority than this.

I wonder if we should enable higher stack and recursion limits for self-hosted code. Not sure we actually can, though. At least for code that's cloned over to user compartments, we currently don't have any means for that, I think.

The patch was also hitting this assertion -- builtin/Intl.cpp asserting the getInternals function is a function, which it isn't any more (it's a CCW), easily fixt: diff --git a/js/src/builtin/Intl.cpp b/js/src/builtin/Intl.cpp --- a/js/src/builtin/Intl.cpp +++ b/js/src/builtin/Intl.cpp @@ -465,7 +465,6 @@ GetInternals(JSContext *cx, HandleObject if (!cx->global()->getIntrinsicValue(cx, cx->names().getInternals, &getInternalsValue)) return false; JS_ASSERT(getInternalsValue.isObject()); - JS_ASSERT(getInternalsValue.toObject().is<JSFunction>()); InvokeArgs args(cx); if (!args.init(1)) As regards the stack quota bits, it's only because these two jsapi-tests don't use the default createRuntime that sets a quota. (And I guess these two tests don't try to wrap any values at all, ever? Bleh, fragile.) This is slightly messy, but it gets the job done, better than duplicating that stack quota stuff many places. Retrying with those changes now: https://tbpl.mozilla.org/?tree=Try&rev=ef0da4e27fdd There could be more stuff that needs doing, but these were the two things I saw in tbpl logs, and remembered, and could fix during The Great SF Network Failganza of July 30, 2013. :-) We Shall See.

Comment on attachment 783439 [details] [diff] [review] Set a stack quota in the two jsapi-tests that don't right now Review of attachment 783439 [details] [diff] [review]: ----------------------------------------------------------------- yes

And I guess there was more bustage that hadn't stuck out in my mind enough, to work through. Those need full browser builds, it looks like, and bug 899635 is higher priority for obvious reasons for now, so I'll get back to this after that.

Well, the stack-quota bits are doable in isolation from everything else, so I might as well land them: https://hg.mozilla.org/integration/mozilla-inbound/rev/d02216b36873 Rest of this is, of course, gated on fully resolving bug 853301, at the moment. (Unless it happens that this is needed to eliminate regressions there, but I kind of doubt it.)

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]

ECMA-402 2nd ed. changes up a bunch of the initialization stuff to play better with ES6 subclassing spec language. I'm not sure how that affects this bug, but it may end up partially voiding it. Investigation needed.

ECMA-402 2nd ed. no longer allows turning arbitrary objects into Intl-objects. From Annex B, Additions and Changes That Introduce Incompatibilities with Prior Editions: > 10.1 , 11.1 , 12.1 In ECMA-402, 1st Edition, constructors could be used to create Intl objects > from arbitrary objects. This is no longer possible in 2nd Edition. That means the current WeakMap approach is no longer needed and should be removed to comply to the new spec. Sub-classing Intl constructors now requires use of the |class| syntax.

bug 1328386 implements the new Intl constructor semantics.