Bug 188794 - [JSC] Array.prototype.reverse modifies JSImmutableButterfly
Summary: [JSC] Array.prototype.reverse modifies JSImmutableButterfly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-21 06:11 PDT by sunlili
Modified: 2018-09-19 12:17 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2018-08-24 07:25 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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