<html>
<head>
<base href="https://bugs.webkit.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebAssembly: several tests added in r214504 crash when building with GCC"
href="https://bugs.webkit.org/show_bug.cgi?id=170390#c15">Comment # 15</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebAssembly: several tests added in r214504 crash when building with GCC"
href="https://bugs.webkit.org/show_bug.cgi?id=170390">bug 170390</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to Saam Barati from <a href="show_bug.cgi?id=170390#c14">comment #14</a>)
<span class="quote">> (In reply to Carlos Garcia Campos from <a href="show_bug.cgi?id=170390#c12">comment #12</a>)
> > (In reply to Saam Barati from <a href="show_bug.cgi?id=170390#c9">comment #9</a>)
> > > Comment on <span class=""><a href="attachment.cgi?id=306063&action=diff" name="attach_306063" title="Updated patch">attachment 306063</a> <a href="attachment.cgi?id=306063&action=edit" title="Updated patch">[details]</a></span>
> > > Updated patch
> > >
> > > View in context:
> > > <a href="https://bugs.webkit.org/attachment.cgi?id=306063&action=review">https://bugs.webkit.org/attachment.cgi?id=306063&action=review</a>
> > >
> > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94
> > > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable {
> > > >>>
> > > >>> Why not plan=WTFMove(plan)?
> > > >>
> > > >> Because I've removed the line that created the plan to simplify it.
> > > >>
> > > >> RefPtr<Plan> plan = makeRef(p);
> > > >> [plan = WTFMove(plan)]
> > > >>
> > > >> is equivalent to
> > > >>
> > > >> [plan = makeRefPtr(p)]
> > > >>
> > > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash).
> > > >
> > > > Gotcha. I see what's going on.
> > > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle.
> > > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable.
> > >
> > > Seems like you could also keep the code as is, and just use p.vm() instead
> > > of plan->vm().
> >
> > Yes, that's my first patch see <a href="show_bug.cgi?id=170390#c1">comment #1</a>
> I like your first patch. I'm not sure what the semantics are here of your
> capture of plan, since you don't define another variable named "plan".
> Anyways, I'm worried about a reference cycle, which is why I like an
> explicit local variable. See the comment in WasmPlan.h.</span >
But this lambda is not the completion task one. That one is kept as a member of Plan, that's why it can cause a cycle. In this case we are keeping a ref in the lambda even if we use a local variable, because create the ref and then move to the lambda is the same as creating the ref in the lambda capture directly.
<span class="quote">> >
> > > Also, I'm not sure why we make a RefPtr instead of a Ref here.
> >
> > I think it's because then we end up doing Ref<> foo = Ref(); and Ref(const
> > Ref& other) is deleted.
> Why not just do:
> Ref<Plan> plan(p);
> ?</span >
JSWebAssemblyModule::createStub() expects a RefPtr<>&& anyway, so it's simpler this way.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>