899361 - Track initialized-as-Intl status using a runtime-wide WeakMap and self-hosted intrinsic functions, instead of through per-global WeakMaps

Status: RESOLVED WONTFIX

(Core :: JavaScript: Internationalization API, defect)

Description

[Jeff Walden [:Waldo]]

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 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=172e3525cb28

Comment 3

[Till Schneidereit [:till]]

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.

Comment 4

[Jeff Walden [:Waldo]]

Nope.

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

Investigating.

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Till Schneidereit [:till]]

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.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Set a stack quota in the two jsapi-tests that don't right now

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 8

[Till Schneidereit [:till]]

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

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

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.)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d02216b36873

Comment 12

[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 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[André Bargull [:anba]]

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.

Comment 15

[André Bargull [:anba]]

bug 1328386 implements the new Intl constructor semantics.

Comment 16

[André Bargull [:anba]]

No longer necessary because the spec changed - bug 1328386.