Closed Bug 559565 Opened 10 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q3 11 - Serrano

People

(Reporter: pnkfelix, Assigned: pnkfelix)

References

Details

(Whiteboard: has-patch)

Attachments

(2 files, 9 obsolete files)

2.37 KB, patch
stejohns
: review+
Details | Diff | Splinter Review
2.60 KB, patch
stejohns
: review+
Details | Diff | Splinter Review
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.
Attached file demonstrates assert fail (obsolete) —
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 . . .
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
Changes behavior of delUintProperty to maintain invariant that m_lowHTentry never points to a deleted entry.
(whoops, forgot to look at patch file before uploading it...)
Attachment #439264 - Attachment is obsolete: true
(arrgh bugzilla user error!)
Attachment #439265 - Attachment is obsolete: true
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".)
(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.
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.)
Attachment #439266 - Attachment is obsolete: true
Attachment #439263 - Flags: review?(edwsmith)
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")
Attachment #439263 - Flags: review?(edwsmith)
same refactoring but with revised indentation; happens to make diff clearer.
Attachment #439263 - Attachment is obsolete: true
Attachment #439526 - Flags: review?(edwsmith)
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.
Attachment #439292 - Attachment is obsolete: true
Attachment #439535 - Flags: review?(edwsmith)
Attachment #439526 - Flags: review?(edwsmith) → review+
Attachment #439535 - Flags: review?(edwsmith) → review?(stejohns)
Issue occurs in Coral.
Attachment #439535 - Flags: review?(stejohns) → review+
Assignee: nobody → fklockii
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Rebased to tip (should still be pure refactoring).  I sandbox-built this+forthcoming revised patches.  Feel free to rubber-stamp review.
Attachment #439526 - Attachment is obsolete: true
Attachment #451015 - Flags: review?(edwsmith)
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
Attachment #439535 - Attachment is obsolete: true
Attachment #451016 - Flags: review?(stejohns)
Attached patch regression testSplinter Review
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.
Attachment #439238 - Attachment is obsolete: true
Attachment #451017 - Flags: review?(stejohns)
Attachment #451015 - Flags: review?(edwsmith) → review+
Whiteboard: has-patch
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 451015 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method

push me please.
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
Attachment #451015 - Attachment is obsolete: true
Attachment #451016 - Flags: review?(stejohns) → review+
Attachment #451017 - Flags: review?(stejohns) → review+
Comment on attachment 451016 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent

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

push please
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.