Bug 199202 - Add didBecomePrototype() calls to global context prototypes
Summary: Add didBecomePrototype() calls to global context prototypes
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-25 13:56 PDT by Keith Miller
Modified: 2019-06-25 14:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.15 KB, patch)
2019-06-25 13:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (1.08 KB, patch)
2019-06-25 14:36 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-06-25 13:56:58 PDT
Add didBecomePrototype() calls to global context prototypes
Comment 1 Keith Miller 2019-06-25 13:57:54 PDT
Created attachment 372860 [details]
Patch
Comment 2 Saam Barati 2019-06-25 14:02:16 PDT
Comment on attachment 372860 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This fixes some crashes related to checking that all prototypes have been
> +        marked as such to JSC.

What crashes? I thought the assert got rolled out? And we were doing things in another way?
Comment 3 Saam Barati 2019-06-25 14:03:56 PDT
Comment on attachment 372860 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        marked as such to JSC.
> 
> What crashes? I thought the assert got rolled out? And we were doing things in another way?

Do you mean this will crash a future change? Can you be more specific here
Comment 4 Yusuke Suzuki 2019-06-25 14:04:57 PDT
Comment on attachment 372860 [details]
Patch

r=me too, and can we add this `didBecomePrototype()` call before calling setPrototypeWithoutTransition() in JSGlobalObject's m_functionPrototype thing too?
Comment 5 Keith Miller 2019-06-25 14:05:36 PDT
Comment on attachment 372860 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        marked as such to JSC.
>> 
>> What crashes? I thought the assert got rolled out? And we were doing things in another way?
> 
> Do you mean this will crash a future change? Can you be more specific here

The patch in https://trac.webkit.org/changeset/246801 breaks debug builds right now because setPrototypeWithoutTransition looks for isValidPrototype. I'll update the ChangeLog to make this clearer.
Comment 6 Mark Lam 2019-06-25 14:07:05 PDT
Comment on attachment 372860 [details]
Patch

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

> Source/WebCore/bindings/js/JSWindowProxy.cpp:115
> +    properties.didBecomePrototype();
>      prototype->structure(vm)->setPrototypeWithoutTransition(vm, &properties);

I thought we were going too make setPrototypeWithoutTransition() call didBecomePrototype() on the passed in prototype object.  Was there a change of plans?
Comment 7 Mark Lam 2019-06-25 14:12:24 PDT
Comment on attachment 372860 [details]
Patch

restoring Saam's r+.
Comment 8 Keith Miller 2019-06-25 14:14:47 PDT
Comment on attachment 372860 [details]
Patch

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

>> Source/WebCore/bindings/js/JSWindowProxy.cpp:115
>>      prototype->structure(vm)->setPrototypeWithoutTransition(vm, &properties);
> 
> I thought we were going too make setPrototypeWithoutTransition() call didBecomePrototype() on the passed in prototype object.  Was there a change of plans?

No, we were going to do it on Structure::create because we know no objects already have that structure thus we don't need to check for haveABadTime. setPrototypeWithoutTransition() can modify existing objects thus haveABadTime should be enforced.
Comment 9 Keith Miller 2019-06-25 14:19:35 PDT
Committed r246808: <https://trac.webkit.org/changeset/246808>
Comment 10 Radar WebKit Bug Importer 2019-06-25 14:22:05 PDT
<rdar://problem/52133576>
Comment 11 Keith Miller 2019-06-25 14:36:25 PDT
Reopening to attach new patch.
Comment 12 Keith Miller 2019-06-25 14:36:26 PDT
Created attachment 372862 [details]
Patch