808483 - "Assertion failure: chars >= base->chars() && chars < base->chars() + base->length(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

pSandbox = newGlobal('new-compartment');
evalcx("\
    x = ArrayBuffer;\
    y = Map();\
    x += 1;\
    w = x;\
    x += '0';\
    z = x;\
", pSandbox);
evalcx("\
    x + '0';\
", pSandbox);
evalcx("\
    y.delete(z);\
    w.slice(2);\
", pSandbox)

asserts js debug shell on m-c changeset 2937fd8e35a1 without any CLI arguments at Assertion failure: chars >= base->chars() && chars < base->chars() + base->length(),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   107556:326ee1d5c9b0
user:        Terrence Cole
date:        Tue Aug 28 10:28:19 2012 -0700
summary:     Bug 785551 - Do not allow inline JSString chars in the frontend; r=luke

Comment 1

[Terrence Cole [:terrence]]

Marking S-S until I investigate because the assertion is an out-of-range access attempt.

Comment 2

[Terrence Cole [:terrence]]

Unmarking s-s, since this turned out not to be s-s after all.  Patch coming as soon as jit-tests finish running.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0

The assertion that the chars pointer passed to JSDependentString::new_ be within the most base dependent string is wrong if any string in the chain of dependent strings has been undepended.  Instead, we want to assert that the chars we are pointing into are kept alive by some string in the list of base strings, not just the one we happened to land on.

Luke: I also rolled in a trivial bug fix for the case where we want to create a short string.  I think I clearly just made a mistake there because the test should clearly depend on length and not on base->length().

Comment 4

[Terrence Cole [:terrence]]

Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=1d089d041af9

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 678501 [details] [diff] [review]
v0

r+ on all the changes.  I'd split the short-string change into a separate patch (rs=me), though, for the purpose of later bisection if this moves the needle on perf.  (Conceivably it could (for the negative) since short-strings can entail more copying (esp. on 64 bit).)

Comment 6

[Terrence Cole [:terrence]]

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3c629e056a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f11212f4869

Green (folded) try run at:
https://tbpl.mozilla.org/?tree=Try&rev=1d089d041af9

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f11212f4869

Comment 8

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.