All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Q3 11 - Serrano

Status

P3
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: pnkfelix, Assigned: pnkfelix)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug +

Details

(Whiteboard: has-patch)

Attachments

(2 attachments, 9 obsolete attachments)

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.
Created attachment 439238 [details]
demonstrates assert fail
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 . . .
Created attachment 439263 [details] [diff] [review]
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
Created attachment 439264 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent

Changes behavior of delUintProperty to maintain invariant that m_lowHTentry never points to a deleted entry.
Created attachment 439265 [details]
Change delUintProperty to keep m_lowHTentry consistent

(whoops, forgot to look at patch file before uploading it...)
Attachment #439264 - Attachment is obsolete: true
Created attachment 439266 [details] [diff] [review]
Change delUintProperty to keep m_lowHTentry consistent

(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.
Created attachment 439292 [details] [diff] [review]
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.)
Attachment #439266 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
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)
Created attachment 439526 [details] [diff] [review]
refactoring m_lowHTentry update code into its own method

same refactoring but with revised indentation; happens to make diff clearer.
Attachment #439263 - Attachment is obsolete: true
Attachment #439526 - Flags: review?(edwsmith)
Created attachment 439535 [details] [diff] [review]
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.
Attachment #439292 - Attachment is obsolete: true
Attachment #439535 - Flags: review?(edwsmith)

Updated

9 years ago
Attachment #439526 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Attachment #439535 - Flags: review?(edwsmith) → review?(stejohns)
Issue occurs in Coral.

Updated

9 years ago
Attachment #439535 - Flags: review?(stejohns) → review+

Updated

9 years ago
Assignee: nobody → fklockii
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Created attachment 451015 [details] [diff] [review]
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.
Attachment #439526 - Attachment is obsolete: true
Attachment #451015 - Flags: review?(edwsmith)
Created attachment 451016 [details] [diff] [review]
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
Attachment #439535 - Attachment is obsolete: true
Attachment #451016 - Flags: review?(stejohns)
Created attachment 451017 [details] [diff] [review]
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.
Attachment #439238 - Attachment is obsolete: true
Attachment #451017 - Flags: review?(stejohns)

Updated

9 years ago
Attachment #451015 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Whiteboard: has-patch

Updated

9 years ago
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 18

9 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

9 years ago
Attachment #451016 - Flags: review?(stejohns) → review+

Updated

9 years ago
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
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.