451187 - JS causes a crash in nspr [@ Bfree ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Brooks [:db48x]]

The crash was uncovered in code I'm writing for Fennec, which is a xulrunner app. Here's the top of the stack:

#4  <signal handler called>
#5  Bfree (v=0x7f394e6a7570) at /home/db48x/moz/mozilla-central/nsprpub/pr/src/misc/prdtoa.c:625
#6  0x00007f394e3fc89a in dtoa (d=-0, mode=0, ndigits=0, decpt=0x7fff573c9404, sign=0x7fff573c9400, rve=0x7fff573c93f8) at /home/db48x/moz/mozilla-central/js/src/dtoa.c:2713
#7  0x00007f394e3fd709 in JS_dtostr (buffer=0x7fff573c9480 "\210\226<W�\177", bufferSize=26, mode=DTOSTR_STANDARD, precision=0, d=-0) at /home/db48x/moz/mozilla-central/js/src/jsdtoa.cpp:162
#8  0x00007f394e42ed4a in js_NumberToCString (cx=0x115aa20, d=0, buf=0x7fff573c9480 "\210\226<W�\177", bufSize=26) at /home/db48x/moz/mozilla-central/js/src/jsnum.cpp:720
#9  0x00007f394e42ed69 in js_NumberToString (cx=0xcca1a0, d=11244299.51238361) at /home/db48x/moz/mozilla-central/js/src/jsnum.cpp:735
#10 0x00007f394e460330 in js_ValueToString (cx=0x115aa20, v=15516762) at /home/db48x/moz/mozilla-central/js/src/jsstr.cpp:2729
#11 0x00007f394e42250d in js_Interpret (cx=0x115aa20) at /home/db48x/moz/mozilla-central/js/src/jsinterp.cpp:3630
#12 0x00007f394e428a91 in js_Invoke (cx=0x115aa20, argc=1, vp=0x11cdfc0, flags=0) at /home/db48x/moz/mozilla-central/js/src/jsinterp.cpp:1320

The JS stack indicates that calling the following function causes the crash:

function hideTablist() { tablist.left = -tablist.boxObject.width; }

From the stack, I would say that tablist.boxobject.width is 0, since dtoa is being called with a -0.

This is with a xulrunner compiled from this morning's mozilla-central, though I first saw the crash a few days ago. This is a 64-bit linux build.

Comment 1

[Brian Crowder]

Ugh!  Somehow dtoa.c is ending up linking to nspr's Bfree, even though it has its own!  Bleah.  I'll check this out as soon as I can.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

> dtoa is being called with a -0.

A negative zero?  Is this on a ones-complement CPU?

Ones-complement CPUs have separate representations (bit patterns) for positive zero and negative zero.  Twos-complement CPUs, such as Intel x86, have no separate representation for negative zero.  On them, zero is zero.

The last ones complement machine on which I worked was a CDC 6500, now sitting
in a computer museum.  If any CPUs made this millennium are ones-complement, I'd like to know what they are.

I'd be interesting in knowing how gdb decided it was a negative zero on a 
twos-complement machine.

Comment 3

[Brendan Eich [:brendan]]

Nelson: -0 is defined by IEEE 754 binary and 754r decimal specs. It's negative 0, useful, e.g., with atan2(y, x) -- a JS demo:

js> Math.atan2(0,0)
0
js> Math.atan2(0,-0)
3.141592653589793
js> Math.atan2(-0,-0)
-3.141592653589793
js> Math.atan2(-0,0)  
0

/be

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Oh, that number is a float/double?  (I don't remember what dtoa does, 
guess I could have looked it up.)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Perhaps the shared library containing dtoa needs to be linked with 
  -B symbolic

Is this really an NSPR bug?

Comment 6

[Benjamin Smedberg]

db48x: what compiler is this being compiled with? Bfree is "static" in both js/src and NSPR, so having them mix symbols is highly unusual.

Comment 7

