201249 2019-08-28 16:26:12 -0700 CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread` 2019-08-29 10:12:27 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 joepeck cdumez achristensen benjamin cdumez cmarcelo commit-queue dbates ews-watchlist joepeck webkit-bug-importer oldest_to_newest 1565943 0 joepeck 2019-08-28 16:26:12 -0700 CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread` 1565946 1 joepeck 2019-08-28 16:28:43 -0700 Debug only member for assertions. I'm seeing assertions! 1565947 2 cdumez 2019-08-28 16:31:06 -0700 (In reply to Joseph Pecoraro from comment #1) > Debug only member for assertions. I'm seeing assertions! Oh, I guess if somebody default-constructs a CompletionHandler and then later on assigns a lambda to it, and then calls the completion handler on the wrong thread. You found cases like this? 1565950 3 joepeck 2019-08-28 16:39:56 -0700 (In reply to Chris Dumez from comment #2) > (In reply to Joseph Pecoraro from comment #1) > > Debug only member for assertions. I'm seeing assertions! > > Oh, I guess if somebody default-constructs a CompletionHandler and then > later on assigns a lambda to it, and then calls the completion handler on > the wrong thread. You found cases like this? I'm adding code that is essentially like: CompletionHandler<void> handler; callOnMainThread([handler = WTFMove(handler)] mutable { handler(); }); And it is asserting. If I instead did: CompletionHandler<void> handler = []{}; callOnMainThread([handler = WTFMove(handler)] mutable { handler(); }); it is not asserting. 1565952 4 cdumez 2019-08-28 16:41:09 -0700 (In reply to Joseph Pecoraro from comment #3) > (In reply to Chris Dumez from comment #2) > > (In reply to Joseph Pecoraro from comment #1) > > > Debug only member for assertions. I'm seeing assertions! > > > > Oh, I guess if somebody default-constructs a CompletionHandler and then > > later on assigns a lambda to it, and then calls the completion handler on > > the wrong thread. You found cases like this? > > I'm adding code that is essentially like: > > CompletionHandler<void> handler; > callOnMainThread([handler = WTFMove(handler)] mutable { > handler(); > }); > > And it is asserting. > > If I instead did: > > CompletionHandler<void> handler = []{}; > callOnMainThread([handler = WTFMove(handler)] mutable { > handler(); > }); > > it is not asserting. Yes, you're not allowed to call a default constructed Function or CompletionHandler, it is by design and has nothing to do with threading. 1565953 5 joepeck 2019-08-28 16:46:26 -0700 > Yes, you're not allowed to call a default constructed Function or > CompletionHandler, it is by design and has nothing to do with threading. Okay, sounds good. I can check if the completion handler was initialized (operator bool) in these cases. Note that `CompletionHandlerCallingScope` does just that. Thanks! 1565954 6 cdumez 2019-08-28 16:47:39 -0700 Why are we closing this. We should probably still fix the bug where m_wasConstructedOnMainThread is not initialized. 1565955 7 cdumez 2019-08-28 16:47:57 -0700 Will fix. 1565956 8 joepeck 2019-08-28 16:49:11 -0700 Fair enough, but if we fix this won't it just assert next that the function is not callable next. So some assertion will happen. 1565957 9 cdumez 2019-08-28 16:50:30 -0700 (In reply to Joseph Pecoraro from comment #8) > Fair enough, but if we fix this won't it just assert next that the function > is not callable next. So some assertion will happen. Right, but still seems wrong to have an uninitialized data member. 1565958 10 377511 cdumez 2019-08-28 16:51:06 -0700 Created attachment 377511 Patch 1565960 11 377511 joepeck 2019-08-28 16:52:02 -0700 Comment on attachment 377511 Patch r=me 1565961 12 joepeck 2019-08-28 16:52:32 -0700 > Right, but still seems wrong to have an uninitialized data member. Sounds good! 1565963 13 377511 achristensen 2019-08-28 16:54:09 -0700 Comment on attachment 377511 Patch Why not just put this in an initializer list and keep the =default? 1565964 14 cdumez 2019-08-28 16:56:53 -0700 (In reply to Alex Christensen from comment #13) > Comment on attachment 377511 [details] > Patch > > Why not just put this in an initializer list and keep the =default? Oh, I can try this. 1566233 15 377587 cdumez 2019-08-29 08:24:06 -0700 Created attachment 377587 Patch 1566279 16 377587 commit-queue 2019-08-29 10:11:12 -0700 Comment on attachment 377587 Patch Clearing flags on attachment: 377587 Committed r249280: <https://trac.webkit.org/changeset/249280> 1566280 17 commit-queue 2019-08-29 10:11:13 -0700 All reviewed patches have been landed. Closing bug. 1566281 18 webkit-bug-importer 2019-08-29 10:12:27 -0700 <rdar://problem/54841787> 377511 2019-08-28 16:51:06 -0700 2019-08-29 08:24:05 -0700 Patch bug-201249-20190828165106.patch text/plain 1495 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ5MjI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGQ0YjhkZDEyZGM0NzZlODllY2MyYmJk Nzc3OWU1OTFmMWZmYzg2MzcuLjkxMzNhZjQ4NjZmMTZkYWNlMzY3NzgzZjAxZWI1ZmIwYTRkM2Rj NDkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTktMDgtMjggIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KKworICAgICAgICBDb21wbGV0aW9uSGFuZGxlciBkZWZhdWx0IGNvbnN0cnVj dG9yIGRvZXMgbm90IGluaXRpYWxpemUgYG1fd2FzQ29uc3RydWN0ZWRPbk1haW5UaHJlYWRgCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDEyNDkKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHd0Zi9Db21w bGV0aW9uSGFuZGxlci5oOgorICAgICAgICAoV1RGOjpDb21wbGV0aW9uSGFuZGxlcjxPdXQpOgor CiAyMDE5LTA4LTI1ICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgog CiAgICAgICAgIFJlZ3Jlc3Npb24ocjI0ODUzMykgQXNzZXJ0aW9uIGhpdCBpbiBpc01haW5UaHJl YWQoKSBmb3Igc29tZSBjbGllbnRzIHVzaW5nIFdURiBiZWNhdXNlIHRoZSBtYWluIHRocmVhZCBp cyBub3QgaW5pdGlhbGl6ZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL0NvbXBsZXRpb25I YW5kbGVyLmggYi9Tb3VyY2UvV1RGL3d0Zi9Db21wbGV0aW9uSGFuZGxlci5oCmluZGV4IDdmMTIz ZWNiZTMwMmVhN2FmMWIwYzY2YjVjZjQzMzkwNDM2NGFmODIuLjcxNGU0NmI1MGNkMWVlYjU2NDg3 NWQ3NTJhYWIyNWI1ZWQ2MTJhZDEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0NvbXBsZXRp b25IYW5kbGVyLmgKKysrIGIvU291cmNlL1dURi93dGYvQ29tcGxldGlvbkhhbmRsZXIuaApAQCAt MzcsNyArMzcsMTIgQEAgdGVtcGxhdGUgPHR5cGVuYW1lIE91dCwgdHlwZW5hbWUuLi4gSW4+CiBj bGFzcyBDb21wbGV0aW9uSGFuZGxlcjxPdXQoSW4uLi4pPiB7CiAgICAgV1RGX01BS0VfRkFTVF9B TExPQ0FURUQ7CiBwdWJsaWM6Ci0gICAgQ29tcGxldGlvbkhhbmRsZXIoKSA9IGRlZmF1bHQ7Cisg ICAgQ29tcGxldGlvbkhhbmRsZXIoKQorI2lmICFBU1NFUlRfRElTQUJMRUQKKyAgICAgICAgOiBt X3dhc0NvbnN0cnVjdGVkT25NYWluVGhyZWFkKGlzTWFpblRocmVhZCgpKQorI2VuZGlmCisgICAg eworICAgIH0KIAogICAgIHRlbXBsYXRlPHR5cGVuYW1lIENhbGxhYmxlVHlwZSwgY2xhc3MgPSB0 eXBlbmFtZSBzdGQ6OmVuYWJsZV9pZjxzdGQ6OmlzX3J2YWx1ZV9yZWZlcmVuY2U8Q2FsbGFibGVU eXBlJiY+Ojp2YWx1ZT46OnR5cGU+CiAgICAgQ29tcGxldGlvbkhhbmRsZXIoQ2FsbGFibGVUeXBl JiYgY2FsbGFibGUpCg== 377587 2019-08-29 08:24:06 -0700 2019-08-29 10:11:12 -0700 Patch bug-201249-20190829082405.patch text/plain 1595 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ5MjI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGQ0YjhkZDEyZGM0NzZlODllY2MyYmJk Nzc3OWU1OTFmMWZmYzg2MzcuLjMzYzYzODcyMTgyODE1YWUwYjYyZmY0YjMwZDA2ZDdkMDUzNDE2 YWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTktMDgtMjggIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KKworICAgICAgICBDb21wbGV0aW9uSGFuZGxlciBkZWZhdWx0IGNvbnN0cnVj dG9yIGRvZXMgbm90IGluaXRpYWxpemUgYG1fd2FzQ29uc3RydWN0ZWRPbk1haW5UaHJlYWRgCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDEyNDkKKwor ICAgICAgICBSZXZpZXdlZCBieSBKb3NlcGggUGVjb3Jhcm8gYW5kIEFsZXggQ2hyaXN0ZW5zZW4u CisKKyAgICAgICAgKiB3dGYvQ29tcGxldGlvbkhhbmRsZXIuaDoKKyAgICAgICAgKFdURjo6Q29t cGxldGlvbkhhbmRsZXI8T3V0KToKKwogMjAxOS0wOC0yNSAgRnVqaWkgSGlyb25vcmkgIDxIaXJv bm9yaS5GdWppaUBzb255LmNvbT4KIAogICAgICAgICBSZWdyZXNzaW9uKHIyNDg1MzMpIEFzc2Vy dGlvbiBoaXQgaW4gaXNNYWluVGhyZWFkKCkgZm9yIHNvbWUgY2xpZW50cyB1c2luZyBXVEYgYmVj YXVzZSB0aGUgbWFpbiB0aHJlYWQgaXMgbm90IGluaXRpYWxpemVkCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV1RGL3d0Zi9Db21wbGV0aW9uSGFuZGxlci5oIGIvU291cmNlL1dURi93dGYvQ29tcGxldGlv bkhhbmRsZXIuaAppbmRleCA3ZjEyM2VjYmUzMDJlYTdhZjFiMGM2NmI1Y2Y0MzM5MDQzNjRhZjgy Li4wMjc2YTI0YTViMmY5MWMzZjFjOTY2NTcyNzdiMjA5ZDRmNTZhNWQ2IDEwMDY0NAotLS0gYS9T b3VyY2UvV1RGL3d0Zi9Db21wbGV0aW9uSGFuZGxlci5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0Nv bXBsZXRpb25IYW5kbGVyLmgKQEAgLTQyLDkgKzQyLDYgQEAgcHVibGljOgogICAgIHRlbXBsYXRl PHR5cGVuYW1lIENhbGxhYmxlVHlwZSwgY2xhc3MgPSB0eXBlbmFtZSBzdGQ6OmVuYWJsZV9pZjxz dGQ6OmlzX3J2YWx1ZV9yZWZlcmVuY2U8Q2FsbGFibGVUeXBlJiY+Ojp2YWx1ZT46OnR5cGU+CiAg ICAgQ29tcGxldGlvbkhhbmRsZXIoQ2FsbGFibGVUeXBlJiYgY2FsbGFibGUpCiAgICAgICAgIDog bV9mdW5jdGlvbihXVEZNb3ZlKGNhbGxhYmxlKSkKLSNpZiAhQVNTRVJUX0RJU0FCTEVECi0gICAg ICAgICwgbV93YXNDb25zdHJ1Y3RlZE9uTWFpblRocmVhZChpc01haW5UaHJlYWQoKSkKLSNlbmRp ZgogICAgIHsKICAgICB9CiAKQEAgLTY4LDcgKzY1LDcgQEAgcHVibGljOgogcHJpdmF0ZToKICAg ICBGdW5jdGlvbjxPdXQoSW4uLi4pPiBtX2Z1bmN0aW9uOwogI2lmICFBU1NFUlRfRElTQUJMRUQK LSAgICBib29sIG1fd2FzQ29uc3RydWN0ZWRPbk1haW5UaHJlYWQ7CisgICAgYm9vbCBtX3dhc0Nv bnN0cnVjdGVkT25NYWluVGhyZWFkIHsgaXNNYWluVGhyZWFkKCkgfTsKICNlbmRpZgogfTsKIAo=