1422661 - [U2F] Fix test failures in new microtask scheduling

Status: RESOLVED FIXED

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

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

This is a follow-up of bug 1193394 comment48 and they are new failures found on try on Last Friday in bug 1193394 comment 62:

TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_appid_facet.html | Test timed out.
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_appid_facet_subdomain.html | Test timed out.
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_multiple_keys.html | Test timed out.
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_register_sign.html | /tests/dom/u2f/tests/frame_register_sign.html: The signing did not error unexpectedly: 4 !== 0
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_register_sign.html | /tests/dom/u2f/tests/frame_register_sign.html: The signing did provide client data unexpectedly: undefined === undefined
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_register_sign.html | Test timed out.
TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 2 remaining tests.
TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | /tests/dom/u2f/tests/test_register_sign.html - finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
TEST-UNEXPECTED-ERROR | (SimpleTest/TestRunner.js) | Finished in 324988ms
TEST-UNEXPECTED-FAIL | leakcheck | tab process: 388725 bytes leaked (APZEventState, ActiveElementManager, AsyncLatencyLogger, BackstagePass, CSPService, ...)

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Take this bug to follow up.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Make sure U2F is reentry-capable after U2F.cpp and U2FManager.cpp are merged.

This is to migrate the fix of bug 1413125 after U2F.cpp and U2FManager.cpp are merged in bug 1410346.
(We didn't capture this because bug 1193394 is still blocked by other test failures.)

Hi J.C.,

May I have your review again for this migration?

Thanks!

Comment 3

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8935299 [details] [diff] [review]
(v1) Patch: Make sure U2F is reentry-capable after U2F.cpp and U2FManager.cpp are merged.

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

Passing this to Tim as he most recently touched this part of the U2F code.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8935299 [details] [diff] [review]
(v1) Patch: Make sure U2F is reentry-capable after U2F.cpp and U2FManager.cpp are merged.

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

I noticed that ClearTransaction() should actually reset the callbacks too, currently that's in the destructor only. It also bothers me a little that ExecuteCallback() calls ClearTransaction() now. I think we can make this a little clearer and do some more cleanup. But I wouldn't want to have you do it :) I hope you don't mind me stealing that...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8935771 [details]
Bug 1422661 - Fix U2F test failures in new microtask scheduling r?jcj

Bevis, can you please check that this fixes the failures you see with your new microtask scheduling patch?

Comment 6

[Phabricator Automation]

Comment on attachment 8935771 [details]
Bug 1422661 - Fix U2F test failures in new microtask scheduling r?jcj

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D329#8147

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4539d6cff1
Fix U2F test failures in new microtask scheduling r=jcj

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1e4539d6cff1

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> Bevis, can you please check that this fixes the failures you see with your
> new microtask scheduling patch?

It's green now with the new scheduling:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8338f5cd7bd3a24b01db8c7f28f661b8f878ea40&selectedJob=151144096

Thanks for your support! \o/

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #9)
> (In reply to Tim Taubert [:ttaubert] from comment #5)
> > Bevis, can you please check that this fixes the failures you see with your
> > new microtask scheduling patch?
> 
> It's green now with the new scheduling:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8338f5cd7bd3a24b01db8c7f28f661b8f878ea40&selectedJob=1
> 51144096
> 
> Thanks for your support! \o/

Glad to hear \o/