794927 - Unsupported format strings in Date().toLocaleFormat abort on Windows

Status: RESOLVED DUPLICATE of bug 818634

(Core :: JavaScript Engine, defect)

Description

[Benjamin Smedberg]

On Windows, calling Date().toLocaleFormat('%R') crashes. %R is a shortcut for %H:%M in the Linux strftime, but is not supported on Windows and I think any unsupported % character causes the CRT to abort on Windows. See bug 794662 for some stacks where this hit B2G.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

See also bug 806231 about %e causing crashes.

Comment 2

[Jim Mathies [:jimm]]

I reported on this over in bug 681704. Weave hits this in metrofx constantly, crashing the browser. I've seen this on two observer events - weave:service:login:start and weave:service:login:finish.

Comment 3

[Richard Newman [:rnewman]]

Sync doesn't use %R:

service.js
1155:    let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);

stages/enginesync.js
180:      let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);

constants.js
179:LOG_DATE_FORMAT:                       "%Y-%m-%d %H:%M:%S",

Comment 4

[Richard Newman [:rnewman]]

Ohhhh, you're running old XUL code.

mobile/xul/chrome/content/sync.js
479:      let syncDate = new Date(lastSync).toLocaleFormat("%a %R");



Try copying desktop instead:

browser/base/content/browser-syncui.js
296:    let lastSyncDate = new Date(lastSync).toLocaleFormat("%a %H:%M");



You might find more things like this; we haven't maintained XUL Sync code for about 15 months.

Comment 5

[Richard Newman [:rnewman]]

I will happily r+ the change from %R to %H:%M if you want to land it on Elm, Jim.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #5)
> I will happily r+ the change from %R to %H:%M if you want to land it on Elm,
> Jim.

Working on it now. Thanks for the f(In reply to Richard Newman [:rnewman] from comment #5)
> I will happily r+ the change from %R to %H:%M if you want to land it on Elm,
> Jim.

Thanks, spun off in bug 827767.

Comment 7

[Jesse Ruderman]

Invalid format strings abort too:

alert((new Date).toLocaleFormat("%"));

What's the reason for aborting?  Seems like it should be a warning, at worst.

Comment 8

[(not currently active) Ted Mielczarek]

This is getting passed directly to the Windows CRT which is doing the aborting.  I don't think this is actually intentional on the JS side, just a lack of sanitization on the input.

Comment 9

[Jesse Ruderman]

http://mxr.mozilla.org/mozilla-central/source/js/src/prmjtime.cpp#499

_CrtSetReportMode(_CRT_ASSERT, 0);

Added in bug 395836.  I guess the default is to crash/throw, and we changed it to a debug-only assert?

Comment 10

[(not currently active) Ted Mielczarek]

Oh. Apparently we shot ourselves in the foot here in bug 587982:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4310
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1352

We made CRT assertions intentionally mozalloc_abort on Windows. I guess you could slip a _CrtSetReportHook into prmjtime as well.

These bother me on a fundamental level, since these are all *process global*. I don't know of another way to handle this aside from not using Microsoft's implementation of strftime.

Comment 11

[Bob Clary [:bc] (inactive)]

Is this just a question of whether _CRT_ASSERT should abort or not?

Comment 12

[(not currently active) Ted Mielczarek]

I don't know about in the general case, but in this specific case we pass the format string directly from JavaScript down to strftime, so it's trivially easy to make this abort from content JS (in a debug build).

Comment 13

[André Bargull [:anba]]

toLocaleFormat was removed in bug 818634.