Bug 134346 – iOS 8 beta 2 ES6 'Set' clear() broken

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ashley Gullen]

Visit the test URL above. In iOS 8 beta 2, 'Set' is present, but broken. Note the last test logs "FAIL: Set does not contain item3". This means after a call to clear(), the size is 0 but it still returns true when calling has() on one of the items.

Reproduces on iOS 8 in both Safari and the web view. Does not reproduce on Firefox desktop or Firefox Android, IE11, or Chrome with experimental Javascript enabled.

Rating this as 'Major' because the Construct 2 HTML5 game engine (www.scirra.com) already has thousands of games deployed over the web which feature-detect 'Set' and if present, uses it for various internal engine structures; if it is not present, it uses a shim based around setting and deleting object properties. We can work around this in our engine, but the bug has catastrophic consequences for large amounts of existing web content using the Construct 2 engine, since it relies heavily on Sets in crucial parts of the engine to the extent that the broken Set ruins gameplay (objects not destroyed, sounds stuck playing in loops, other unpredictable consequences). Therefore 'Set' either needs to be fixed or removed entirely to prevent breaking lots of games.

[Oliver Hunt]

Sigh.  This is a dumb mistake - MapData::clear is not clearing m_cellKeyedTable - probably fallout from when i fixed the hash-timing attack.  Ben, i'm currently stuck on other things any chance you're able to fix this?

[Benjamin Poulain]

(In reply to comment #1)
> Sigh.  This is a dumb mistake - MapData::clear is not clearing m_cellKeyedTable - probably fallout from when i fixed the hash-timing attack.  Ben, i'm currently stuck on other things any chance you're able to fix this?

Sounds good, I'll have a look.

[Benjamin Poulain]

Created attachment 233950 [details]
Patch

[Oliver Hunt]

Comment on attachment 233950 [details]
Patch

Could you also add tests for Map? (i realise that they both just use MapData, but it would be good to have explicit tests of each in case that ever changes)

[Benjamin Poulain]

(In reply to comment #4)
> (From update of attachment 233950 [details])
> Could you also add tests for Map? (i realise that they both just use MapData, but it would be good to have explicit tests of each in case that ever changes)

Sure. I'll clone the test and adapt it for Map on landing.

Thanks for the review.

[Benjamin Poulain]

Committed r170517: <http://trac.webkit.org/changeset/170517>

[Ashley Gullen]

Still reproduces in iOS 8 beta 3. Was this pushed back to the next beta?

[Benjamin Poulain]

(In reply to comment #7)
> Still reproduces in iOS 8 beta 3. Was this pushed back to the next beta?

I don't track the beta branches, but given the timing it seems normal this did not make it to the stabilization branch.

For now, please try WebKit Nightly to make sure everything works as expected. This patch should make it to the next beta.