824321 - "Assertion failure: !IsThingPoisoned(thing),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

x = "\udada\udada"

asserts js debug shell on m-c changeset 84320dffec6e without any CLI arguments at Assertion failure: !IsThingPoisoned(thing), when the testcase is passed in as a CLI argument.

s-s because gc is on the stack.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116562:54696b3f852b
user:        Jon Coppeard
date:        Tue Dec 18 13:27:28 2012 +0000
summary:     Bug 820186 - Various crashes/assertions with gczeal(10) and random recursion. r=billm

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> s-s because gc is on the stack.

Also assuming sec-critical unless otherwise shown - please feel free to change if necessary.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, does the regressing changeset seem correct?

Comment 3

[Jeff Walden [:Waldo]]

"\udada\udada"?  Lulz.  I will bet money that that assert's complaining that the string characters contain a free pattern, for a perfectly legitimate reason.  Which would mean IsThingPoisoned is, um, impossible to write, for string characters.  At least not with the current string/character/etc. memory layout.

Comment 4

[Steve Fink [:sfink] [:s:]]

Not s-s. It's just looking at the data pointed at by a JSString* (well, halfway through the JSString structure) and seeing the free memory pattern, because the jschars are stored inline. So it's just a DEBUG assert gone wrong. Maybe the best thing is to overload IsPoisonedThing<JSString*> and always return true when storage is inline?

Comment 5

[Steve Fink [:sfink] [:s:]]

er, always return false, that is. :-)

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think the right thing to do is to change the assert so that it looks like this:

JS_ASSERT_IF(object-contains-free-pattern, object-is-not-in-the-free-list);

Jon, does that sound right to you? That will also allow us to remove the unpoisoning business during allocation.

Comment 7

[Steve Fink [:sfink] [:s:]]

How would that help in this particular case? It's a totally valid live string that happens to contain the free pattern. So object-contains-free-pattern is true, and object-is-not-in-the-free-list is also true.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steve Fink [:sfink] from comment #7)
> How would that help in this particular case? It's a totally valid live
> string that happens to contain the free pattern. So
> object-contains-free-pattern is true, and object-is-not-in-the-free-list is
> also true.

Um, yeah, so it wouldn't assert then :-). Currently the assert is
  JS_ASSERT(!(object-contains-free-pattern));

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Potential fix

Fix as suggested, but it turns out we can't always check whether something is on the free list or not, so the assert is not as strong as I'd like.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 697070 [details] [diff] [review]
Potential fix

Review of attachment 697070 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Heap.h
@@ +997,5 @@
> +    FreeSpan firstSpan(aheader->getFirstFreeSpan());
> +    uintptr_t addr = reinterpret_cast<uintptr_t>(thing);
> +
> +    for (const FreeSpan *span = &firstSpan;;) {
> +        /* If the thing comes fore the current span, it's not free. */

s/fore/before/

@@ +1004,5 @@
> +
> +        /*
> +         * If we find it inside the span, it's dead. We use here "<=" and not
> +         * "<" even for the last span as we know that thing is inside the
> +         * arena. Thus for the last span thing < span->end.

I think this needs a ',' after Thus.

::: js/src/jsgc.cpp
@@ +266,5 @@
>       * We can be called from the background finalization thread when the free
>       * list in the compartment can mutate at any moment. We cannot do any
>       * checks in this case.
>       */
> +    if (IsBackgroundFinalized(getAllocKind()) && !compartment->rt->isHeapBusy())

Good find. I'm guess this isn't directly related, but no need for a separate bug.

::: js/src/jsgcinlines.h
@@ -486,5 @@
>   */
>  
> -template<typename T>
> -static inline void
> -UnpoisonThing(T *thing)

Nice to see this go.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b4a698e036

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3b4a698e036

Comment 13

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/gc/bug-824321.js.