Closed Bug 909574 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: gkw, Assigned: bhackett1024)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:])

Attachments

(3 files, 2 obsolete files)

Attached file 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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
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?
Blocks: 906060
Flags: needinfo?(bhackett1024)
WFM on tip.  Can you still reproduce?
Flags: needinfo?(bhackett1024)
(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).
Flags: needinfo?(bhackett1024)
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #797314 - Flags: review?(wmccloskey)
Flags: needinfo?(bhackett1024)
This doesn't affect anything usable in the browser yet.
Group: core-security
Comment on attachment 797314 [details] [diff] [review]
patch

With this patch I still assert identically, and it even asserts with "dis()".
Attachment #797314 - Flags: feedback-
(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.
Hmm strange. Let me retest.
> 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.
Ah, ok.  Can you attach a stack for the new assert?
Flags: needinfo?(bhackett1024)
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: general → bhackett1024
Attachment #797560 - Flags: review?(wmccloskey)
Flags: needinfo?(bhackett1024)
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|.
Attachment #797560 - Flags: review?(wmccloskey)
Attached patch lifoallocSplinter Review
Attachment #797314 - Attachment is obsolete: true
Attachment #797560 - Attachment is obsolete: true
Attachment #797314 - Flags: review?(wmccloskey)
Attachment #798081 - Flags: review?(bhackett1024)
Attachment #798081 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/d6a0066e7081
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.