559401 - set/delete sequence assert fails (and breaks in operator)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: if this prints whaa, something is wrong

the following code:

    var arr = []
    
    arr[1] = 1
    delete arr[1]
    
    arr[0] = 1
    delete arr[0]
    
    if (1 in arr)
        print("whaaa")

Prints "whaaa" in a non-debug build (both in tr and tr-argo).

On a debug build, it signals the following assertion failure:

Assertion failed: "((ScriptObject::hasUintProperty (m_lowHTentry)))" ("/Users/fklockii/Dev/tamarin-redux/core/ArrayObject.cpp":80)

I found this while working on bug 559321

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: if this prints whaa, something is wrong

(provide a bit more info about context)

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: if this prints whaa, something is wrong

(oops previous attach was for another ticket)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: if this prints whaa, something is wrong

(... and i managed to screw up the last attachment too, sorry)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

This test case isn't perfect yet, because it should probably be separating modifications to a freshly allocatoin (clean) object/array/etc from modifications to one that has already undergone a series of modifications.  The current test bangs on one entity repeatedly; this is indeed important to test (see bug 559401) but so is the other case.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

The assertion in ArrayObject::checkForSparseToDenseConversion seems bogus: the m_lowHTentry tracks the smallest element in the hash table, but AFAICT that element may be the DELETED sentinel.  So its not okay to assume that ScriptObject::hasUintProperty will be true for such an index.

(There is still the other bug to resolve, but this assertion should go away.)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

As discussed in bug 559565, the assertion need not be bogus, *if* we change the code to maintain the invariant that checkForSparseToDenseConversion is assuming -- namely that m_lowHTentry never points to a deleted sentinel node.

The patches posted to bug 559565 ensure that del*Property enforces this rule, and in the process fix this bug.

Comment 7

[Dan Smith]

Please confirm if this issue occurs in Coral or is an injection.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Issue occurs in Coral.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Since the fix to bug 559565 has been pushed, I think this can be resolved.