488808 - TraceRecorder::import's localNames corruption crash

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Attachment: stack backtrace

The attached stack shows a crash I just saw, plus the bogus JSString contents being passed by reference to js_GetStringBytes, make me think something GC'ed away the atoms -- but last-ditch GCs keep atoms. Hrm.

I wonder if bug 487546 changed something to expose this. js_GetStringBytes is very suspicious, but I don't see it atm.

/be

Comment 1

[Brendan Eich [:brendan]]

DEBUG-only, but scary. Will try to repro.

/be

Comment 3

[Rick Stockton]

Sorry, I have a "paperwork" question: Does closing the "critical" 488896 as a dup while leaving this one "normal" make sense?

I've just turned in a few Linux crash reports which might be duplicates, but I don't know how to turn them into stack traces:
http://crash-stats.mozilla.com/report/index/81562829-66e7-46e3-8370-1b9c02090417,
and three duplicate crashes from a few minutes before.

I'm running a tinderbox "regular" build, not a "debug" version.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre - Build ID: 20090417120935

Comment 4

[David Mandelin [:dmandelin]]

Regardless of its official priority, it's currently on the top of my stack.

Comment 5

[Brendan Eich [:brendan]]

Sure, this should be severity critical.

/be

Comment 6

[Andreas Gal :gal]

487546 can't go into trunk until this is fixed.

Comment 7

[Rick Stockton]

Thanks, guys. As Brian found on that other bug, "Tinder Boxen" from earlier in the day are OK, this one doesn't crash on my Linux:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre - Build ID: 20090417065044

Have you got it isolated already, or should I work to get the exact change set were it started breaking?

Comment 8

[David Mandelin [:dmandelin]]

I have the cause of the bug. It's from the changesets for bug 487546 that add a flag to track which strings have entries in the deflated cache. Once set, the flag should never be cleared. JS_GetStringChars clears the flag. I'm working on a fix.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 373418 [details] [diff] [review]
Patch

>diff -r b9af39a5db0a js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp    Fri Apr 17 15:25:25 2009 -0700
>+++ b/js/src/jsapi.cpp    Fri Apr 17 18:06:05 2009 -0700
>@@ -5444,17 +5444,17 @@ JS_GetStringChars(JSString *str)
>      */
>     if (JSSTRING_IS_DEPENDENT(str)) {
>         n = JSSTRDEP_LENGTH(str);
>         size = (n + 1) * sizeof(jschar);
>         s = (jschar *) malloc(size);
>         if (s) {
>             memcpy(s, JSSTRDEP_CHARS(str), n * sizeof *s);
>             s[n] = 0;
>-            JSFLATSTR_INIT(str, s, n);
>+            JSFLATSTR_INIT_SHARABLE(str, s, n);

JSFLATSTR_REINIT.

>+void
>+js_AtomicSetBits(jsword *w, jsword value, jsword mask)
>+{
>+    jsword ov, nv;
>+
>+    do {
>+        ov = *w;
>+        nv = (ov & ~mask) | (value & mask);
>+    } while (!js_CompareAndSwap(w, ov, nv));
>+}

Don't need this -- we only REINIT single-threaded (dependent, mutable strings).

/be

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch 2

As noted by the reviewer, that first try was a bit...misguided (although it did solve this particular bug). This one takes out the unneeded synchronization and adds more REINIT macros to make sure the flag is always preserved.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 373428 [details] [diff] [review]
Patch 2

>+/* 
>+ * Special flat string initializer that preserves the JSSTR_DEFLATED flag.
>+ * Use this macro when reinitializing a new string. Newborn strings must

s/a new string/an existing string/ -- could say " (which may be hashed to its deflated bytes)."

Looks good, let's get it into tracemonkey ASAP -- r?igor for good measure as his timezone allows.

/be

Comment 13

[David Mandelin [:dmandelin]]

Attachment: Patch 3

Pushed to TM as 90bccf6f94e8. I pushed it now per brendan's rec, and to get these asserts fixed before the weekend.

This is a small fix of patch 2, which I noticed did the extra masking on PREFIX_INIT instead of PREFIX_REINIT. I also did the comment revision.

I should also give a more detailed explanation of the bug and fix:

Cause: I introduced the 'deflated' bit for strings so that the expensive purging of the deflated cache entry would be done only if necessary in bug 487546. But the INIT macros clear that bit, causing the deflated cache entry not to be deleted. Then, if later a new string is created with the same address as the old one, JS_GetStringBytes will find the old deflated string cache entry. The assert catches the fact that the entry doesn't actually match the current string.

Fix: I found all the places where INIT macros were called on existing strings. I created REINIT macros that do the same thing, except they don't clear the deflated bit. Then I changed the macro invocations. One question is whether the REINIT macros need synchronization. It appears that they are always called on dependent strings, which I'm told will not be modified from multiple threads.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

(adding fixed-in-tracemonkey as per comment 13)

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/90bccf6f94e8

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2eb22cbd8e89

Comment 18

[Bob Clary [:bc] (inactive)]

e4x/extensions/regress-352846-01.js covered Assertion failure: *bytes == '\0' && JSSTRING_LENGTH(str) == 0

v 1.9.1, 1.9.2