[Daniel Brooks [:db48x]]

gcc version 4.3.0 20080428 (Red Hat 4.3.0-8) (GCC)

Comment 8

[Daniel Brooks [:db48x]]

This patch of crowder's "fixes" it:

diff -r 85c82edb887a js/src/jsdtoa.cpp
--- a/js/src/jsdtoa.cpp Mon Aug 18 20:49:32 2008 -0700
+++ b/js/src/jsdtoa.cpp Thu Aug 21 11:08:05 2008 -0500
@@ -88,6 +88,11 @@
 #define LOCK_DTOA()
 #define UNLOCK_DTOA()
 #endif
+
+#define Bfree __internal_Bfree
+#define Balloc __internal_Balloc
+#define freedtoa __internal_freedtoa
+
 #include "dtoa.c"
 
 JS_FRIEND_API(JSBool)

Comment 10

[Brendan Eich [:brendan]]

Need an assignee.

/be

Comment 11

[Brian Crowder]

Wasn't this resolved as a build-bug on db's end?

Comment 12

[Robert Sayre]

Assigned to crowder for now, since he has both a patch and a root cause, it seems.

Comment 13

[Brian Crowder]

Daniel:  Still see this?

Comment 14

[Brian Crowder]

No reply in a month, closing INCO.  Please reopen if you can provide more info.

Comment 15

[timeless]

I've seen this internally @nokia, it'd be nice if we could fix the build system(s) to make this harder to trigger. Debugging it was a huge waste of my time (I hope that the engineer watching me do the debugging learned something but...)

Comment 16

[timeless]

Attachment: mark freedtoa as static

so, while trying to get wtc's static build working, i hit this and spent a bit more time on it. i'm tired of spending time on this. everything else in these files from dtoa is marked as static. and the way we use dtoa, the data we 'return' is not supposed to be freed with 'freedtoa' so it is truly static.

the js version (here) is slightly different in that jsdtoa #include's dtoa. Whereas nspr's version seems to just have an inlined copy (see next patch).

Comment 17

[timeless]

Attachment: mark freedtoa as static

this is the equivalent patch for nspr. again, PR_dtoa manages its own memory in a way which is independent of the memory managed by dtoa itself. freedtoa should be marked as static just like all the other private functions in this file.

Comment 18

[Wan-Teh Chang]

Comment on attachment 421351 [details] [diff] [review]
mark freedtoa as static

r=wtc.  Yes, if dtoa() is static, so should freedtoa().

Comment 19

[timeless]

http://hg.mozilla.org/mozilla-central/rev/7f72dea3d949
http://hg.mozilla.org/mozilla-central/rev/d64fd027e4ad

I'll let ted resolve this with the commit message for nspr CVS (which is locked...)

Comment 20

[Wan-Teh Chang]

timeless: NSPR and NSS changes should be pushed to mozilla-central
using the procedure at
https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central

Comment 21

[timeless]

brother. so you fix it.

Comment 22

[Wan-Teh Chang]

Comment on attachment 421351 [details] [diff] [review]
mark freedtoa as static

I checked in this patch on the NSPR trunk (NSPR 4.8.4).

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.14; previous revision: 4.13
done

Comment 23

[Matthew Gertner]

Looks like I'm having this problem with Prism as well. I can patch my tree locally but was wondering if the fix will make it into 1.9.2.

Comment 24

[timeless]

Comment on attachment 421350 [details] [diff] [review]
mark freedtoa as static

wtc: how does one pull in a fix into 1.9.2.x?

Comment 25

[Wan-Teh Chang]

timeless: we use the same procedure as mozilla-central
(https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central)
except that the NSPR CVS tag needs to be an RTM (final)
release, or a Beta release if there is time to switch
to an RTM tag subsequently.

Ted or I can generate a NSPR_4_8_4_BETA1 or NSPR_4_8_4_RTM
tag for you.  Just curious: why is this bug suddenly becoming
critical?  Was there some new Mozilla code that manifests
this bug?

