Closed Bug 1413125 Opened 2 years ago Closed 2 years ago

[U2F] Mark MozPromiseRequestHolder::Complete() earlier before calling JS callback to prevent unexpected call to ThenValueBase::Disconnect()

Categories

(Core :: DOM: Device Interfaces, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file)

This change is needed to meet the fix of microtask checkpoints addressed in bug 1193394:
The scheduling of microtask in current implementation is incomplete and the only check point we have done is at the end of a task according to step 6 of 
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model
However, there is another checkpoint at the return of outermost JS callback specified in https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script, which is expected to be introduced in bug 1193394.

With this new check point added, an assertion failure is captured in 
http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/xpcom/threads/MozPromise.h#472, because the next U2F::Register() will be called immediately at this new checkpoint earlier than MozPromiseRequestHolder::Complete() which is unexpected, compared to the PASSED call flow:
http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/u2f/U2F.cpp#317,327,394,404

Hence, we should call MozPromiseRequestHolder::Complete() before ExecuteCallback() to fix this assertion failure.

--
Call stack of the assertion in test_appid_facet.html:
#01: mozilla::MozPromise<nsTString<char16_t>, mozilla::dom::ErrorCode, false>::ThenValueBase::Disconnect [xpcom/threads/MozPromise.h:472]
#02: mozilla::MozPromise<nsTString<char16_t>, mozilla::dom::ErrorCode, false>::ThenValue<mozilla::dom::U2F::Register(const nsAString&, const mozilla::dom::Sequence<mozilla::dom::RegisterRequest>&, const mozilla::dom::Sequence<mozilla::dom::RegisteredKey>&, mozilla::dom::U2FRegisterCallback&, const mozilla::dom::Optional<mozilla::dom::Nullable<int> >&, mozilla::ErrorResult&)::<lambda(nsString)>, mozilla::dom::U2F::Register(const nsAString&, const mozilla::dom::Sequence<mozilla::dom::RegisterRequest>&, const mozilla::dom::Sequence<mozilla::dom::RegisteredKey>&, mozilla::dom::U2FRegisterCallback&, const mozilla::dom::Optional<mozilla::dom::Nullable<int> >&, mozilla::ErrorResult&)::<lambda(mozilla::dom::ErrorCode)> >::Disconnect [mfbt/Maybe.h:445]
#03: mozilla::MozPromiseRequestHolder<mozilla::MozPromise<nsTString<char16_t>, mozilla::dom::ErrorCode, false> >::Disconnect [mfbt/RefPtr.h:63]
#04: mozilla::dom::U2F::Cancel [xpcom/threads/MozPromise.h:1323]
#05: mozilla::dom::U2F::Register [dom/u2f/U2F.cpp:252]
#06: mozilla::dom::U2FBinding::_register_ [s3:gecko-generated-sources-l1:dfa87a19c6abfefd951703ec1084cd8835a94b9790a9358a15a055f25c67c701c2e4245d35afc0a9d230231b0fbbd106fd70890e33a18e4a91bb99d3da9e07e1/dom/bindings/U2FBinding.cpp::1537]
#07: mozilla::dom::GenericBindingMethod [dom/bindings/BindingUtils.cpp:3042]
#08: js::CallJSNative [js/src/jscntxtinlines.h:292]
#09: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:458]
#10: InternalCall [js/src/vm/Interpreter.cpp:522]
#11: Interpret [js/src/vm/Interpreter.cpp:3066]
#12: js::RunScript [js/src/vm/Interpreter.cpp:422]
#13: js::InternalCallOrConstruct [js/src/vm/Interpreter.h:205]
#14: InternalCall [js/src/vm/Interpreter.cpp:522]
#15: js::Call [js/src/vm/Interpreter.cpp:540]
#16: js::PromiseObject::create [js/src/builtin/Promise.cpp:1666]
#17: PromiseConstructor [js/src/builtin/Promise.cpp:1595]
#18: js::CallJSNative [js/src/jscntxtinlines.h:292]
#19: js::CallJSNativeConstructor [js/src/jscntxtinlines.h:324]
#20: InternalConstruct [js/src/vm/Interpreter.cpp:567]
#21: Interpret [js/src/vm/Interpreter.cpp:3058]
#22: js::RunScript [js/src/vm/Interpreter.cpp:422]
#23: js::InternalCallOrConstruct [js/src/vm/Interpreter.h:205]
#24: InternalCall [js/src/vm/Interpreter.cpp:522]
#25: js::Call [js/src/vm/Interpreter.cpp:540]
#26: js::CallSelfHostedFunction [js/src/vm/SelfHosting.cpp:1780]
#27: AsyncFunctionResume [js/src/vm/AsyncFunction.cpp:192]
#28: PromiseReactionJob [js/src/builtin/Promise.cpp:1094]
#29: js::CallJSNative [js/src/jscntxtinlines.h:292]
#30: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:458]
#31: InternalCall [js/src/vm/Interpreter.cpp:522]
#32: js::Call [js/src/vm/Interpreter.cpp:540]
#33: JS::Call [js/src/jsapi.cpp:3019]
#34: mozilla::dom::PromiseJobCallback::Call [s3:gecko-generated-sources-l1:1ac589ef3f4415b1cad140c5945f932e3fda630e97fc7dc01bdf6d306292c6c729b28d3a34f0a84b44362e7d528cc9d9627990562f56625771b02d265e090411/dom/bindings/PromiseBinding.cpp::21]
#35: mozilla::dom::PromiseJobCallback::Call [s3:gecko-generated-sources-l1:b4f3b4deaa975bd9f9a7a45c24b53dd1d24f707951e2b563dea1a56e9905e156ca2d24d3a6123e7e3ccf3c663f65339cbe6ee3dc1f98154c1a3c47a4d8f1d0d0/dist/include/mozilla/dom/PromiseBinding.h::89]
#36: mozilla::PromiseJobRunnable::Run [xpcom/base/CycleCollectedJSContext.cpp:211]
#37: mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint [xpcom/base/CycleCollectedJSContext.cpp:523]
#38: mozilla::dom::CallbackObject::CallSetup::~CallSetup [xpcom/base/CycleCollectedJSContext.h:195]
#39: mozilla::dom::ExecuteCallback<mozilla::dom::RegisterResponse, mozilla::dom::U2FRegisterCallback> [dom/u2f/U2F.cpp:187]
#40: mozilla::dom::U2F::Register(nsTSubstring<char16_t> const&, mozilla::dom::Sequence<mozilla::dom::RegisterRequest> const&, mozilla::dom::Sequence<mozilla::dom::RegisteredKey> const&, mozilla::dom::U2FRegisterCallback&, mozilla::dom::Optional<mozilla::dom::Nullable<int> > const&, mozilla::ErrorResult&)::{lambda(nsTString<char16_t>)#1}::operator()(nsTString<char16_t>) const
#41: mozilla::MozPromise<nsTString<char16_t>, mozilla::dom::ErrorCode, false>::ThenValue<mozilla::dom::U2F::Register(const nsAString&, const mozilla::dom::Sequence<mozilla::dom::RegisterRequest>&, const mozilla::dom::Sequence<mozilla::dom::RegisteredKey>&, mozilla::dom::U2FRegisterCallback&, const mozilla::dom::Optional<mozilla::dom::Nullable<int> >&, mozilla::ErrorResult&)::<lambda(nsString)>, mozilla::dom::U2F::Register(const nsAString&, const mozilla::dom::Sequence<mozilla::dom::RegisterRequest>&, const mozilla::dom::Sequence<mozilla::dom::RegisteredKey>&, mozilla::dom::U2FRegisterCallback&, const mozilla::dom::Optional<mozilla::dom::Nullable<int> >&, mozilla::ErrorResult&)::<lambda(mozilla::dom::ErrorCode)> >::DoResolveOrRejectInternal [xpcom/string/nsTSubstring.h:77]
#42: mozilla::MozPromise<nsTString<char16_t>, mozilla::dom::ErrorCode, false>::ThenValueBase::ResolveOrRejectRunnable::Run [mfbt/RefPtr.h:63]
#43: mozilla::SchedulerGroup::Runnable::Run [xpcom/threads/SchedulerGroup.cpp:396]
#44: nsThread::ProcessNextEvent [mfbt/Maybe.h:100]
This is to fix the reentry problem in U2F (see comment 0) once performing microtask-checkpoint for promise callbacks at the end of outermost JS callback [1] is introduced by 1193394.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script
Attachment #8924046 - Flags: review?(bugs)
Aha, ok, so this bug has nothing to do with MozPromises, since those have nothing to do with Promises, but about calling some callback and the code not expecting that during the callback someone may call back to the code.
Comment on attachment 8924046 [details] [diff] [review]
(v1) Patch: Allow reentry properly from the microtask-checkpoint at the end of a JS callback.

Since I don't know the control flow in U2F at all, perhaps better if jcj reviews this.

If I understand this correctly, this isn't about microtasks as such, but the code just not expecting scripts to do something, so the commit message should say something about that, and not about microtasks.
Attachment #8924046 - Flags: review?(bugs) → review?(jjones)
Comment on attachment 8924046 [details] [diff] [review]
(v1) Patch: Allow reentry properly from the microtask-checkpoint at the end of a JS callback.

Review of attachment 8924046 [details] [diff] [review]:
-----------------------------------------------------------------

There shouldn't be any issue with the control-flow like this, looks good to me. Thanks for the fix!
Attachment #8924046 - Flags: review?(jjones) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b1756603b4
Support reentry synchronously from U2F callbacks. r=jcj
https://hg.mozilla.org/mozilla-central/rev/f2b1756603b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.