Closed Bug 595963 Opened 9 years ago Closed 9 years ago

Sand Trap game stops animating

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: dmandelin, Assigned: dmandelin)

References

()

Details

(Whiteboard: [chromeexperiments][fixed-in-tracemonkey])

Attachments

(2 files, 3 obsolete files)

The game at the linked URL stops working. Specifically, it stops animating the sand after I play with it for 5 seconds or so. In the JS console, I get:

  a.grains[h] is undefined

This happens at least as early as 4.0b5.
blocking2.0: --- → ?
Works in TM nightly of 7 May 2010
Fails in TM nightly of 8 May 2010

http://hg.mozilla.org/tracemonkey/pushloghtml?startdate=5%2F6%2F2010&enddate=5%2F8%2F2010
Duplicate of this bug: 596677
I did a non-minified version of this game with some extra debug info in case you want it for testing:

http://scotland.proximity.on.ca/dxr/tmp/sand-trap-chrome.html
Whiteboard: [chromeexperiments]
I wonder if bug 597186 comment 4 is also this bug, where we seem to lose array elements Chrome doesn't.  There's a test case in that bug.
Exception: a.grains[h] is undefined

I get this even with JM and TM disabled.

And another thing:

It seems this game is calculating the JS engine's speed first, then gives the amount of sands wich the JS engine can handle.

It gives me the same amount with JM + TM and just TM and JM+TM disabled, to...
blocking2.0: ? → beta8+
This is breaking when sand falls into the bucket for the first time, on this line:

    for (var h in b.grains) { b.grains[h].update(g); }

So the regression range which involves iterators seems accurate.
This might have to do with deletion during iteration?
Attached file reduced testcase (obsolete) —
Yup, good call. Seems to only happen with nested iterators. Here's a reduced test case.
Attached file testcase #2 (obsolete) —
No nested iteration needed. All you have to do is delete the current element.
Attachment #478456 - Attachment is obsolete: true
Attached file testcase
Okay, here is the final testcase, that *actually* reproduces a valid problem. Array.splice() doesn't seem to update iterators in the way delete does.
Attachment #478473 - Attachment is obsolete: true
Interesting. Yeah, I guess some array functions delete without going through the delete hook. Good catch.
Yeah, exactly. array_splice doesn't call DeleteArrayElement() on dense arrays. Any takers that know this code?
Pretty straight forward fix. Call

bool
js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)

on every index that was deleted (INT_TO_JSID for id). If its a large range it might be worth to do a SuppressDeletedProperties() with a range.
Do we have to do that on all splice calls?  Or only the ones when we have an iterator live (and can we tell the latter?)?
Assignee: general → dmandelin
Attached patch Patch (obsolete) — Splinter Review
Attachment #479957 - Flags: review?(gal)
Comment on attachment 479957 [details] [diff] [review]
Patch

js_SuppressDeletedProperty does an expensive lookup of open iterators. The loop should happen inside of it (find iterator, if its same object remove the range of ids), not outside.
Attached patch PatchSplinter Review
Attachment #479957 - Attachment is obsolete: true
Attachment #479984 - Flags: review?(gal)
Attachment #479957 - Flags: review?(gal)
Comment on attachment 479984 [details] [diff] [review]
Patch

Pretty :)
Attachment #479984 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/97d2c33271e8
Status: NEW → ASSIGNED
Whiteboard: [chromeexperiments] → [chromeexperiments][fixed-in-tracemonkey]
Backed out due to crashtest failures:

  http://hg.mozilla.org/tracemonkey/rev/d2d4a7115949
Whiteboard: [chromeexperiments][fixed-in-tracemonkey] → [chromeexperiments]
Relanded:

  http://hg.mozilla.org/tracemonkey/rev/f1c60a6d07ba

Tinderbox failures the last time appear to have been bogus.
Whiteboard: [chromeexperiments] → [chromeexperiments][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/97d2c33271e8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 624041
You need to log in before you can comment on or make changes to this bug.