Closed Bug 329383 Opened 18 years ago Closed 18 years ago

Text size change either via control key or View -> Text Size fails with Error: uncaught exception: 2147942487

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: wgianopoulos, Assigned: brendan)

References

Details

(Keywords: regression)

Attachments

(2 files)

attempt at a fix
1.93 KB, patch
Details | Diff | Splinter Review
attempt at a fix, v2
3.15 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review 
Attempting to change the text size of a page either via control keys or via the View -> Text size menu has no effect on the rendered page.  Additionally the error console shows Error: uncaught exception: 2147942487.
Only happens in NON-cairo trunk builds
This is happening in my trunk builds which are cairo builds.  Perhaps it is only failing in Windows builds that don't use vc8?
CC->Stuart
This regression is caused by the switch from libm fdlibm from the check-in for bug 326842.
Assignee: nobody → general
Blocks: 326842
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
2147942487 is 0x80070057, which is NS_ERROR_ILLEGAL_VALUE.  What is throwing that as a numeric exception value?  IIRC XPConnect throws objects, not numbers; CAPS throws strings.  Who throws raw failure nsresults?

/be
That exception is likely thrown from the JS code that attempts to validate it's input before setting the text zoom. See:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/viewZoomOverlay.js&rev=1.6#85
which throws NS_ERROR_INVALID_ARG if a range check fails. Not sure how that check could be failing now when it wasn't before...
Fwiw, this worksforme in a Linux Seamonkey build from today.
Ah, comment 2 is correct. the problem is at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/viewZoomOverlay.js&rev=1.6&mark=72#69
Math.round is giving bogus results in a Pacifica trunk build (VC6), but is working correctly with a Gaius nightly build (VC8).

With the VC6 build, I get the following in the JS console:
Math.round(1, 100);
1072693248

