909574 - Assertion failure: latest == first, at ds/LifoAlloc.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

try {
    gcPreserveCode()
} catch (e) {}
for each(w in this) {
    try {
        'a'.replace(/a/, w)
    } catch (e) {}
}

asserts js debug (64-bit threadsafe deterministic) shell on m-c changeset 17143a9a0d83 with --ion-eager at Assertion failure: latest == first, at ds/LifoAlloc.cpp

Setting s-s because gcPreserveCode is in the testcase.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/a155905a9d08
user:        Brian Hackett
date:        Thu Aug 22 07:13:18 2013 -0600
summary:     Bug 906060 - Allow ExclusiveContext zones to have TI enabled, r=billm.

Brian, is bug 906060 a possible regressor?

Comment 3

[Brian Hackett [Laid off!]]

WFM on tip.  Can you still reproduce?

Comment 4

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

(In reply to Brian Hackett (:bhackett) from comment #3)
> WFM on tip.  Can you still reproduce?

Yes, I can still reproduce with --enable-debug --enable-optimize --enable-more-deterministic --enable-threadsafe on rev dc7b76fcf7e4 (current m-c tip).

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

I still can't repro, but looking at the stack the problem seems to be that we're using transferFrom to add chunks to a non-empty LifoAlloc.  In principle this is fine, but the transferFrom code is somewhat broken and doesn't update |latest| properly --- the other LifoAlloc code assumes that chunks after |latest| are unused.

Comment 6

[Brian Hackett [Laid off!]]

This doesn't affect anything usable in the browser yet.

Comment 7

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

Comment on attachment 797314 [details] [diff] [review]
patch

With this patch I still assert identically, and it even asserts with "dis()".

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #7)
> Comment on attachment 797314 [details] [diff] [review]
> patch
> 
> With this patch I still assert identically, and it even asserts with "dis()".

Well, the patch removed that assert entirely, so I'm not sure what you're seeing.

Comment 9

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

Hmm strange. Let me retest.

Comment 10

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

> Well, the patch removed that assert entirely, so I'm not sure what you're
> seeing.

Instead of triggering this assert:

http://hg.mozilla.org/mozilla-central/annotate/21b5af569ca2/js/src/ds/LifoAlloc.cpp#l127

It's now triggering the following identical assert at another line:

http://hg.mozilla.org/mozilla-central/annotate/21b5af569ca2/js/src/ds/LifoAlloc.cpp#l143

So this should explain it.

Comment 11

[Brian Hackett [Laid off!]]

Ah, ok.  Can you attach a stack for the new assert?

Comment 12

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

Attachment: Stack for assertion at a separate line

Comment 13

[Brian Hackett [Laid off!]]

Attachment: patch

The assert in transferUnusedFrom is also bogus, as multiple LifoAllocs could be transferred into the freeLifoAlloc.  The attached patch fixes this and is a bit smarter about what it sets |latest| to after transferring.

Comment 14

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

Comment on attachment 797560 [details] [diff] [review]
patch

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

It seems like this patch has a few problems, so I ended up rewriting it just to figure out what to do. Let's just use that if you're okay with it.

::: js/src/ds/LifoAlloc.cpp
@@ +129,5 @@
>      if (!other->first)
>          return;
>  
>      incrementCurSize(other->curSize_);
> +    append(other->first, other->latest, other->last);

In the case where |this| and |other| have only unused blocks, this seems wrong. We'll set this->latest to other->first, when really we want to set it to this->first.

@@ +159,5 @@
>              }
>          }
>  
> +        // All the chunks being appended are unused, so preserve our |latest|.
> +        append(other->latest->next(), NULL, other->last);

I think this will be wrong if |this| has no blocks in it. In that case |this->latest| will end up as NULL even when we have appended stuff in from |other|.

Comment 15

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

Attachment: lifoalloc

Comment 16

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

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

Comment 17

[Ed Morley [:emorley]]

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