Bug 170390 - WebAssembly: several tests added in r214504 crash when building with GCC
Summary: WebAssembly: several tests added in r214504 crash when building with GCC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 170391
  Show dependency treegraph
 
Reported: 2017-04-02 09:54 PDT by Carlos Garcia Campos
Modified: 2017-04-05 00:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2017-04-02 09:56 PDT, Carlos Garcia Campos
sbarati: review+
Details | Formatted Diff | Diff
Updated patch (3.62 KB, patch)
2017-04-02 23:03 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-04-02 09:54:08 PDT
The pattern foo->bar([f = WTFMove(foo)]); crashes when building with GCC, I assume the move happens before the foo is used to invoke the function.

Thread 1 (Thread 0x7ff8237ff700 (LWP 9200)):
#0  0x00007ffa7b5071e6 in std::_Function_handler<void (JSC::Wasm::Plan&), JSC::compileAndInstantiate(JSC::VM&, JSC::ExecState*, JSC::JSPromiseDeferred*, JSC::JSValue, JSC::JSObject*)::{lambda(JSC::Wasm::Plan&)#1}>::_M_invoke(std::_Any_data const&, JSC::Wasm::Plan&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#1  0x00007ffa7b4d101b in JSC::Wasm::Plan::complete(WTF::AbstractLocker const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007ffa7b4d3e0b in JSC::Wasm::Plan::parseAndValidateModule() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#3  0x00007ffa7b4f8098 in JSC::Wasm::Worklist::Thread::work() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#4  0x00007ffa7b5760ef in std::_Function_handler<void (), WTF::AutomaticThread::start(WTF::AbstractLocker const&)::{lambda()#1}>::_M_invoke(std::_Any_data const&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#5  0x00007ffa7b589705 in WTF::threadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#6  0x00007ffa7b5bb73a in WTF::wtfThreadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007ffa77c0e424 in start_thread (arg=0x7ff8237ff700) at pthread_create.c:333
#8  0x00007ffa76a209bf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105
Comment 1 Carlos Garcia Campos 2017-04-02 09:56:54 PDT
Created attachment 306054 [details]
Patch
Comment 2 JF Bastien 2017-04-02 09:57:22 PDT
That's not good! Would you want to prepare a patch to fix it?
Comment 3 JF Bastien 2017-04-02 09:58:36 PDT
(In reply to JF Bastien from comment #2)
> That's not good! Would you want to prepare a patch to fix it?

Ha, we raced!


I think it would be better to get `VM& vm` from plan, and then use it, instead of doing p->vm().
Comment 4 Carlos Garcia Campos 2017-04-02 22:45:47 PDT
(In reply to JF Bastien from comment #3)
> (In reply to JF Bastien from comment #2)
> > That's not good! Would you want to prepare a patch to fix it?
> 
> Ha, we raced!
> 
> 
> I think it would be better to get `VM& vm` from plan, and then use it,
> instead of doing p->vm().

Why? p and plan are actually the same, plan is just a Ref created from p (which is a reference, we are not doing p->vm() but p.vm()) to pass it to the lambda.
Comment 5 Carlos Garcia Campos 2017-04-02 23:03:36 PDT
Created attachment 306063 [details]
Updated patch

Make it simpler by using makeRefPtr in the lambda capture
Comment 6 Saam Barati 2017-04-02 23:17:42 PDT
Comment on attachment 306063 [details]
Updated patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94
> +        p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable {

Why not plan=WTFMove(plan)?
Comment 7 Carlos Garcia Campos 2017-04-03 03:26:34 PDT
(In reply to Saam Barati from comment #6)
> Comment on attachment 306063 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> 
> > 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).
Comment 8 Saam Barati 2017-04-03 09:11:31 PDT
Comment on attachment 306063 [details]
Updated patch

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

>>> 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.
Comment 9 Saam Barati 2017-04-03 09:14:36 PDT
Comment on attachment 306063 [details]
Updated patch

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

>>>> 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().
Also, I'm not sure why we make a RefPtr instead of a Ref here.
Comment 10 JF Bastien 2017-04-03 09:30:17 PDT
(In reply to Saam Barati from comment #9)
> Comment on attachment 306063 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> 
> >>>> 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().
> Also, I'm not sure why we make a RefPtr instead of a Ref here.

This weirdness is why I suggest getting VM, and then using it on this line. The entire problem is using plan two ways on one line of code: once to get to VM, once to lambda capture. Getting rid of one is an easy fix, lambda capture can't go, so VM on another line seems like the right fix.
Comment 11 Carlos Garcia Campos 2017-04-03 09:40:38 PDT
(In reply to Saam Barati from comment #8)
> Comment on attachment 306063 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> 
> >>> 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.

I think it's the same, we are taking a reference of p and transferring it to the lambda.

> Even if that assignment doesn't cause this to happen, I think it's harder to
> reason about than just having a local variable.

I don't mind leaving the local variable, I just thought it was simpler this way, in one step.
Comment 12 Carlos Garcia Campos 2017-04-03 09:43:21 PDT
(In reply to Saam Barati from comment #9)
> Comment on attachment 306063 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> 
> >>>> 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 comment #1

> 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.
Comment 13 Carlos Garcia Campos 2017-04-03 09:45:01 PDT
(In reply to JF Bastien from comment #10)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 306063 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> > 
> > >>>> 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().
> > Also, I'm not sure why we make a RefPtr instead of a Ref here.
> 
> This weirdness is why I suggest getting VM, and then using it on this line.
> The entire problem is using plan two ways on one line of code: once to get
> to VM, once to lambda capture. Getting rid of one is an easy fix, lambda
> capture can't go, so VM on another line seems like the right fix.

I don't see anything weird in the current patch, TBH. But I'm fine with any other solution, either my first patch or adding another local variable for the VM or a combination of both using a local variable for VM, but using makeRefPtr directly in the lambda capture. All of them should work.
Comment 14 Saam Barati 2017-04-03 11:42:36 PDT
(In reply to Carlos Garcia Campos from comment #12)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 306063 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> > 
> > >>>> 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 comment #1
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.
> 
> > 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);
?
Comment 15 Carlos Garcia Campos 2017-04-04 01:09:22 PDT
(In reply to Saam Barati from comment #14)
> (In reply to Carlos Garcia Campos from comment #12)
> > (In reply to Saam Barati from comment #9)
> > > Comment on attachment 306063 [details]
> > > Updated patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=306063&action=review
> > > 
> > > >>>> 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 comment #1
> 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.

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.

> > 
> > > 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);
> ?

JSWebAssemblyModule::createStub() expects a RefPtr<>&& anyway, so it's simpler this way.
Comment 16 Carlos Garcia Campos 2017-04-05 00:32:11 PDT
Saam, anything else you want me to change in the patch?
Comment 17 Saam Barati 2017-04-05 00:45:49 PDT
Comment on attachment 306054 [details]
Patch

r=me
Comment 18 Carlos Garcia Campos 2017-04-05 00:53:53 PDT
Committed r214935: <http://trac.webkit.org/changeset/214935>