559565 - "set a[1];delete a[1];set a[0]" asserts during Sparse to Dense conv

Status: RESOLVED FIXED

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

Description

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

var arr = []

  arr[1] = 1
  delete arr[1]

  arr[0] = 1

dies in debug builds.

This is because of an overzealous assertion in ArrayObject::checkForSparseToDenseConversion that checks via ScriptObject::hasUintProperty, which will return false for entries that may still have (deleted) entries with sentinel objects in the hashtable.

One fix is to remove the assertion entirely.  Another fix would add a method to the hashtable that allows one to observe if an index maps to the delete-sentinel in the hash table.

Comment 1

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

Attachment: demonstrates assert fail

Comment 2

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

Oh, another possible fix would be to change ArrayObject.cpp::delUintProperty (and possibly delAtomProperty) so that when you delete m_lowHTentry, it updates m_lowHTentry to point to the next highest numeric entry.  That might also fix 559401, but seems a bit more high risk . . .

Comment 3

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

Attachment: refactoring m_lowHTentry update code into its own method

The first step toward changing ArrayObject::delUintProperty to update m_lowHTentry is to make a function that does the updating.  Luckily we already had the appropriate code for doing this lying around in the ArrayObject::checkForSparseToDenseConversion

Comment 4

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

Changes behavior of delUintProperty to maintain invariant that m_lowHTentry never points to a deleted entry.

Comment 5

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

(whoops, forgot to look at patch file before uploading it...)

Comment 6

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

(arrgh bugzilla user error!)

Comment 7

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

Comment on attachment 439266 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent

Questions: 

1. Should the invocation of updateToSucceedingLowHtEntry() be predicated on retval == true?  (probably, based on ScriptObject::deleteAtomProperty.)  Can someone give me an AS example expression where delete[x] returns false?

2. Does delAtomProperty need a similar update?  (It sometimes delegates to the delUintProperty method, but /not/ always, so the answer here might be "yes".)

Comment 8

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

(In reply to comment #7)
> 2. Does delAtomProperty need a similar update?  (It sometimes delegates to the
> delUintProperty method, but /not/ always, so the answer here might be "yes".)

Yep; the need for this is easily exposed by running the same test with -Ojit.

Comment 9

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

Generalization of patch to also update m_lowHTentry from ArrayObject::deleteAtomProperty. (Running the test attached to bug 559321 exposed this problem first, but then even the test attached here exposed the problem once I tried running with -Ojit.)

Comment 10

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

Comment on attachment 439263 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method

(I want to fix the indentation in the patch before it gets reviewed; the refactored code needs to be brought back left by one "tab")

Comment 11

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

Attachment: refactoring m_lowHTentry update code into its own method

same refactoring but with revised indentation; happens to make diff clearer.

Comment 12

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

Resolved my question of whether to predicate update on retval by asserting that it is not necessary.

This (plus attachment 439526 [details] [diff] [review]) should be good to go, and fixes both this bug and bug 559401.

Comment 13

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

Issue occurs in Coral.

Comment 14

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

Attachment: refactoring m_lowHTentry update code into its own method

Rebased to tip (should still be pure refactoring).  I sandbox-built this+forthcoming revised patches.  Feel free to rubber-stamp review.

Comment 15

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

Attachment: Change delUintProperty to keep m_lowHTentry consistent

Rebased to tip (and made code follow a more C++-ish style).  I sandbox-built
this+forthcoming revised patches.  Feel free to rubber-stamp review

Comment 16

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

Attachment: regression test

Stephen, feel free to hand off to someone else if you think others have more free cycles or are better suited.

This patch was part of the sandbox build I did, so hopefully its all ready to be pushed.

Comment 17

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

Comment on attachment 451015 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method

push me please.

Comment 18

[Edwin Smith]

Comment on attachment 451015 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method

TR: http://hg.mozilla.org/tamarin-redux/rev/2d106d2781b1

Comment 19

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

Comment on attachment 451016 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent

push please

Comment 20

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

Comment on attachment 451017 [details] [diff] [review]
regression test

push please

Comment 21

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/84daecd5e2b8