167088 2017-01-16 02:14:31 -0800 [Win] Clipboard tests are flaky. 2017-01-18 05:11:10 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pvollan pvollan achristensen bfulgham lforschler oldest_to_newest 1267116 0 pvollan 2017-01-16 02:14:31 -0800 Tests involving the clipboard are flaky when running with multiple DRTs, since the clipboard is global. We can fix this by assigning each DRT a separate window station (each window station has its own clipboard). 1267118 1 298949 pvollan 2017-01-16 02:22:08 -0800 Created attachment 298949 Patch 1267370 2 298949 bfulgham 2017-01-17 09:26:03 -0800 Comment on attachment 298949 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298949&action=review What a great idea! I didn't know this was a problem, or that Window Stations even existed. Fantastic! I just had a question about how error state is handled in this patch, and whether we should be calling "CreateDesktop" with bad inputs. If it is safe, I approve the patch. But if you are not sure, I'd prefer you bypass these steps. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1485 > + fprintf(stderr, "DumpRenderTree should be run as Administrator!\n"); This is a shame! Is there no permission that can be granted a test user account to allow creation of unique Window Stations? > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1488 > + if (!::SetProcessWindowStation(windowsStation)) Is it okay to pass a nullptr windowStation here (assuming line 1481 is false)? We probably shouldn't terminate if this stuff fails, since we've been living without Window Stations for years. But I wonder if we should bypass the steps in lines 1481-1493 when stuff is failing, rather than passing bad values to these API calls. 1267717 3 pvollan 2017-01-18 05:03:24 -0800 (In reply to comment #2) > Comment on attachment 298949 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298949&action=review > > What a great idea! I didn't know this was a problem, or that Window Stations > even existed. Fantastic! I just had a question about how error state is > handled in this patch, and whether we should be calling "CreateDesktop" with > bad inputs. If it is safe, I approve the patch. But if you are not sure, I'd > prefer you bypass these steps. > > > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1485 > > + fprintf(stderr, "DumpRenderTree should be run as Administrator!\n"); > > This is a shame! Is there no permission that can be granted a test user > account to allow creation of unique Window Stations? > > > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1488 > > + if (!::SetProcessWindowStation(windowsStation)) > > Is it okay to pass a nullptr windowStation here (assuming line 1481 is > false)? We probably shouldn't terminate if this stuff fails, since we've > been living without Window Stations for years. But I wonder if we should > bypass the steps in lines 1481-1493 when stuff is failing, rather than > passing bad values to these API calls. Thanks for reviewing! I will update the patch before landing :) 1267719 4 pvollan 2017-01-18 05:11:10 -0800 Committed r210850: <https://trac.webkit.org/changeset/210850> 298949 2017-01-16 02:22:08 -0800 2017-01-17 09:26:03 -0800 Patch bug-167088-20170116022023.patch text/plain 2707 pvollan SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIxMDc4NCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDE3LTAxLTE2ICBQZXIgQXJuZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNv bT4KKworICAgICAgICBbV2luXSBDbGlwYm9hcmQgdGVzdHMgYXJlIGZsYWt5LgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY3MDg4CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGVzdHMgaW52b2x2aW5nIHRo ZSBjbGlwYm9hcmQgYXJlIGZsYWt5IHdoZW4gcnVubmluZyB3aXRoIG11bHRpcGxlIERSVHMsIHNp bmNlIHRoZSBjbGlwYm9hcmQgaXMgZ2xvYmFsLiAKKyAgICAgICAgV2UgY2FuIGZpeCB0aGlzIGJ5 IGFzc2lnbmluZyBlYWNoIERSVCBhIHNlcGFyYXRlIHdpbmRvdyBzdGF0aW9uIChlYWNoIHdpbmRv dyBzdGF0aW9uIGhhcyBpdHMgb3duIGNsaXBib2FyZCkuCisKKyAgICAgICAgKiBEdW1wUmVuZGVy VHJlZS93aW4vRHVtcFJlbmRlclRyZWUuY3BwOgorICAgICAgICAobWFpbik6CisKIDIwMTctMDEt MTUgIE1pY2hhZWwgQ2F0YW56YXJvICA8bWNhdGFuemFyb0BpZ2FsaWEuY29tPgogCiAgICAgICAg IFVucmV2aWV3ZWQsIHJlbW92ZSBzdGFsZSBjb21tZW50LgpJbmRleDogVG9vbHMvRHVtcFJlbmRl clRyZWUvd2luL0R1bXBSZW5kZXJUcmVlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9EdW1wUmVu ZGVyVHJlZS93aW4vRHVtcFJlbmRlclRyZWUuY3BwCShyZXZpc2lvbiAyMTA2MzEpCisrKyBUb29s cy9EdW1wUmVuZGVyVHJlZS93aW4vRHVtcFJlbmRlclRyZWUuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0xNDcxLDYgKzE0NzEsMjcgQEAgaW50IG1haW4oaW50IGFyZ2MsIGNvbnN0IGNoYXIqIGFyZ3Zb XSkKICAgICAvLyBSZWRpcmVjdCBzdGRvdXQgdG8gc3RkZXJyLgogICAgIGludCByZXN1bHQgPSBf ZHVwMihfZmlsZW5vKHN0ZGVyciksIDEpOwogCisgICAgLy8gVGVzdHMgaW52b2x2aW5nIHRoZSBj bGlwYm9hcmQgYXJlIGZsYWt5IHdoZW4gcnVubmluZyB3aXRoIG11bHRpcGxlIERSVHMsIHNpbmNl IHRoZSBjbGlwYm9hcmQgaXMgZ2xvYmFsLgorICAgIC8vIFdlIGNhbiBmaXggdGhpcyBieSBhc3Np Z25pbmcgZWFjaCBEUlQgYSBzZXBhcmF0ZSB3aW5kb3cgc3RhdGlvbiAoZWFjaCB3aW5kb3cgc3Rh dGlvbiBoYXMgaXRzIG93biBjbGlwYm9hcmQpLgorICAgIERXT1JEIHByb2Nlc3NJZCA9IDo6R2V0 Q3VycmVudFByb2Nlc3NJZCgpOworICAgIFN0cmluZyB3aW5kb3dTdGF0aW9uTmFtZSA9IFN0cmlu Zzo6Zm9ybWF0KCJ3aW5kb3dTdGF0aW9uJWQiLCBwcm9jZXNzSWQpOworICAgIFN0cmluZyBkZXNr dG9wTmFtZSA9IFN0cmluZzo6Zm9ybWF0KCJkZXNrdG9wJWQiLCBwcm9jZXNzSWQpOworCisgICAg YXV0byB3aW5kb3dzU3RhdGlvbiA9IDo6Q3JlYXRlV2luZG93U3RhdGlvbih3aW5kb3dTdGF0aW9u TmFtZS5jaGFyYWN0ZXJzV2l0aE51bGxUZXJtaW5hdGlvbigpLmRhdGEoKSwgQ1dGX0NSRUFURV9P TkxZLCBXSU5TVEFfQUxMX0FDQ0VTUywgbnVsbHB0cik7CisgICAgaWYgKCF3aW5kb3dzU3RhdGlv bikgeworICAgICAgICBEV09SRCBlcnJvciA9IDo6R2V0TGFzdEVycm9yKCk7CisgICAgICAgIGZw cmludGYoc3RkZXJyLCAiRmFpbGVkIHRvIGNyZWF0ZSB3aW5kb3cgc3RhdGlvbiB3aXRoIGVycm9y ICVkXG4iLCBlcnJvcik7CisgICAgICAgIGlmIChlcnJvciA9PSBFUlJPUl9BQ0NFU1NfREVOSUVE KQorICAgICAgICAgICAgZnByaW50ZihzdGRlcnIsICJEdW1wUmVuZGVyVHJlZSBzaG91bGQgYmUg cnVuIGFzIEFkbWluaXN0cmF0b3IhXG4iKTsKKyAgICB9CisKKyAgICBpZiAoITo6U2V0UHJvY2Vz c1dpbmRvd1N0YXRpb24od2luZG93c1N0YXRpb24pKQorICAgICAgICBmcHJpbnRmKHN0ZGVyciwg IlNldFByb2Nlc3NXaW5kb3dTdGF0aW9uIGZhaWxlZCB3aXRoIGVycm9yICVkXG4iLCA6OkdldExh c3RFcnJvcigpKTsKKworICAgIGF1dG8gZGVza3RvcCA9IDo6Q3JlYXRlRGVza3RvcChkZXNrdG9w TmFtZS5jaGFyYWN0ZXJzV2l0aE51bGxUZXJtaW5hdGlvbigpLmRhdGEoKSwgbnVsbHB0ciwgbnVs bHB0ciwgMCwgR0VORVJJQ19BTEwsIG51bGxwdHIpOworICAgIGlmICghZGVza3RvcCkKKyAgICAg ICAgZnByaW50ZihzdGRlcnIsICJGYWlsZWQgdG8gY3JlYXRlIGRlc2t0b3Agd2l0aCBlcnJvciAl ZFxuIiwgOjpHZXRMYXN0RXJyb3IoKSk7CisKICAgICBpbml0aWFsaXplKCk7CiAKICAgICBzZXRE ZWZhdWx0c1RvQ29uc2lzdGVudFZhbHVlc0ZvclRlc3RpbmcoKTsKQEAgLTE1NzMsNiArMTU5NCw5 IEBAIGludCBtYWluKGludCBhcmdjLCBjb25zdCBjaGFyKiBhcmd2W10pCiAgICAgX0NydER1bXBN ZW1vcnlMZWFrcygpOwogI2VuZGlmCiAKKyAgICA6OkNsb3NlRGVza3RvcChkZXNrdG9wKTsKKyAg ICA6OkNsb3NlV2luZG93U3RhdGlvbih3aW5kb3dzU3RhdGlvbik7CisKICAgICA6Ok9sZVVuaW5p dGlhbGl6ZSgpOwogCiAgICAgcmV0dXJuIDA7Cg==