Bug 188794

Summary: [JSC] Array.prototype.reverse modifies JSImmutableButterfly
Product: WebKit Reporter: sunlili
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: asmqb7, darin, ews-watchlist, fpizlo, keith_miller, ljharb, mark.lam, msaboff, omarandemad, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch sbarati: review+

Description sunlili 2018-08-21 06:11:56 PDT
Executing following code : 
-------------------------------------
var a= [1, 2.2, 3.3];
function Test()
{
    +a;
    a.reverse();
    print(a);
}
Test();
print("BT_FLAG");
-------------------------------------

Output should be :
3.3,2.2,1
BT_FLAG

However, output of JavaScriptCore is :
1,2.2,3.3
BT_FLAG


BT_GROUP
2018/8/21
Comment 1 Yusuke Suzuki 2018-08-24 07:25:22 PDT
Created attachment 348007 [details]
Patch
Comment 2 Mark Lam 2018-08-24 09:53:25 PDT
<rdar://problem/43637041>
Comment 3 Saam Barati 2018-08-24 10:50:14 PDT
Comment on attachment 348007 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.h:873
> +    void ensureWritable(VM& vm)
> +    {
> +        if (isCopyOnWrite(indexingMode()))
> +            convertFromCopyOnWrite(vm);
> +    }

We do this exact pattern in JSObject.cpp a few times. Maybe we can also refactor that code to just call this?
Comment 4 Darin Adler 2018-08-24 21:27:51 PDT
Comment on attachment 348007 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.h:873
>> +    }
> 
> We do this exact pattern in JSObject.cpp a few times. Maybe we can also refactor that code to just call this?

Same goes for four functions in JSArray.cpp.
Comment 5 Yusuke Suzuki 2018-08-24 23:01:26 PDT
Comment on attachment 348007 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSObject.h:873
>>> +    }
>> 
>> We do this exact pattern in JSObject.cpp a few times. Maybe we can also refactor that code to just call this?
> 
> Same goes for four functions in JSArray.cpp.

Sounds good. Fixed.
Comment 6 Yusuke Suzuki 2018-08-27 01:31:49 PDT
Committed r235356: <https://trac.webkit.org/changeset/235356>
Comment 7 i336_ 2018-09-19 06:23:50 PDT
Awesome to hear this is fixed.

There's currently a (mildly) trending submission on Hacker News regarding this bug: https://news.ycombinator.com/item?id=18021835

It would appear that current releases of Safari (apparently on both iOS and macOS) are still on WebKit revisions from before the fix landing.

I'm sure there's lots more detail and discussion in the rdar:// links, but I can't be sure since I cannot view them.

It would be great if a quick TL;DR on progress/status/next steps/instructions etc could be released somewhere.

(As someone who has never really used macOS/iOS, I'm very curious how the fixes will be rolled out - perhaps silent/automatic patch updates? This breaks JS pretty impressively, so I can't see this being slated for the next iOS/macOS major release.)

NB. I found this thread because the bug submitter linked to it - thanks! - https://news.ycombinator.com/item?id=18023508