Closed
Bug 559565
Opened 15 years ago
Closed 15 years ago
"set a[1];delete a[1];set a[0]" asserts during Sparse to Dense conv
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P3)
Tamarin Graveyard
Virtual Machine
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.
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Comment 2•15 years ago
|
||
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 . . .
Assignee | ||
Comment 3•15 years ago
|
||
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
Assignee | ||
Comment 4•15 years ago
|
||
Changes behavior of delUintProperty to maintain invariant that m_lowHTentry never points to a deleted entry.
Assignee | ||
Comment 5•15 years ago
|
||
(whoops, forgot to look at patch file before uploading it...)
Attachment #439264 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
(arrgh bugzilla user error!)
Attachment #439265 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
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".)
Assignee | ||
Comment 8•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #439263 -
Flags: review?(edwsmith)
Assignee | ||
Comment 10•15 years ago
|
||
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)
Assignee | ||
Comment 11•15 years ago
|
||
same refactoring but with revised indentation; happens to make diff clearer.
Attachment #439263 -
Attachment is obsolete: true
Attachment #439526 -
Flags: review?(edwsmith)
Assignee | ||
Comment 12•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #439526 -
Flags: review?(edwsmith) → review+
Updated•15 years ago
|
Attachment #439535 -
Flags: review?(edwsmith) → review?(stejohns)
Assignee | ||
Comment 13•15 years ago
|
||
Issue occurs in Coral.
Updated•15 years ago
|
Attachment #439535 -
Flags: review?(stejohns) → review+
Assignee: nobody → fklockii
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Assignee | ||
Comment 14•15 years ago
|
||
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)
Assignee | ||
Comment 15•15 years ago
|
||
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)
Assignee | ||
Comment 16•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #451015 -
Flags: review?(edwsmith) → review+
Updated•15 years ago
|
Whiteboard: has-patch
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 17•15 years ago
|
||
Comment on attachment 451015 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method
push me please.
Comment 18•15 years ago
|
||
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
Updated•15 years ago
|
Attachment #451016 -
Flags: review?(stejohns) → review+
Updated•15 years ago
|
Attachment #451017 -
Flags: review?(stejohns) → review+
Assignee | ||
Comment 19•15 years ago
|
||
Comment on attachment 451016 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent
push please
Assignee | ||
Comment 20•15 years ago
|
||
Comment on attachment 451017 [details] [diff] [review]
regression test
push please
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Flags: flashplayer-bug+
You need to log in
before you can comment on or make changes to this bug.
Description
•