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

NEW
Unassigned

Status

()

Core
JavaScript Engine
--
critical
5 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Unassigned)

Tracking

(Blocks: 1 bug, {csectype-dos})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2])

(Reporter)

Description

5 years ago
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.
See also bug 806231 about %e causing crashes.
Whiteboard: [js:p2]

Comment 2

5 years ago
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.

Updated

5 years ago
Whiteboard: [js:p2] → [js:p2][metro-mvp]
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",
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.
I will happily r+ the change from %R to %H:%M if you want to land it on Elm, Jim.

Comment 6

5 years ago
(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.

Updated

4 years ago
Blocks: 681704

Updated

4 years ago
Summary: Date().toLocaleFormat('%R') crashes → Unsupported format strings in Date().toLocaleFormat abort on Windows
Whiteboard: [js:p2][metro-mvp] → [js:p2]

Comment 7

4 years ago
Invalid format strings abort too:

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

What's the reason for aborting?  Seems like it should be a warning, at worst.
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

4 years ago
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?
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

4 years ago
Is this just a question of whether _CRT_ASSERT should abort or not?
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).
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.