While with the VC8 build I get:
Math.round(1,100);
1
as expected. Seems as though something from bug 326842 doesn't like VC6.
(In reply to comment #8)
> Math.round(1,100);

Er, ignore the second argument in these examples.
Flags: blocking1.9a1+
Flags: blocking1.8.1+
(In reply to comment #8)
> as expected. Seems as though something from bug 326842 doesn't like VC6.
> 

Or VC7 either.

I tested various math functions on a pacifica build, and round is the only one that behaves this strangely.  Math.floor works fine, and the coding of the 2 functions looks similar.  Could this possibly be an issue with copysign?
Can someone test and say what inputs to Math.round give obviously wrong outputs? Try checking against the results from this emulation:

function rnd(x) {
  if (!isFinite(x) || x === 0) return x
  if (-0.5 <= x && x < 0) return -0
  return Math.floor(x + 0.5)
}

/be
(In reply to comment #12)
> function rnd(x) {
>   if (!isFinite(x) || x === 0) return x
>   if (-0.5 <= x && x < 0) return -0
>   return Math.floor(x + 0.5)
> }

I don't know if you had any particularly "interesting" inputs you want tested, but here are some examples:

[-Math.PI, -0, 0, Math.PI].map(rnd)
-3,0,0,3
[-Math.PI, -0, 0, Math.PI].map(Math.round)
-1073217536,-2147483648,0,1074266112

[1,2,3,4,5,6,7,8,9].map(rnd)
1,2,3,4,5,6,7,8,9
[1,2,3,4,5,6,7,8,9].map(Math.round)
1072693248,1073741824,1074266112,1074790400,1075052544,1075314688,1075576832,1075838976,1075970048
This looks like a _copysign bug, or more likely a bug in our use of it:

js> a = [1072693248,1073741824,1074266112,1074790400,1075052544,1075314688,1075576832,1075838976,1075970048]
1072693248,1073741824,1074266112,1074790400,1075052544,1075314688,1075576832,1075838976,1075970048
js> function tohex(n){return n.toString(16)}
js> a.map(tohex)
3ff00000,40000000,40080000,40100000,40140000,40180000,401c0000,40200000,40220000

Looks like the high word of the double result of Math.floor(x + 0.5).  By any chance is _copysign prototyped on those pre-VC8 versions as returning a 32-bit integral type?  Because jslibmath.h now, via macro name substitution, prototypes _copysign as taking two doubles and returning double.

/be
my old msdn install for vc6 and the newer vc8 docs both say 
double _copysign( 
   double x, 
   double y 
);

(In reply to comment #8)
> Math.round is giving bogus results in a Pacifica trunk build (VC6), but is
> working correctly with a Gaius nightly build (VC8).

What is the right MSC_VERS_ or whatever macro to test for < VC8, and what value?

/be

(In reply to comment #16)
> What is the right MSC_VERS_ or whatever macro to test for < VC8, and what
> value?

#if _MSC_VER < 1400 matches compilers older than VC8.
Attached patch attempt at a fix — — Splinter Review 
I'm at home, Linux only -- anyone able to test this?

/be
Also, to confirm that _copysign is busted in VC6 and 7, try these:

js> z = Math.min(-0, 0)
0
js> Math.atan2(z, z)
-3.141592653589793
js> z = Math.max(-0, 0)
0
js> Math.atan2(z, z)
0

and report results.

/be
Or just test

js> z = Math.min(-0, 0)
0
js> 1/z
-Infinity
js> z = Math.max(-0, 0)
0
js> 1/z
Infinity

/be
In the broken build:
js> z = Math.min(-0, 0)
0
js> 1/z
-Infinity
js> z = Math.max(-0, 0)
0
js> 1/z
-Infinity
I can also test the patch once my build completes.
(In reply to comment #19)
> Also, to confirm that _copysign is busted in VC6 and 7, try these:

VC71:

js> z = Math.min(-0,0)
0
js> 1/z
-Infinity
js> z = Math.max(-0,0)
0
js> 1/z
-Infinity
js> Math.atan2(z,z)
-3.141592653589793
js>

I can't find anything saying that copysign is actually a function that is part of any standard.  We already seem to have a mess of kludgey ifdefs becuase of OS and compiler differences.  Why not just define a macro that should work universally?

Something like:

#define fd_copysign(x,y)        y<0?-fabs(x):fabs(x)
This should do it.

/be
Attachment #214138 - Flags: review?(mrbkap)
Patched js.exe:
js> z = Math.min(-0, 0)
0
js> 1/z
-Infinity
js> z = Math.max(-0, 0)
0
js> 1/z
Infinity
js>
(In reply to comment #24)
> I can't find anything saying that copysign is actually a function that is part
> of any standard.  We already seem to have a mess of kludgey ifdefs becuase of
> OS and compiler differences.  Why not just define a macro that should work
> universally?
> 
> Something like:
> 
> #define fd_copysign(x,y)        y<0?-fabs(x):fabs(x)

From IRC, just recording for posterity:

<brendan> wgianopoulos: your proposal is slower on all platforms
<brendan> that's never a good tradeoff for an MS compiler/libm bug
<brendan> wgianopoulos: it's in C99
<brendan> from man copysign on FC3: C99,  BSD  4.3.  This function is defined in IEC 559 (and the appendix
<brendan>        with recommended functions in IEEE 754/IEEE 854).
<wgianopoulos> OK great I was having much troubel with ggogle trying to find anything where the stuff it thought was most relvant being at all recent.

/be
Assignee: general → brendan
Josh, did you land your fdlibm removal patch on the 1.8 branch?

/be
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Fixed on trunk.  I don't believe this bug needs to be marked blocking1.8.1.

/be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
I did not land the fdlibm removal patch (the one that caused this bug) on 1.8.1.
Comment on attachment 214138 [details] [diff] [review]
attempt at a fix, v2

>Index: jsmath.c
>@@ -261,17 +261,17 @@ math_min(JSContext *cx, JSObject *obj, u
>-        if ((x==0)&&(x==z)&&(fd_copysign(1.0,x)==-1))
>+        if (x == 0 && x == z && fd_copysign(1.0,x) == -1)

If you're cleaning up the whitespace, do you want to add a space after that comma?

r=mrbkap
Attachment #214138 - Flags: review?(mrbkap) → review+
Checking in regress-329383.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-329383.js,v  <--  regress-329383.js
initial revision: 1.1
Flags: in-testsuite+
Summary: Text size change either via control key or View -> Text SIze fails with Error: uncaught exception: 2147942487 → Text size change either via control key or View -> Text Size fails with Error: uncaught exception: 2147942487
verified fixed trunk 20060328 win/mac/linux
Status: RESOLVED → VERIFIED
OK, clearing blocking1.8.1+ flag based on previous comments.
Flags: blocking1.8.1+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: