Bug 232019

Summary: We should watch isHavingABadTime if we read from the structureCache
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: JavaScriptCoreAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-feeder, justin_michaud, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Justin Michaud 2021-10-20 09:30:44 PDT 
rdar://84338462

We should watch isHavingABadTime if we read from the structureCache off the mutator thread
Comment 1 Justin Michaud 2021-10-20 09:45:46 PDT 
Created attachment 441889 [details]
Patch
Comment 2 Yusuke Suzuki 2021-10-20 11:19:47 PDT 
Comment on attachment 441889 [details]
Patch

r=me
Comment 3 Yusuke Suzuki 2021-10-20 11:44:49 PDT 
Comment on attachment 441889 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3131
> +                if (!globalObject->isHavingABadTime())

I think you need to insert load-load-fence just after this. So,

bool isHavingABadTime = globalObject->isHavingABadTime();
WTF::loadLoadFence();
if (!isHavingABadTime)
    m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:846
> +                        if (!globalObject->isHavingABadTime())

Ditto
Comment 4 Justin Michaud 2021-10-20 14:08:50 PDT 
Created attachment 441931 [details]
Patch
Comment 5 EWS 2021-10-20 15:08:52 PDT 
Committed r284576 (243316@main): <https://commits.webkit.org/243316@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441931 [details].
Comment 6 Saam Barati 2021-10-22 11:19:00 PDT 
Comment on attachment 441931 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3138
> +                WTF::loadLoadFence();

Why do we have 2 load load fences?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3140
> +                structure = m_vm.structureCache
> +                    .emptyObjectStructureConcurrently(globalObject, base.getObject(), JSFinalObject::defaultInlineCapacity());

this should not be on multiple lines.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:855
> +                        structure = globalObject->vm().structureCache
> +                            .emptyObjectStructureConcurrently(globalObject, base.getObject(), JSFinalObject::defaultInlineCapacity());

ditto, not 2 lines.
If you really like putting things on 2 lines, you should do it for the arguments, not the "."