Bug 134346

Summary: iOS 8 beta 2 ES6 'Set' clear() broken
Product: WebKit Reporter: Ashley Gullen <ashley>
Component: JavaScriptCoreAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Major CC: benjamin, bunhere, cdumez, commit-queue, gyuyoung.kim, oliver, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: iPhone / iPad   
OS: Other   
URL: http://www.scirra.com/labs/bugs/testset.html
Attachments:
Description Flags
Patch oliver: review+

Description Ashley Gullen 2014-06-26 08:02:28 PDT 
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.
Comment 1 Oliver Hunt 2014-06-26 12:39:42 PDT 
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?
Comment 2 Benjamin Poulain 2014-06-26 13:06:12 PDT 
(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.
Comment 3 Benjamin Poulain 2014-06-26 18:02:46 PDT 
Created attachment 233950 [details]
Patch
Comment 4 Oliver Hunt 2014-06-26 18:42:06 PDT 
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)
Comment 5 Benjamin Poulain 2014-06-26 18:44:41 PDT 
(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.
Comment 6 Benjamin Poulain 2014-06-26 20:21:03 PDT 
Committed r170517: <http://trac.webkit.org/changeset/170517>
Comment 7 Ashley Gullen 2014-07-15 06:57:14 PDT 
Still reproduces in iOS 8 beta 3. Was this pushed back to the next beta?
Comment 8 Benjamin Poulain 2014-07-15 14:38:34 PDT 
(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.