Full Text Bug Listing

Carlos Garcia Campos

Reported 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

Carlos Garcia Campos

Comment 1 2017-04-02 09:56:54 PDT

Created attachment 306054 [details] Patch

JF Bastien

Comment 2 2017-04-02 09:57:22 PDT

That's not good! Would you want to prepare a patch to fix it?

JF Bastien

Comment 3 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().

Carlos Garcia Campos

Comment 4 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.

Carlos Garcia Campos

Comment 5 2017-04-02 23:03:36 PDT

Created attachment 306063 [details] Updated patch Make it simpler by using makeRefPtr in the lambda capture

Saam Barati

Comment 6 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)?

Carlos Garcia Campos

Comment 7 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).

Saam Barati

Comment 8 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.

Saam Barati

Comment 9 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.

JF Bastien

Comment 10 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.

Carlos Garcia Campos

Comment 11 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.

Carlos Garcia Campos

Comment 12 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.

Carlos Garcia Campos

Comment 13 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.

Saam Barati

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

Carlos Garcia Campos

Comment 15 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.

Carlos Garcia Campos

Comment 16 2017-04-05 00:32:11 PDT

Saam, anything else you want me to change in the patch?

Saam Barati

Comment 17 2017-04-05 00:45:49 PDT

Comment on attachment 306054 [details] Patch r=me

Carlos Garcia Campos

Comment 18 2017-04-05 00:53:53 PDT

Committed r214935: <http://trac.webkit.org/changeset/214935>

Attachments
Patch (3.34 KB, patch) 2017-04-02 09:56 PDT, Carlos Garcia Campos	saam: 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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bug 170390