Closed Bug 1422661 Opened 2 years ago Closed 2 years ago

[U2F] Fix test failures in new microtask scheduling

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(2 files)

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, ...)
Blocks: 1193394
Take this bug to follow up.
Assignee: nobody → btseng
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!
Attachment #8935299 - Flags: review?(jjones)
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.
Attachment #8935299 - Flags: review?(jjones) → review?(ttaubert)
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...
Attachment #8935299 - Flags: review?(ttaubert)
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?
Attachment #8935771 - Flags: feedback?(btseng)
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
Attachment #8935771 - Flags: review+
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4539d6cff1
Fix U2F test failures in new microtask scheduling r=jcj
https://hg.mozilla.org/mozilla-central/rev/1e4539d6cff1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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/
Attachment #8935771 - Flags: feedback?(btseng) → feedback+
(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/
You need to log in before you can comment on or make changes to this bug.