Bug 167088 - [Win] Clipboard tests are flaky.
Summary: [Win] Clipboard tests are flaky.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-16 02:14 PST by Per Arne Vollan
Modified: 2017-01-18 05:11 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.64 KB, patch)
2017-01-16 02:22 PST, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-01-16 02:14:31 PST
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).
Comment 1 Per Arne Vollan 2017-01-16 02:22:08 PST
Created attachment 298949 [details]
Patch
Comment 2 Brent Fulgham 2017-01-17 09:26:03 PST
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.
Comment 3 Per Arne Vollan 2017-01-18 05:03:24 PST
(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 :)
Comment 4 Per Arne Vollan 2017-01-18 05:11:10 PST
Committed r210850: <https://trac.webkit.org/changeset/210850>