395836 - (new Date).toLocaleFormat("%D") crashes Minefield

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Dão Gottwald [:dao]]

(new Date).toLocaleFormat("%D") crashes Minefield

http://crash-stats.mozilla.com/report/index/b5238030-60c4-11dc-8c57-001a4bd43ef6?date=2007-09-12-00

Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9a8pre) Gecko/2007091104 Minefield/3.0a8pre

Comment 1

[Jeff Walden [:Waldo]]

Possibly a Microsoft bug in strftime (not that we shouldn't hack around it, if so)?  They claim not to support anything for the %D escape, so the return should be the literal string "%D".  (I haven't looked at our code enough to say whether the bug is our invalid use of the API or not.)

http://msdn2.microsoft.com/en-us/library/fe06s4ak(VS.80).aspx

Comment 2

[Mats Palmgren (inactive)]

Attachment: Like so?

Mute the default invalid_parameter_handler for the strftime() call.
We will fall back to the default date format.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 281001 [details] [diff] [review]
Like so?

JS is single-review, not r+sr.

/be

Comment 4

[Brian Crowder]

Comment on attachment 281001 [details] [diff] [review]
Like so?

Looks fine and works well on Windows for me.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 281001 [details] [diff] [review]
Like so?


>+static void PRMJ_InvalidParameterHandler(const wchar_t* expression,
>+                                         const wchar_t* function, 
>+                                         const wchar_t* file, 
>+                                         unsigned int line, 
>+                                         uintptr_t pReserved)
>+{
>+    /* empty */
>+}
>+#endif
>+
> /* Format a time value into a buffer. Same semantics as strftime() */
> size_t
> PRMJ_FormatTime(char *buf, int buflen, const char *fmt, PRMJTime *prtm)

Nit: as you can see above, prevailing style breaks function definitions up like so

static type
fun(args)
{
    body;
}

Don't need PRMJ_ prefix on the static callback, if it's really-for-sure static. But:

Non-nit? JS_STATIC_DLL_CALLBACK might be needed here. If so, a prmj_ prefix is necessary -- or was once -- for some platforms that must make such callbacks extern instead of static.

/be

Comment 6

[Mats Palmgren (inactive)]

