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

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 313725 [details] [diff] [review]
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.
Assignee: general → jorendorff
Status: NEW → ASSIGNED
Attachment #313725 - Flags: review?(jim)
(Assignee)

Updated

11 years ago
Blocks: 422706

Comment 2

11 years ago
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?
Attachment #313725 - Flags: review?(jim) → review+
(Assignee)

Comment 3

11 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-

Updated

11 years ago
Duplicate of this bug: 422706
You need to log in before you can comment on or make changes to this bug.