Comment 26

[Brian Crowder]

I was wondering that myself; I think what's happened is that this bug has emerged as being one of the major issues with a recent release of Prism.  It's probably also one of those things that's causes lots of poorly-understood crashes elsewhere.

Comment 27

[timeless]

roughly, every embedder hits this (my team, prism, etc.). my team hits it once every 6 months. it's an interesting learning experience and a total waste of my time.

i personally hit this trying to build your static nspr-nss stuff, we could call your code 'new' i suppose :).

Comment 28

[Matthew Gertner]

Not sure what you mean by "embedder" but Prism is a XULRunner app.

Personally I'm not sure the fix in this bug gets at the root cause of the issue. How come Firefox on Linux doesn't have the same problem?

Comment 29

[timeless]

it's roughly picking some random set of build flags for some 'static' build which will trigger this. whatever build flags firefox on linux *normally* uses doesn't trigger it. but it's certainly possible to trigger it w/ firefox on linux....

i'd rather not spend time chasing "why does this happen", it happens because two modules have the same function name which should have been declared static, and that's what this bug fixes.

Comment 30

[Matthew Gertner]

Ah ok. For some reason when I first checked I only saw the patch that renames the functions in JS using #defines. Declaring as static seems like a clean solution.

Comment 31

[(not currently active) Ted Mielczarek]

This is fixed in NSPR CVS.

Comment 32

[Johnathan Nightingale [:johnath]]

Comment on attachment 421350 [details] [diff] [review]
mark freedtoa as static

Didn't make 1.9.2.2 - this can be nominated for 1.9.2.3 and it's worth fixing crashes, but can you tell me whether this will be fixed by taking the new NSPR tag in bug 533983? If so, no need to renom, but please mark this bug's dependency on that one.

Comment 33

[Wan-Teh Chang]

Jonathan: you need to use the NSPR_4_8_4_RTM CVS tag instead.
That tag has been tested in mozilla-central for several weeks
now.  NSPR_4_8_4_RTM contains all the changes in the tag in
bug 533983.

Comment 34

[Matthew Gertner]

Just wanted to check whether/where this was slated to land. I'm still getting this crash on Linux from a fresh 1.9.2 pull.

Comment 35

[Wan-Teh Chang]

Matthew: You're right. I found that neither the JS nor the
NSPR patch in this bug has been checked in to mozilla-1.9.2.

Can someone set the appropriate approval/blocking flags to
start the process?  I'm not up to date on the mozilla-1.9.2
status.

Note: mozilla-1.9.2 now has a third copy of freedtoa, in
ipc/chromium/src/base/third_party/dmg_fp/dtoa.cc.  It's defined
inside a "dmg_fp" namespace, so it is OK.

Comment 36

[Daniel Veditz [:dveditz]]

We'll take the JS patch here on the stable branches, and push to upgrade NSPR in bug 575620

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 421350 [details] [diff] [review]
mark freedtoa as static

Approved for 1.9.2.8, a=dveditz for release-drivers

Comment 38

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5d60b8125b69

Comment 39

[Al Billings [:abillings - ex-MoCo]]

What are the STR for the original crash so QA can verify the fix?

Comment 40

[timeless]

I'm not sure there are particularly good steps. The easiest thing to do is to check to see if the function in question is exported, if it isn't exported, then this can't happen = verified.

Comment 41

[Wan-Teh Chang]

Al: I suggest you verify this by code inspection.
Using this MXR query:
http://mxr.mozilla.org/mozilla1.9.2/ident?i=freedtoa&filter=
verify that the three copies of freedtoa() are all
defined as 'static'.

Note: nsprpub/pr/src/misc/dtoa.c is not used, so
ignore that file.

The freedtoa() in
ipc/chromium/src/base/third_party/dmg_fp/dtoa.cc
is not static.  I think that's fine.  You can make it
static as well to be safe.

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 in source using mxr.