Bug 195827 - [JSC] Owner of watchpoints should validate at GC finalizing phase
Summary: [JSC] Owner of watchpoints should validate at GC finalizing phase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 196314
  Show dependency treegraph
 
Reported: 2019-03-15 15:24 PDT by Mark Lam
Modified: 2019-03-27 19:36 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (8.33 KB, patch)
2019-03-15 15:28 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing. (9.45 KB, patch)
2019-03-15 15:58 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (44.67 KB, patch)
2019-03-24 23:25 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
Patch (24.60 KB, patch)
2019-03-26 22:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (46.85 KB, patch)
2019-03-26 23:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (49.24 KB, patch)
2019-03-27 00:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (50.41 KB, patch)
2019-03-27 00:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (50.41 KB, patch)
2019-03-27 00:30 PDT, Yusuke Suzuki
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-03-15 15:24:24 PDT
m_object in ObjectPropertyCondition may no longer be live by the time the watchpoint fires.

<rdar://problem/48845513>
Comment 1 Mark Lam 2019-03-15 15:28:16 PDT
Created attachment 364857 [details]
proposed patch.
Comment 2 Filip Pizlo 2019-03-15 15:39:39 PDT
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.
Comment 3 Mark Lam 2019-03-15 15:58:39 PDT
Created attachment 364865 [details]
patch for landing.

Thanks for the review.  I've added the FIXMEs.
Comment 4 WebKit Commit Bot 2019-03-15 21:45:03 PDT
Comment on attachment 364865 [details]
patch for landing.

Clearing flags on attachment: 364865

Committed r243032: <https://trac.webkit.org/changeset/243032>
Comment 5 WebKit Commit Bot 2019-03-15 21:45:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Truitt Savell 2019-03-18 09:32:10 PDT
Tracking new failing test in https://bugs.webkit.org/show_bug.cgi?id=195892
Comment 8 Shawn Roberts 2019-03-21 09:53:13 PDT
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
Comment 9 Mark Lam 2019-03-23 20:56:34 PDT
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.
Comment 10 Mark Lam 2019-03-24 23:25:07 PDT
Created attachment 365851 [details]
proposed patch.
Comment 11 Mark Lam 2019-03-24 23:48:13 PDT
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/
Comment 12 Yusuke Suzuki 2019-03-26 21:29:10 PDT
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.
Comment 13 Yusuke Suzuki 2019-03-26 22:05:32 PDT
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.
Comment 14 Yusuke Suzuki 2019-03-26 22:34:47 PDT
Created attachment 366051 [details]
Patch

WIP
Comment 15 Yusuke Suzuki 2019-03-26 23:44:09 PDT
Created attachment 366056 [details]
Patch

WIP
Comment 16 Yusuke Suzuki 2019-03-27 00:03:21 PDT
Created attachment 366058 [details]
Patch

WIP
Comment 17 Yusuke Suzuki 2019-03-27 00:28:29 PDT
Created attachment 366060 [details]
Patch

WIP
Comment 18 Yusuke Suzuki 2019-03-27 00:30:04 PDT
Created attachment 366061 [details]
Patch
Comment 19 Yusuke Suzuki 2019-03-27 00:51:10 PDT
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.
Comment 20 Yusuke Suzuki 2019-03-27 10:53:02 PDT
I'll check JSGlobalObjects' watchpoints too in a follow-up bug.
Comment 21 Saam Barati 2019-03-27 13:16:50 PDT
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?
Comment 22 Yusuke Suzuki 2019-03-27 13:29:38 PDT
Committed r243560: <https://trac.webkit.org/changeset/243560>
Comment 23 Yusuke Suzuki 2019-03-27 13:36:59 PDT
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 :)
Comment 24 Saam Barati 2019-03-27 18:41:36 PDT
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?
Comment 25 Yusuke Suzuki 2019-03-27 19:36:32 PDT
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).