Bug 187836 - Conservatively make Object.assign's fast path do a two phase protocol of loading everything then storing everything to try to prevent a crash
Summary: Conservatively make Object.assign's fast path do a two phase protocol of load...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 187837
  Show dependency treegraph
 
Reported: 2018-07-19 17:56 PDT by Saam Barati
Modified: 2018-07-19 21:48 PDT (History)
13 users (show)

See Also:


Attachments
patch (4.50 KB, patch)
2018-07-19 18:21 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (7.26 KB, patch)
2018-07-19 18:53 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-07-19 17:56:42 PDT
We're crashing here, and my best hypothesis of why is something like:
- we end up transitioning the structure of the source when we do the put (maybe we even end up flattening the dictionary structure, since we end up seeing a null butterfly). This could happen maybe if we fire a watchpoint during that loop.
- I'm not able to reproduce this, but we can guarantee it doesn't transition under us if we first do all loads, then do all stores
Comment 1 Mark Lam 2018-07-19 18:01:33 PDT
<rdar://problem/41357620>
Comment 2 Mark Lam 2018-07-19 18:11:38 PDT
<rdar://problem/42409527>
Comment 3 Saam Barati 2018-07-19 18:21:54 PDT
Created attachment 345414 [details]
patch
Comment 4 Saam Barati 2018-07-19 18:22:58 PDT
Comment on attachment 345414 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        butterfly. Which is curious, because source's Structure indicated that it has

Going to change:
Which is curious => This is curious
Comment 5 Mark Lam 2018-07-19 18:34:18 PDT
Comment on attachment 345414 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:14
> +        - We end up firing a watchpoint when assining to the target (this can happen

/assining/assigning/

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:340
> -            Structure* structure = source->structure(vm);
> -            if (canPerformFastPropertyEnumerationForObjectAssign(structure)) {
> +            if (canPerformFastPropertyEnumerationForObjectAssign(source->structure(vm))) {

I think you should still cache the structure above.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:354
> +                source->structure(vm)->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {

Use the cached structure here.
Comment 6 Saam Barati 2018-07-19 18:37:35 PDT
Comment on attachment 345414 [details]
patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:340
>> +            if (canPerformFastPropertyEnumerationForObjectAssign(source->structure(vm))) {
> 
> I think you should still cache the structure above.

LLVM will CSE this

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:354
>> +                source->structure(vm)->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
> 
> Use the cached structure here.

so it will do this for us. I didn't want someone to make the mistake of writing code that depends on Structure, since the hypothesis here is it will change.
Comment 7 Saam Barati 2018-07-19 18:47:02 PDT
(In reply to Saam Barati from comment #6)
> Comment on attachment 345414 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345414&action=review
> 
> >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:340
> >> +            if (canPerformFastPropertyEnumerationForObjectAssign(source->structure(vm))) {
> > 
> > I think you should still cache the structure above.
> 
> LLVM will CSE this
> 
> >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:354
> >> +                source->structure(vm)->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
> > 
> > Use the cached structure here.
> 
> so it will do this for us. I didn't want someone to make the mistake of
> writing code that depends on Structure, since the hypothesis here is it will
> change.

will => can
Comment 8 Saam Barati 2018-07-19 18:53:10 PDT
Created attachment 345417 [details]
patch for landing
Comment 9 WebKit Commit Bot 2018-07-19 21:47:58 PDT
Comment on attachment 345417 [details]
patch for landing

Clearing flags on attachment: 345417

Committed r234022: <https://trac.webkit.org/changeset/234022>
Comment 10 WebKit Commit Bot 2018-07-19 21:48:00 PDT
All reviewed patches have been landed.  Closing bug.