Bug 195827 – [JSC] Owner of watchpoints should validate at GC finalizing phase

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

m_object in ObjectPropertyCondition may no longer be live by the time the watchpoint fires.

<rdar://problem/48845513>

[Mark Lam]

Created attachment 364857 [details]
proposed patch.

[Filip Pizlo]

Comment on attachment 364857 [details]
proposed patch.

I’m fine with this fix but please file a bug about how we shouldn’t have watchpoints that fire after death and add a fixme at all isStillLive checks that links to that bug.

[Mark Lam]

Created attachment 364865 [details]
patch for landing.

Thanks for the review.  I've added the FIXMEs.

[WebKit Commit Bot]

Comment on attachment 364865 [details]
patch for landing.

Clearing flags on attachment: 364865

Committed r243032: <https://trac.webkit.org/changeset/243032>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/243032/webkit

has caused inspector/model/remote-object.html

to become a near constant failure.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object.html

Diff:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r243061%20(11964)/inspector/model/remote-object-pretty-diff.html

[Truitt Savell]

Tracking new failing test in https://bugs.webkit.org/show_bug.cgi?id=195892

[Shawn Roberts]

It appears that changes in https://trac.webkit.org/changeset/243032

Are causing a JSC failure on the Debug queue

microbenchmarks/data-view-accesses-2.js.ftl-no-cjit-small-pool

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2348/steps/jscore-test/logs/stdio

Regression range is 243030 to 243033

[Mark Lam]

The fix is incorrect: it relies on being able to determine liveness of an object in an ObjectPropertyCondition based on the state of the object's MarkedBit.  However, there's no guarantee that GC has run and that the MarkedBit is already set if the object is live.  As a result, we may not re-install adaptive watchpoints based on presumed dead objects which are actually live.

I'm going to roll this out, and implement a more comprehensive fix to handle watchpoint liveness.

[Mark Lam]

Created attachment 365851 [details]
proposed patch.

[Mark Lam]

Comment on attachment 365851 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:21
> +        responsibility of each Watchpoint subclass to override it to check for if any

/check for if/check if/

> Source/JavaScriptCore/ChangeLog:22
> +        cells it references is till alive or not.

/is till/is still/

> Source/JavaScriptCore/ChangeLog:43
> +        reference in the watchpoints.  I opted not to go fo this approach because it will

/reference/referenced/
/go fo/go for/

> Source/JavaScriptCore/ChangeLog:50
> +        Preliminary perf measurements on JetStream via the CLU shows a possible .36%

/CLU/CLI/

[Yusuke Suzuki]

Rough thought. I'm now start considering about why these watchpoints are not destroyed.
I think we have only several Cells that are allowed to hold watchpoints (not WatchpointSet).
CodeBlock, JSGlobalObject, FunctionRareData, etc. If they manage these watchpoints lifetime correctly, I think we can destroy watchpoints before we go into super crazy state like having dead objects.
I need to check this is right even with the incremental sweeper, but maybe, it would be OK.

From the viewpoint of the above thought, AllocationProfileClearingWatchpoint's implementation is right. It only holds FunctionRareData*, and this watchpoint itself is managed by this cell.
ObjectToStringAdaptiveStructureWatchpoint is obviously wrong. Nobody is responsible to m_key's liveness.
DFG::AdaptiveStructureWatchpoint should be right. Its object cell should be held as weak/strong by DFG code, and CodeBlock should maintain this liveness, and it should destroy this watchpoint if this object cell becomes dead.
I believe StructureStubClearingWatchpoint is right. Its object cell should be managed by PolymorphicAccess. I need to look into it deeply but I think it should be right. And if it has a bug, fixing it should be easy.
LLIntPrototypeLoadAdaptiveStructureWatchpoint should be right because it is managed by CodeBlock. Actually, we are reaping dead watchpoint in CodeBlock::finalizeLLIntInlineCaches.
CodeBlockJettisoningWatchpoint is obviously right, managed by CodeBlock.
ObjectPropertyChangeAdaptiveWatchpoint is only used by JSGlobalObject. I think we should rename ObjectPropertyChangeAdaptiveWatchpoint to JSGlobalObjectPropertyChangeAdaptiveWatchpoint or something like that, and we should insert liveness check of JSGlobalObject here. And it should be safe.

