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

VERIFIED FIXED in mozilla1.9beta2

Status

()

P4
critical
VERIFIED FIXED
11 years ago
4 years ago

People

(Reporter: dao, Assigned: mats)

Tracking

(Depends on: 1 bug, {crash, testcase})

Trunk
mozilla1.9beta2
x86
Windows XP
crash, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
(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
Flags: in-testsuite?
Flags: blocking1.9?

Comment 1

11 years ago
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
(Assignee)

Comment 2

11 years ago
Created attachment 281001 [details] [diff] [review]
Like so?

Mute the default invalid_parameter_handler for the strftime() call.
We will fall back to the default date format.
Assignee: general → mats.palmgren
Status: NEW → ASSIGNED
Attachment #281001 - Flags: superreview?(brendan)
Attachment #281001 - Flags: review?(brendan)
Comment on attachment 281001 [details] [diff] [review]
Like so?

JS is single-review, not r+sr.

/be
Attachment #281001 - Flags: superreview?(brendan)
Attachment #281001 - Flags: superreview?
Attachment #281001 - Flags: review?(crowder)
Attachment #281001 - Flags: review?(brendan)
(Reporter)

Updated

11 years ago
Attachment #281001 - Flags: superreview?

Comment 4

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

Looks fine and works well on Windows for me.
Attachment #281001 - Flags: review?(crowder) → review+
(Assignee)

Updated

11 years ago
Attachment #281001 - Flags: approval1.9?
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
Attachment #281001 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 6

11 years ago
(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.
(Assignee)

Comment 7

11 years ago
Created attachment 283951 [details] [diff] [review]
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
Attachment #281001 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

11 years ago
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?
I don't see it anywhere in the VC71 standard includes or the Platform SDK for Win2k3 R2.
(Assignee)

Comment 12

11 years ago
Created attachment 284199 [details] [diff] [review]
Followup patch to fix the build error for VC++ < 8

Ben, does this fix the build error for you?
(Assignee)

Comment 14

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

11 years ago
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.
(Assignee)

Comment 17

11 years ago
(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

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

(Assignee)

Comment 19

11 years ago
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.
Attachment #284199 - Flags: review?(crowder)

Updated

11 years ago
Attachment #284199 - Flags: review?(crowder) → review+

Comment 20

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

Comment 21

11 years ago
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

11 years ago
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.
Flags: in-testsuite? → in-testsuite+

Comment 23

11 years ago
sayrer:  Any thoughts on re-landing this?

Updated

11 years ago
Keywords: testcase

Comment 24

11 years ago
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

11 years ago
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.
Priority: -- → P4

Comment 26

11 years ago
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.
(Assignee)

Comment 27

11 years ago
Created attachment 288068 [details] [diff] [review]
Combined patch
Attachment #283951 - Attachment is obsolete: true
Attachment #284199 - Attachment is obsolete: true

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+

Updated

11 years ago
Attachment #288068 - Flags: review+
Attachment #288068 - Flags: approval1.9?
Comment on attachment 288068 [details] [diff] [review]
Combined patch

This is a blocker. Approval isn't required.
Attachment #288068 - Flags: approval1.9?

Updated

11 years ago
Keywords: checkin-needed
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!
Keywords: checkin-needed
(Assignee)

Comment 30

11 years ago
Created attachment 288656 [details] [diff] [review]
Combined patch, updated to tip

Reed, make sure you watch perf metrics after landing (see earlier comments).
Attachment #288068 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
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...
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Comment 32

11 years ago
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.
Status: RESOLVED → VERIFIED

Comment 33

11 years ago
Does the fact that this stuck this time mean the previous perf suspicions were bogus?
You need to log in before you can comment on or make changes to this bug.