934889 - JS_InitStandardClasses fails intermittently on a web worker due to the Intl API

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Web workers currently do not call JS_InitStandardClasses.  I added code to call it but the function fails intermittently.  When it does fail, it fails in IntlInitialize (http://hg.mozilla.org/mozilla-central/annotate/770de5942471/js/src/builtin/Intl.cpp#l396).

Filing this for a bug number to put in my comment.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is it possible that intl init is somehow racy/unsafe off the main thread?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Absolutely.

Comment 3

[Jeff Walden [:Waldo]]

Intl init is basically plain old JS operations, plus some ICU calls.  ICU is supposed to be threadsafe such that the ICU calls will be okay.  (There are some things you have to do before using ICU at all, in u_init -- which we call at startup well before any sorts of racing things could happen.)

Catching this in a debugger is really what's desired here, at this point, because there's definitely too much under IntlInitialize to eyeball what's wrong.  :-\

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also because not everything is hooked up to JS_ResolveStandardClass, workers are missing some builtins.  e.g. bug 935165.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Anybody interested in looking into this?

Comment 6

[Norbert Lindenberg]

Sorry, I'm working on other things now.

Comment 7

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

[Jeff Walden [:Waldo]]

Kyle claims that workers resolve standard classes now, everywhere.  They didn't when this bug was filed, but since then with WebIDL progress that's changed, and now everyone does resolve them.  And if everyone's resolving them, that's enough for observably correct semantics.  Getting the properties via init, just means they show up earlier, but there's not really much reason to care about that if nobody's looking for them.

And indeed, this all comports with my testing demonstrating that Intl stuff works just fine on workers.  For example, this:

http://whereswalden.com/files/mozilla/intl-in-workers/intl-worker.html

So I am going to claim this is fine now.  There's still the code that http://mxr.mozilla.org/mozilla-central/search?string=bug%20934889 refers to, that should be cleaned up, to be sure.  But that's a much lesser concern than the one originally expressed here.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This seems to work now (at least on my machine).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8736376 [details] [diff] [review]
Patch

r=me

Do we still need resolve standard classes stuff?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37bfc8717cf3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37bfc8717cf3