426996 - ActionMonkey: Assertion failure: *bytes == '\0' && JSSTRING_LENGTH(str) == 0, at js/src/jsstr.cpp:3240

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JS_GetStringBytes() has a sanity check, which ActionMonkey is flunking:

  /* Try to catch failure to JS_ShutDown between runtime epochs. */
  if (!js_CStringsAreUTF8) {
      JS_ASSERT_IF(*bytes != (char) JSSTRING_CHARS(str)[0],
                   *bytes == '\0' && JSSTRING_LENGTH(str) == 0);
  }

Igor tells me the comment is bogus in these enlightened days of per-runtime deflated string caches.  But the assertion's still good.

The tests don't trigger this, but Minefield hits it eventually, if I actually browse with it.  The string and the bytes are indeed two completely different strings.  Neither is corrupted.  Looks like the first one got GC'd without being removed from the deflated string cache.

This might mean a JSNativeString destructor wasn't properly called or js_PurgeDeflatedStringCache is broken.  printf debugging is in my future.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Nope, diffing ActionMonkey vs. CVS trunk caught my mistake.  The call to js_PurgeDeflatedStringCache() in js_FinalizeString must run whether the string is external or not.  But I was only running it for native strings--which is all you ever have in js/tests.

This is a candidate for C-level JSAPI tests, when I get around to that...

This patch duplicates the code (we have two concrete classes, JSNativeString and JSExternalString, and the Purge now appears in both destructors).  Everything else seemed worse.  Suggestions welcome.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 313725 [details] [diff] [review]
v1

This looks right to me, except that I don't understand why the test for JSFLATSTR_CHARS is needed.  Can't one create an external string with length zero and a garbage chars pointer?  Wouldn't such a string find its way into the deflated string cache if someone called JS_GetStringBytes on it?  So wouldn't it need to be purged?

Comment 3

[Jason Orendorff [:jorendorff]]

You're right.  The JSFLATSTR_CHARS check is parroting the existing code in SpiderMonkey:  (jsstr.c:2645, js_FinalizeStringRT).  This check is fretting about the destructor being called even though post-alloc initialization failed.  Here that's clearly unnecessary, as JSExtendedString's constructor can't fail.

But even if initialization fails, it seems like the check is unnecessary-- an invalid string won't be in the deflated string cache, so the call to Purge is harmless.  So it seems like the check could also be removed from ~JSNativeString--and from CVS trunk SpiderMonkey, for that matter.  I'll ask Igor about it.

For now, I omitted the check in ~JSExtendedString and committed the rest.  

Pushed changeset f46ec6447041 to actionmonkey branch.