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

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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

[Mark Lam]

<rdar://problem/41357620>

[Mark Lam]

<rdar://problem/42409527>

[Saam Barati]

Created attachment 345414 [details]
patch

[Saam Barati]

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

[Mark Lam]

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.

[Saam Barati]

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.

[Saam Barati]

(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

[Saam Barati]

Created attachment 345417 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 345417 [details]
patch for landing

Clearing flags on attachment: 345417

Committed r234022: <https://trac.webkit.org/changeset/234022>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.