I need to check the other watchpoints, but I think we do not need to iterate all the watchpoints.

[Yusuke Suzuki]

These watchpoints' fire procedure should be guarded by owner cell's isLive check. Let's consider the following case,

Watchpoint
    JSCell* owner;
    ObjectConditionProperty m_key;

This watchpoint can be live until owner cell's destructor is called. And m_key's object's destructor can be called earlier because GC does not have any restriction on the order of destructions. As long as we check owner->isLive(), we can ensure that this watchpoint is correctly maintained by the owner.

[Yusuke Suzuki]

Created attachment 366051 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 366056 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 366058 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 366060 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 366061 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 366061 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:78
> +        * runtime/ArrayBufferNeuteringWatchpointSet.h: Renamed from Source/JavaScriptCore/runtime/ArrayBufferNeuteringWatchpoint.h.

This is renaming. It is not Watchpoint, it is WatchpointSet.

[Yusuke Suzuki]

I'll check JSGlobalObjects' watchpoints too in a follow-up bug.

[Saam Barati]

Comment on attachment 366061 [details]
Patch

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

r=me too

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp:39
> +    if (!m_holder.isValid())

why not call it m_owner?

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:82
> +    bool isValid() const;

why not call it isLive() in line with your naming elsewhere?

[Yusuke Suzuki]

Committed r243560: <https://trac.webkit.org/changeset/243560>

[Yusuke Suzuki]

Comment on attachment 366061 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp:39
>> +    if (!m_holder.isValid())
> 
> why not call it m_owner?

I used the original field name but we can consistently use the name "owner", I'll fix it in a follow-up patch :D

>> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:82
>> +    bool isValid() const;
> 
> why not call it isLive() in line with your naming elsewhere?

I think the name "isValid()" is nice because it means that this function name indicates that this returns "isValid" boolean.
Checking the liveness of the owner is the implementation of validity checking, and I think watchpoints can be invalid due to the other reasons.
And AdaptiveInferredPropertyValueWatchpointBase uses "isValid()" for this purpose already. So I like "isValid()" name here :)

[Saam Barati]

Comment on attachment 366061 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:82
>>> +    bool isValid() const;
>> 
>> why not call it isLive() in line with your naming elsewhere?
> 
> I think the name "isValid()" is nice because it means that this function name indicates that this returns "isValid" boolean.
> Checking the liveness of the owner is the implementation of validity checking, and I think watchpoints can be invalid due to the other reasons.
> And AdaptiveInferredPropertyValueWatchpointBase uses "isValid()" for this purpose already. So I like "isValid()" name here :)

But this is so close to the name isStillValid() 

I think it's confusing to have two methods you can invoke on this type, "isValid()" and "isStillValid()". How am i supposed to know the difference just based on their names?

[Yusuke Suzuki]

Comment on attachment 366061 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:82
>>>> +    bool isValid() const;
>>> 
>>> why not call it isLive() in line with your naming elsewhere?
>> 
>> I think the name "isValid()" is nice because it means that this function name indicates that this returns "isValid" boolean.
>> Checking the liveness of the owner is the implementation of validity checking, and I think watchpoints can be invalid due to the other reasons.
>> And AdaptiveInferredPropertyValueWatchpointBase uses "isValid()" for this purpose already. So I like "isValid()" name here :)
> 
> But this is so close to the name isStillValid() 
> 
> I think it's confusing to have two methods you can invoke on this type, "isValid()" and "isStillValid()". How am i supposed to know the difference just based on their names?

Talked with Saam. Watchpoint does not have "isStillValid()" method (it is defined in WatchpointSet).