Another thread may still have a reference to the cancelled plan and hasn't released it yet. <rdar://problem/57795002>
Created attachment 385319 [details] proposed patch.
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
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.
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.
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?
(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.
Thanks for the review. Landed in r253358: <http://trac.webkit.org/r253358>.