Bug 204919 – The compiler thread should not adjust Identifier refCounts.

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

[Mark Lam]

<rdar://problem/57426861>

[Mark Lam]

Created attachment 384960 [details]
proposed patch.

[Saam Barati]

Comment on attachment 384960 [details]
proposed patch.

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

r- because of bug I noticed

> Source/JavaScriptCore/bytecode/GetByStatus.h:98
> +    using IdentifierKeepAlive = std::function<void(Box<Identifier>)>;

let's use WTF::SharedTask so that we don't repeatedly copy around the std::function struct. Instead, we just ref count it.

> Source/JavaScriptCore/dfg/DFGPlan.h:121
> +        m_identifiersKeptAliveForCleanUp.append(WTFMove(identifier));

let's add a branch here:
if (identifier)
    m_identifiersKeptAliveForCleanUp.append(WTFMove(identifier));

So we don't waste memory in the vector for null entries

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:418
> +        m_cancelledPlans.clear();

I think this is wrong, unfortunately. Imagine we're in an API scenario (or web worker scenario). We have multiple VMs. We're now deleting Plans for other VMs. This will lead to the race happening still.

I suggest writing a function like:
deleteCancelledPlansForVM(VM&)

And I'll suggest again what I suggested in person, which is to assert that we're holding the current VM lock.

[Mark Lam]

Created attachment 385055 [details]
proposed patch.

[Saam Barati]

Comment on attachment 385055 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/dfg/DFGPlan.cpp:712
> +    // a cancelled plan needs to keep its m_vm pointer to let the mutator know ]

remove "]"

> Source/JavaScriptCore/dfg/DFGPlan.cpp:717
> +    // Similarly, plans may be cancelled from the GC thread. In that case, the
> +    // treatment is same as the above except that the cancelled plan will be
> +    // kept alive in the worklist's m_keptAliveCancelledPlansFromGCThread.

when the vector's are unified, this paragraph can be omitted

> Source/JavaScriptCore/dfg/DFGPlan.cpp:723
> +    // We rely on this all over the place to filter out Cancelled plans.

you should say what "this" is. Specifically, a bit comparison against VM pointer fails

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:99
> +        bool isInMutator = m_plan->unnukedVM()->currentThreadIsHoldingAPILock();

let's remove this and just always add it to the vector based on what we talked about

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:311
> +void removeCancelledPlans(VM& vm, HashSet<RefPtr<Plan>>& removedPlans, Vector& cancelledPlans)

once we have one vector, let's just put this below

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

once the function is moved in here, you can conditionalize this on !ASSERT_DISABLED

> Source/JavaScriptCore/dfg/DFGWorklist.h:118
> +    Vector<RefPtr<Plan>, 4> m_keptAliveCancelledPlansFromGCThread;
> +    Vector<RefPtr<Plan>, 4> m_keptAliveCancelledPlansFromCompilerThread;

let's join these together and perhaps let's call it  something like "m_cancelledPlansPendingDestruction" or something like that

[Mark Lam]

Created attachment 385074 [details]
patch for landing.

Thanks for the review.  I've applied the changes Saam requested, and also fixed a missing JSLock acquisition in Debugger::detach().  This was causing the inspector tests to fail.

Let's try this on the EWS now.

[Mark Lam]

Landed in r253243: <http://trac.webkit.org/r253243>.

[Mark Lam]

+ Build fix landed in r253257: <http://trac.webkit.org/r253257>.