Bug 180274

Summary: Having a bad time needs to handle ArrayClass indexing type as well
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
mark.lam: review+
patch for landing none

Description Saam Barati 2017-12-01 13:21:06 PST 
...
Comment 1 Saam Barati 2017-12-01 13:27:58 PST 
<rdar://problem/35667869>
Comment 2 Saam Barati 2017-12-01 13:29:29 PST 
Created attachment 328147 [details]
patch
Comment 3 Keith Miller 2017-12-01 13:32:07 PST 
Comment on attachment 328147 [details]
patch

r=me.
Comment 4 Mark Lam 2017-12-01 13:33:08 PST 
Comment on attachment 328147 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328147&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:1611
> +        switchToSlowPutArrayStorage(vm);

Isn't this infinitely recursing?
Comment 5 Mark Lam 2017-12-01 13:34:03 PST 
Comment on attachment 328147 [details]
patch

oops, didn't mean to set the r? flag
Comment 6 Mark Lam 2017-12-01 13:40:12 PST 
Comment on attachment 328147 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328147&action=review

r=me too.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1611
>> +        switchToSlowPutArrayStorage(vm);
> 
> Isn't this infinitely recursing?

OK, I see what's happening.  The ensureArrayStorage() ensures that the indexingType is no longer ArrayClass before recursing.  Nothing to see here.  Moving along.
Comment 7 Mark Lam 2017-12-01 13:42:04 PST 
Comment on attachment 328147 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328147&action=review

> Source/JavaScriptCore/ChangeLog:13
> +        in non empty indexing types as broken, instead of having to opt out all

I suggest replacing "non empty" with "non-empty" to be consistent with your usage below.  Plus it's easier to read.
Comment 8 Saam Barati 2017-12-01 13:59:32 PST 
Created attachment 328153 [details]
patch for landing
Comment 9 WebKit Commit Bot 2017-12-01 15:40:16 PST 
Comment on attachment 328153 [details]
patch for landing

Clearing flags on attachment: 328153

Committed r225423: <https://trac.webkit.org/changeset/225423>
Comment 10 WebKit Commit Bot 2017-12-01 15:40:18 PST 
All reviewed patches have been landed.  Closing bug.