1174386 - Internationalization on workers uses the wrong locale

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[Jeff Walden [:Waldo]]

We set JS runtimes' default locale in xpc_LocalizeRuntime.  But this method is (assuming XPCJSRuntime::XPCJSRuntime's code doing "main runtime" stuff is correct about that) only called for the main thread's JSRuntime.  Nothing sets the default locale for workers' runtimes.

== Testcase ==

Start up a Firefox instance with the LANG environment variable overridden:

[jwalden@find-waldo-now tmp]$ LANG=ar-IQ firefox -no-remote -profile /tmp/foobar/

In one tab load this URL, demonstrating the default locale on the main thread:

data:text/html,<script>document.write(Intl.NumberFormat().resolvedOptions().locale);</script>

I observe ar-IQ in this situation, which is pretty much expected as, once you poke through the layers, the locale service looks to LANG to pick up a default locale.  (I think.  This really should be verified before someone goes assuming I remember what I'm talking about too hard here.) 

Now in another tab load this URL:

data:text/html,<script>w=new%20Worker("data:text/javascript,postMessage(Intl.NumberFormat().resolvedOptions().locale)");w.onmessage=function(e){alert(e.data);};</script>

Here I observe en-GB -- the JS engine's choice of a locale of "last resort" in certain situations, in js/rc/builtin/Intl.js:DefaultLocale.  (At least I *suspect* that's what causes this to be observed.  I haven't verified, but I'd be rather surprised if it's not what causes this.)

== Analysis ==

I doubt the locale service is threadsafe, so we probably can't just call xpc_LocalizeRuntime on all runtimes, at the CycleCollectedJSRuntime level beneath XPCJSRuntime.  So we'll have to do something moderately messy here, to determine locale on the main thread and propagate that to non-main threads.  Joy.

(And it gets worse if some lunacy compels us to make this all work if the user wants to change locale *while the browser is running*.  I hope we won't need to do that, but a sense of foreboding suggests we may not be so lucky.)

Comment 1

[Jeff Walden [:Waldo]]

So I *think* we can deal with this by having WorkerJSRuntime -- the only other subclass of CycleCollectedJSRuntime (?) -- perform some sort of analog to xpc_LocalizeRuntime.  It doesn't appear to me that we can't solve this by having every CCJSRT subclass do so in its constructor.  The unfortunate fun will be ensuring that all subclasses indeed do so.  Perhaps we can use Your Friend And Mine, CRTP, to put a set-locale call in CCJSRT::CCJSRT with the implementation living in the CRTP-provided most-derived class.

I might ponder this some over the weekend, so as not to leave bug 867501 hanging too much in limbo.  Not sure.

Comment 2

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

WorkerJSRuntime is the only other subclass of CycleCollectedJSRuntime.  I think you can rely on that not changing for the foreseeable future.

You can look at how we handle preferences for an example of reading and updating something that's main thread only in the worker.

Comment 3

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0002-Bug-1174386-Regression-test-for-Worker-international.patch

Comment 5

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch

Comment 6

[Arthur Edelstein [:arthur]]

Comment on attachment 8683936 [details] [diff] [review]
0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch

(Fixed error in patch.)

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8683937 [details] [diff] [review]
0002-Bug-1174386-Regression-test-for-Worker-international.patch

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

Some of the trappings of these tests (the .manifest, chrome: URLs working in xpcshell tests, etc.) I no longer recognize.  But making plausible assumptions about them, this looks okay.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8683961 [details] [diff] [review]
0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch

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

I guess I'm assuming the parent runtime is always passed, for all the cases we care about.

::: js/src/jsapi.cpp
@@ +472,5 @@
>  
> +    if (parentRuntime) {
> +        // Ensure that the new runtime has the same `Intl` locale
> +        // as the parent runtime.
> +        JS_SetDefaultLocale(rt, parentRuntime->getDefaultLocale());

Needs a little error-checking:

    if (parentRuntime) {
        // Ensure that the new runtime has the same default locale as the parent runtime.
        const char* defaultLocale = parentRuntime->getDefaultLocale();
        if (!defaultLocale || !JS_SetDefaultLocale(rt, defaultLocale)) {
            JS_DestroyRuntime(rt);
            return nullptr;
        }
    }

This of course isn't responsive to locale changes in the parent runtime.  But I guess we're not doing those yet/now, so okay enough.

Comment 9

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Comment on attachment 8683961 [details] [diff] [review]
> 0001-Bug-1174386-Use-correct-default-Intl-locale-on-worke.patch
> 
> Review of attachment 8683961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess I'm assuming the parent runtime is always passed, for all the cases
> we care about.
> 
> ::: js/src/jsapi.cpp
> @@ +472,5 @@
> >  
> > +    if (parentRuntime) {
> > +        // Ensure that the new runtime has the same `Intl` locale
> > +        // as the parent runtime.
> > +        JS_SetDefaultLocale(rt, parentRuntime->getDefaultLocale());
> 
> Needs a little error-checking:
> 
>     if (parentRuntime) {
>         // Ensure that the new runtime has the same default locale as the
> parent runtime.
>         const char* defaultLocale = parentRuntime->getDefaultLocale();
>         if (!defaultLocale || !JS_SetDefaultLocale(rt, defaultLocale)) {
>             JS_DestroyRuntime(rt);
>             return nullptr;
>         }
>     }
> 
> This of course isn't responsive to locale changes in the parent runtime. 
> But I guess we're not doing those yet/now, so okay enough.

Thank you for the review. Here's a new version of the patch with the requested changes.

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3167561ea848

Comment 10

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

We found that the previous patch was not thread safe. Here's a new version of the patch for review.

Comment 11

[Arthur Edelstein [:arthur]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b66f71b978

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8693347 [details] [diff] [review]
0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

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

::: dom/workers/RuntimeService.cpp
@@ +1760,5 @@
>      NS_WARNING("Could not set the thread's priority!");
>    }
>  
> +  JSRuntime* parentRuntime = JS_GetParentRuntime(aCx);
> +  const char* defaultLocale = parentRuntime ? JS_GetDefaultLocale(parentRuntime) : nullptr;

Wait, if JS_GetDefaultLocale returns rt->getDefaultLocale() which returns a string owned by the runtime, isn't this bad if JS_SetDefaultLocale happens, exactly as I noted in bug 867501 comment 3?

Or wait.  Is this only supposed to land *after* the patch there lands?  I would prefer to see that land and sit in the tree at least a few days, and only then review/deal with this patch.  Can you land that first, if indeed there's no reason not to land it?

On the other hand, if you're assuming that's landed first, don't you leak mDefaultLocale here (assuming it's non-null)?

So something seems wrong here, regardless whether or not you're building atop that patch.  Please sort out that bug and land, then deal with updating this patch, or post a fresh patch that doesn't depend on that bug happening first.  Ideally, whichever results in more bite-sized reviewable patches.  :-)

Comment 13

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> Comment on attachment 8693347 [details] [diff] [review]
> 0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch
> 
> Review of attachment 8693347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/RuntimeService.cpp
> @@ +1760,5 @@
> >      NS_WARNING("Could not set the thread's priority!");
> >    }
> >  
> > +  JSRuntime* parentRuntime = JS_GetParentRuntime(aCx);
> > +  const char* defaultLocale = parentRuntime ? JS_GetDefaultLocale(parentRuntime) : nullptr;
> 
> Wait, if JS_GetDefaultLocale returns rt->getDefaultLocale() which returns a
> string owned by the runtime, isn't this bad if JS_SetDefaultLocale happens,
> exactly as I noted in bug 867501 comment 3?

Argh, sorry for making you catch the same mistake twice. I've fixed this now.

> Or wait.  Is this only supposed to land *after* the patch there lands?  I
> would prefer to see that land and sit in the tree at least a few days, and
> only then review/deal with this patch.  Can you land that first, if indeed
> there's no reason not to land it?

This patch is supposed to be independent of the patch in bug 867501.

> On the other hand, if you're assuming that's landed first, don't you leak
> mDefaultLocale here (assuming it's non-null)?

Thanks for catching that. Fixed as well.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8694837 [details] [diff] [review]
0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

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

Sorry for the delay.  :-(  Will get back within a day next time around, assuming nothing arises beyond what I point out below.

This looks broadly good, but the ownership and management of strings here is much too manual, and non-obvious/undocumented, for my tastes.  Let's throw some UniquePtr at this -- start with the jsapi.h comment, as it's logically the first thing to understand here.

::: dom/workers/RuntimeService.cpp
@@ +1061,4 @@
>    WorkerPrivate* mWorkerPrivate;
>    RefPtr<WorkerThread> mThread;
>    JSRuntime* mParentRuntime;
> +  const char* mDefaultLocale;

This will be UniquePtr<const char*, JS::FreePolicy> with the JSAPI signature change mentioned elsewhere.

@@ +1087,5 @@
>    WorkerThreadPrimaryRunnable(WorkerPrivate* aWorkerPrivate,
>                                WorkerThread* aThread,
> +                              JSRuntime* aParentRuntime,
> +                              const char* aDefaultLocale)
> +  : mWorkerPrivate(aWorkerPrivate), mThread(aThread), mParentRuntime(aParentRuntime), mDefaultLocale(aDefaultLocale)

|aDefaultLocale| will also be UniquePtr<const char*, JS::FreePolicy>.  And as a consequence, rather than initializing the member with the parameter directly, initialize it with |Move(aDefaultLocale)| (mozilla::Move, if necessary).

@@ +1098,5 @@
>  
>  private:
>    ~WorkerThreadPrimaryRunnable()
> +  {
> +    JS_free(nullptr, (void*) mDefaultLocale);

UniquePtr means you don't have to remember this.  \o/

@@ +1762,5 @@
>      NS_WARNING("Could not set the thread's priority!");
>    }
>  
> +  JSRuntime* parentRuntime = JS_GetParentRuntime(aCx);
> +  const char* defaultLocale = parentRuntime ? JS_GetDefaultLocale(parentRuntime) : nullptr;

And UniquePtr<const char*, JS::FreePolicy> for the type here, too.

@@ +1771,2 @@
>    nsCOMPtr<nsIRunnable> runnable =
> +    new WorkerThreadPrimaryRunnable(aWorkerPrivate, thread, parentRuntime, defaultLocale);

You'll have to pass Move(defaultLocale) for single-ownership fidelity.

@@ +2711,4 @@
>      WorkerJSRuntime runtime(mParentRuntime, mWorkerPrivate);
>      JSRuntime* rt = runtime.Runtime();
>  
> +    if (!JS_SetDefaultLocale(rt, mDefaultLocale)) {

Interesting.  Once this call is reached, mDefaultLocale is unused modulo freeing.  So it seems best to eagerly free, for greater precision of operation and tightness of code.  So then,

    if (mDefaultLocale) {
      if (!JS_SetDefaultLocale(rt, mDefaultLocale.get())) {
        NS_WARNING("could not set worker locale!");
      }

      mDefaultLocale = nullptr;
    }

::: js/src/jsapi.cpp
@@ +5608,5 @@
> +    if (const char* locale = rt->getDefaultLocale()) {
> +        return JS_strdup(rt, locale);
> +    } else {
> +        return nullptr;
> +    }

SpiderMonkey style has this as

    if (const char* locale = rt->getDefaultLocale())
        return UniquePtr<const char*, JS::FreePolicy>(JS_strdup(rt, locale));
    return nullptr;

i.e. don't brace ifs with single-line condition, consequent, and any alternatives, and don't have |else| after a return statement (because there's no need for it).

::: js/src/jsapi.h
@@ +4655,5 @@
>  
>  /**
> + * Look up the default locale for the ECMAScript Internationalization API.
> + */
> +extern JS_PUBLIC_API(const char*)

Don't return a raw pointer.  That makes ownership of the returned string unclear, and even if we document what happens, people still have to be aware of it, think about it, etc.  Instead let's do this:

extern JS_PUBLIC_API(mozilla::UniquePtr<const char*, JS::FreePolicy>)

Using UniquePtr *enforces* that there's only a single owner (so long as nobody willfully copies via .get()) and is self-documenting, so let's do that.

Comment 15

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

Here's a new version of the patch with changes based closely on those requested. I decided to use UniqueChars because it pretty much matches the behavior we want.

Comment 16

[Arthur Edelstein [:arthur]]

Here are the try results for the latest version of the patch (including the regression patch as well):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbb044b0de0

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8764499 [details] [diff] [review]
0001-Bug-1174386-Make-workers-inherit-default-Intl-locale.patch

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

Great!  There was a very minor bit of bitrot, but it was easily fixt.  Will land patch and test together within the next hour or so, after I do a build without the test and verify it fails, then a build with the fix in place and verify it passes.

Sorry for the delay reviewing this, been busy on my end.  :-(

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9726b95aadf6
Make workers inherit the default Intl locale from the main thread, rather than using a bogus fallback value.  r=jwalden

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71634609bbaf
Bustage fixes; things changed underneath me during the final rebase.  :-(  r=red in a CLOSED TREE

Comment 20

[Lina Butler [:lina]]

Sorry, had to back both out for worker mochitest bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=31682502&repo=mozilla-inbound and Android xpcshell bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=31684916&repo=mozilla-inbound#L3120.

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

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e603e9aba7d
Backed out changeset 37aed188b674 unexpected pass in 576878.xhtml

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Pulsebot from comment #21)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3e603e9aba7d
> Backed out changeset 37aed188b674 unexpected pass in 576878.xhtml

ignore that, catched the wrong chageset, restored the backout now

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20f2a977ecb7
Back out 2 changesets for mochitest and xpcshell bustage.

Comment 24

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

So, is this in the tree or out?

Comment 25

[Carsten Book [:Tomcat]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> So, is this in the tree or out?

its backed out

Comment 26

[Jeff Walden [:Waldo]]

Gettin' back to gettin' this in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d51552235525

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f06ba15fda
Make workers inherit the default Intl locale from the main thread, rather than using a bogus fallback value.  r=jwalden

Comment 28

[Jeff Walden [:Waldo]]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6dd26b94f231f92770d031a60c93ad43ab08ac

It turns out this worker startup code isn't run quite early enough -- it looks like it happens *on the worker thread*, not on the thread that's spawning the worker.  I think.  So sometimes (maybe always) JS_GetDefaultLocale(cx or parentCx) will act on a context across a thread, and if that thread is doing JS stuff we'll assert.  Looking to see where the right-er place is to do this, will figure out tomorrow.

Comment 29

[Jeff Walden [:Waldo]]

Attachment: A quite-different approach to setting workers' default locale

New-ish approach!

So as I said, the old approach does cross-thread things that are bad.  This approach does all default locale-copying into the freshly created WorkerPrivate, on the thread that is creating the worker.  Then on the worker, that copied default-locale string is used to set the worker's context's default locale.  It's necessary to copy the string here, because if we touch the main-thread's string directly, we're doing cross-thread access and hit heap-is-idle assertions and the like.

I believe this approach does all default-locale-copying on the thread spawning the worker, and it does all JS_SetDefaultLocale using that copied value on the worker itself.  And I believe this applies regardless whether the thread doing the spawning of a worker is the main thread, or is itself another worker.  Please correct me if I'm wrong!

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8779432 [details] [diff] [review]
A quite-different approach to setting workers' default locale

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

Do we want to support the changing of locale?

::: dom/workers/WorkerPrivate.cpp
@@ +4201,5 @@
> +  // XPJSRuntime::Initialize for the main thread, set by
> +  // WorkerThreadPrimaryRunnable::Run for workers just before running worker
> +  // code), so this is never SpiderMonkey's builtin default locale.
> +  JS::UniqueChars defaultLocale = JS_GetDefaultLocale(aCx);
> +  if (!defaultLocale) {

if (NS_WARN_IF(!defaultLocale)) {

::: dom/workers/WorkerPrivate.h
@@ +940,5 @@
>  
>    // fired on the main thread if the worker script fails to load
>    nsCOMPtr<nsIRunnable> mLoadFailedRunnable;
>  
> +  JS::UniqueChars mDefaultLocale;

Write a comment saying that this value is nullified when the JS Context is initialized.

@@ +1053,5 @@
>      mDebugger = aDebugger;
>    }
>  
> +  JS::UniqueChars
> +  AdoptDefaultLocale() {

{ in a new line.

@@ +1057,5 @@
> +  AdoptDefaultLocale() {
> +    MOZ_ASSERT(mDefaultLocale,
> +               "the default locale must have been successfully set for anyone "
> +               "to be wanting to adopt it");
> +    return mozilla::Move(mDefaultLocale);

do you need mozilla:: ?

::: intl/locale/tests/unit/data/intl_on_workers_worker.js
@@ +1,5 @@
> +self.onmessage = function (data) {
> +  let myLocale = Intl.NumberFormat().resolvedOptions().locale;
> +  self.postMessage(myLocale);
> +};
> +

extra line.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df1c5ca718b6
Make workers inherit the default Intl locale from the main thread, rather than using a bogus fallback value.  r=jwalden, r=baku

Comment 32

[Jeff Walden [:Waldo]]

(In reply to Andrea Marchesini [:baku] from comment #30)
> Do we want to support the changing of locale?

It seems reasonable we might want to support changing the locale in the main process -- eventually; there's no way to do it now.  I don't think it'd be worth the effort to propagate that into newly-created workers -- and it might disrupt expectations about behavioral consistency.  This patch would support that set-at-creation model, so nothing to do if we ever support the main process changing its default locale.

> @@ +1057,5 @@
> > +  AdoptDefaultLocale() {
> > +    MOZ_ASSERT(mDefaultLocale,
> > +               "the default locale must have been successfully set for anyone "
> > +               "to be wanting to adopt it");
> > +    return mozilla::Move(mDefaultLocale);
> 
> do you need mozilla:: ?

No, removed.  (Although I should say I'm not totally pleased with this -- I somewhat prefer seeing names in headers qualified.  And in JS-land, mozilla:: in headers is mandatory because we're not in namespace mozilla.)

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2061eabc08
Fix a rebase-induced pointer-to-bool conversion error.  r=bustage in a CLOSED TREE

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/df1c5ca718b6
https://hg.mozilla.org/mozilla-central/rev/9b2061eabc08