858946 - Download panel does not display the date correctly due to toLocaleFormat() called from JavaScript code modules

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: Screenshot

See the attached screenshot.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Screenshot before restart

The date was displayed correctly right after updating to Firefox 20 and before restarting (that is, when the entries are not yet removed from downloads.sqlite).

Comment 2

[Masatoshi Kimura [:emk]]

Hey, toLocaleString("%A") returned a broken string on Japanese Windows.
< new Date().toLocaleFormat("%A")
> "\x8C\x8E\x97j\x93ú"
"jú" matches a broken pattern in the screenshot.
Download Panel uses toLocaleString to get a localized day of week string.
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm?rev=dc6c1d4bff95#345

Comment 3

[Masatoshi Kimura [:emk]]

s/toLocaleString/toLocaleFormat/g;

Comment 4

[Marco Bonardo [:mak]]

you mean this may be an issue of the javascript engine?
Having a small test case file would be nice.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Marco Bonardo [:mak] from comment #4)
> you mean this may be an issue of the javascript engine?

Yes, but we must have a workaround because the date is displayed correctly in some entries ("金曜日" is correct in the above screenshot).

> Having a small test case file would be nice.

I already wrote a minimum testcase in comment #2. Just type |new Date().toLocaleFormat("%A")| into Web Console or Error Console or Scratchpad or something.

Actual result (if today is Monday):
"\x8C\x8E\x97j\x93ú"

Expected result (ditto):
"月曜日"

Comment 6

[Masatoshi Kimura [:emk]]

Looks like this is already fixed on Nightly by bug 769871. But is is unlikely to be suitable to branches.
Is it possible to write a smaller workaround in the browser side for branches?

Comment 8

[Masatoshi Kimura [:emk]]

Bug 769871 was unrelated.

SpiderMonkey depends on local callbacks to convert the platform charset to Unicode.
Only XPConnect will set local callbacks right now.
On Firefox 20, toLocalFormat() is called from a sandbox (read: Web Console) or a JavaScript code module, it will just inflate the string encoded in the platform charset. So the result string will be garbled as I show in comment #5.
On Nightly, local callbacks has been moved from JSContext to JSRuntime. So toLocalFormat() works in Web Console or JavaScript code modules. But it does not work in workers yet.

For first a few items in Download Panel, getReadableDates() was directly called from the overlay, so the date was shown correctly. The stack was:
DU_getReadableDates@resource://gre/modules/DownloadUtils.jsm:354
DES__getStatusText@chrome://browser/content/downloads/allDownloadsViewOverlay.js:431
DES__updateDownloadStatusUI@chrome://browser/content/downloads/allDownloadsViewOverlay.js:460
DES__updateUI@chrome://browser/content/downloads/allDownloadsViewOverlay.js:512
DES_ensureActive@chrome://browser/content/downloads/allDownloadsViewOverlay.js:117
@chrome://browser/content/downloads/allDownloadsViewOverlay.js:1082

For the rest items, getReadableDates() was called via _PromiseWorker.jsm:
DU_getReadableDates@resource://gre/modules/DownloadUtils.jsm:354
DES__getStatusText@chrome://browser/content/downloads/allDownloadsViewOverlay.js:431
DES__updateDownloadStatusUI@chrome://browser/content/downloads/allDownloadsViewOverlay.js:460
onSuccess@chrome://browser/content/downloads/allDownloadsViewOverlay.js:255
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:120
then@resource://gre/modules/commonjs/sdk/core/promise.js:45
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:187
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:120
then@resource://gre/modules/commonjs/sdk/core/promise.js:45
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:187
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:120
then@resource://gre/modules/commonjs/sdk/core/promise.js:45
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:187
onmessage@resource://gre/modules/osfile/_PromiseWorker.jsm:134

This in consistent with the screenshot attached in comment #0.

Comment 9

[Marco Bonardo [:mak]]

Thanks for the analysis, I tried searching a bit in bugzilla and looks like it was an explicit choice in the past to leave workers out, though this case could maybe be fixed?
The method is used in a promise callback and since those are being used more and more, looks like a dangerous pitfall.

We can probably hack up a workaround in the Downloads panel, though I'd first like to get an opinion from someone working on the js engine.

Comment 10

[Masatoshi Kimura [:emk]]

Locale callbacks were moved into JSRuntime since Firefox 21 (bug 826009). And I confirmed the problem was fixed on Firefox 21 Beta.
The fix will not be backported to the Firefox 20 release, of course.

Comment 11

[Masatoshi Kimura [:emk]]

Oh, mid-aired.

(In reply to Marco Bonardo [:mak] from comment #9)
> Thanks for the analysis, I tried searching a bit in bugzilla and looks like
> it was an explicit choice in the past to leave workers out, though this case
> could maybe be fixed?
> The method is used in a promise callback and since those are being used more
> and more, looks like a dangerous pitfall.
> 
> We can probably hack up a workaround in the Downloads panel, though I'd
> first like to get an opinion from someone working on the js engine.

Fortunately, we can just wait until Firefox 21 :)

Comment 12

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on FF 21.0 and FF 22.b1 on Windows 8 x64:

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0(20130511120803)

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0(20130514181517)

Comment 13

[Mihai Morar, (:MihaiMorar)]

Verified as fixed on FF 23b1: 

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
20130625125232