Bug 205086 – Worklist::deleteCancelledPlansForVM() should not assume that a cancelled plan is ready for deletion.

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

[Mark Lam]

Another thread may still have a reference to the cancelled plan and hasn't released it yet.

<rdar://problem/57795002>

[Mark Lam]

Created attachment 385319 [details]
proposed patch.

[Saam Barati]

Comment on attachment 385319 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:310
>      HashSet<RefPtr<Plan>> removedPlans;

this shouldn't be a HashSet, right?

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:318
> +        removedPlans.add(WTFMove(plan));

isn't this easier to just append only when ref count is 1?

like:
Plan* plan = m_cancelledPlansPendingDestruction[I].get();
if (plan->refCount() == 1) removedPlans.append(...)

and remove the +1 ref in the above code

[Mark Lam]

Comment on attachment 385319 [details]
proposed patch.

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

>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:310
>>      HashSet<RefPtr<Plan>> removedPlans;
> 
> this shouldn't be a HashSet, right?

This should be a HashSet because (as I explained offline previously for the original patch) there's a chance that the GC thread cancels the plan and appends it to m_cancelledPlansPendingDestruction, and then the compiler thread sees it and appends it to m_cancelledPlansPendingDestruction again before the mutator can iterate m_cancelledPlansPendingDestruction.  As a result, the same plan may show up in m_cancelledPlansPendingDestruction more than once, but we only want to add it to removedPlans once.

>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:318
>> +        removedPlans.add(WTFMove(plan));
> 
> isn't this easier to just append only when ref count is 1?
> 
> like:
> Plan* plan = m_cancelledPlansPendingDestruction[I].get();
> if (plan->refCount() == 1) removedPlans.append(...)
> 
> and remove the +1 ref in the above code

Good point.  I will change the fix.

[Mark Lam]

Comment on attachment 385319 [details]
proposed patch.

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

>>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:318
>>> +        removedPlans.add(WTFMove(plan));
>> 
>> isn't this easier to just append only when ref count is 1?
>> 
>> like:
>> Plan* plan = m_cancelledPlansPendingDestruction[I].get();
>> if (plan->refCount() == 1) removedPlans.append(...)
>> 
>> and remove the +1 ref in the above code
> 
> Good point.  I will change the fix.

I take this back.  Because the plan can appear in m_cancelledPlansPendingDestruction more than once, this scheme won't work.  We have to filter it through the HashSet.

[Saam Barati]

Comment on attachment 385319 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:323
>          RELEASE_ASSERT(plan->stage() == Plan::Cancelled);

should we make this a debug assert like before?

[Mark Lam]

(In reply to Saam Barati from comment #5)
> Comment on attachment 385319 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385319&action=review
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:323
> >          RELEASE_ASSERT(plan->stage() == Plan::Cancelled);
> 
> should we make this a debug assert like before?

I'll change this to a debug assert.  I also added a comment explaining how we can have the same cancelled plan appear more than once in m_cancelledPlansPendingDestruction, and why we need to filter it through the HashSet.  The fact that you asked about it shows that we'll probably forget about this detail in the future.  Hence, a comment is needed to document it for posterity.

[Mark Lam]

Thanks for the review.  Landed in r253358: <http://trac.webkit.org/r253358>.