(In reply to comment #5)
> static type
> fun(args)

fixed

> Non-nit? JS_STATIC_DLL_CALLBACK might be needed here

Sure, I added JS_STATIC_DLL_CALLBACK and kept the prefix for now, but
it looks to me like it's obsolete - it translates to "static" for all
platforms except #ifdef WIN16:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jstypes.h&rev=3.42&root=/cvsroot&mark=87,97,108,117,134#79
As far as I know, WIN16 is only defined for Windows 3.1 or older - 
which we don't support. I filed bug 398946 about removing it from the tree.

Comment 7

[Mats Palmgren (inactive)]

Attachment: patch with nits fixed

Here's the updated patch... I don't have permission to check in under js/
so someone needs to land it for me, or grant me the privilege.
Suggested commit message:
Wrap the strftime() call with an empty "invalid parameter handler" on Windows. b=395836 r=crowder a=brendan

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This broke vc71 builds with the following error:

prmjtime.c(581) : error C2065: '_invalid_parameter_handler' : undeclared identifier

I'm reopening this but I guess it could be a new bug?

Comment 10

[Mats Palmgren (inactive)]

Ben, I don't have access to a VC71 environment - could you search the header
files and see if "invalid_parameter_handler" is available at all?
If not, I guess we could adjust the #ifdef to exclude VC71 since we don't
use that for the official builds.
Did this bug even occur in a VC71 build before the patch?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't see it anywhere in the VC71 standard includes or the Platform SDK for Win2k3 R2.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Followup patch to fix the build error for VC++ < 8

Ben, does this fix the build error for you?

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yes, it does, thanks!

Comment 14

[Mats Palmgren (inactive)]

Good, could you try the URL in this bug to see if it terminates the
application?  If not, then the followup patch is good for checkin - otherwise
we need to investigate a fix also for VC71.  Thanks for your help.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I made a xulrunner build yesterday before the original patch went in and it does not crash on the URL above. I can make another build now if you'd like, but I think it is ok.

Comment 16

[Robert Sayre]

It looks like this patch caused a rise in Tp numbers. Which JS functions have this patch in their code paths? Knowing that would help to determine whether it's problem in the harness.

Comment 17

[Mats Palmgren (inactive)]

(In reply to comment #16)
Starting up Firefox, visiting a few sites, opening Options... and
clicking on "Main" through "Advanced", opening DOMI on a web page --
all that caused zero calls to PRMJ_FormatTime().

Looks like something in the harness... some, but not all, of these:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsdate.c&rev=3.90&root=/cvsroot&mark=1982,1989#1982
specially the ones with a "to" prefix I would guess.

Comment 18

[Robert Sayre]

(In reply to comment #17)
> (In reply to comment #16)
> Starting up Firefox, visiting a few sites, opening Options... and
> clicking on "Main" through "Advanced", opening DOMI on a web page --
> all that caused zero calls to PRMJ_FormatTime().
> 
> Looks like something in the harness... some, but not all, of these:

Some items in our Talos pageset (~375 Alexa Top 500 sites) use date functions pretty heavily. Can we back this patch out for a day to see if we observe a drop? I will then produce a graph showing stats for the sites in the pageset that use date functions.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 284199 [details] [diff] [review]
Followup patch to fix the build error for VC++ < 8

Limit the fix to VC++ 8.0 or later. Older versions are not affected by the original bug.

Comment 20

[Brian Crowder]

I backed out "patch with nits fixed" due to perf regressions.

Comment 21

[Brian Crowder]

Mats:  Can you go ahead and rollup your two patches and attach them here?  It will be easier to review.  sayrer is going to add data to this bug tomorrow.

Comment 22

[Bob Clary [:bc] (inactive)]

I resurrected several non cross-platform tests from the old js1_5/Date/toLocaleFormat.js (now js1_5/extensions/toLocaleFormat-01.js) since they covered this case in addition to several others. 

Checking in toLocaleFormat-02.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/toLocaleFormat-02.js,v  <--  toLocaleFormat-02.js
initial revision: 1.1
done

Note:
/*
 * SpiderMonkey only.
 *
 * This test uses format strings which are not supported cross
 * platform and are expected to fail on at least some platforms
 * however they all currently pass on Linux (Fedora Core 6). They are
 * included here in order to increase coverage for cases where a crash
 * may occur.  These failures will be tracked in the
 * mozilla/js/tests/public-failures.txt list.
 * 
 */

If these failures (particularly windows but also a couple on CentOS5) cause anyone problems, I can change the test to fail only if there is a crash.

Comment 23

[Brian Crowder]

sayrer:  Any thoughts on re-landing this?

Comment 24

[Brian Crowder]

I'm increasingly tempted to try landing this again just to see if the same regression shows up.  Sayrer: if I'm wrong, please provide evidence here?

Comment 25

[Brian Crowder]

Not a huge bug, but we have a patch.  If the patch isn't proven to cause a perf regression, then we should land it.  P4.

Comment 26

[Brian Crowder]

Mats:  Please roll "patch with nits fixed" and "Followup patch to fix the build error..." together into one patch and obsolete the other two for ease of landing.

Comment 27

[Mats Palmgren (inactive)]

Attachment: Combined patch

Comment 28

[Reed Loden [:reed]]

Comment on attachment 288068 [details] [diff] [review]
Combined patch

This is a blocker. Approval isn't required.

Comment 29

[Reed Loden [:reed]]

Mats, the "Combined patch" is very much bitrot now after your other changes to prmjtime.c. Mind fixing the bitrot and attaching a new patch for landing, please? Just add the "checkin-needed" keyword when you do so. Thanks!

Comment 30

[Mats Palmgren (inactive)]

Attachment: Combined patch, updated to tip

Reed, make sure you watch perf metrics after landing (see earlier comments).

Comment 31

[Reed Loden [:reed]]

Checking in js/src/prmjtime.c;
/cvsroot/mozilla/js/src/prmjtime.c,v  <--  prmjtime.c
new revision: 3.65; previous revision: 3.64
done

I'll watch the perf numbers and reopen if needed...

Comment 32

[Bob Clary [:bc] (inactive)]

Date.toLocaleFormat("%D") no longer crashes on winxp 1.9.0

note js1_5/extensions/toLocaleFormat-02.js fails on winxp due to lack of OS support for several format strings.

Comment 33

[Brian Crowder]

Does the fact that this stuck this time mean the previous perf suspicions were bogus?