WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153919
tests fail if display sleeps while run-webkit-tests is running
https://bugs.webkit.org/show_bug.cgi?id=153919
Summary
tests fail if display sleeps while run-webkit-tests is running
Darin Adler
Reported
2016-02-05 09:25:57 PST
For a while I’ve been suffering a problem where I see tests fail if the display sleeps while run-webkit-tests runs. While I don’t understand the underlying cause, I see a really simple way to work around this by invoking the OS X command line tool named caffeinate.
Attachments
Proposed patch
(2.85 KB, patch)
2016-02-05 19:02 PST
,
Aakash Jain
no flags
Details
Formatted Diff
Diff
Proposed patch
(1.40 KB, patch)
2016-05-28 00:38 PDT
,
Aakash Jain
no flags
Details
Formatted Diff
Diff
Updated patch
(3.28 KB, patch)
2016-06-06 19:50 PDT
,
Aakash Jain
ap
: review+
ap
: commit-queue-
Details
Formatted Diff
Diff
Updated patch
(3.19 KB, patch)
2016-06-08 11:37 PDT
,
Aakash Jain
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2016-02-05 09:26:56 PST
This is a practical problem for me because my home iMac is in a public place where I don’t want the distracting light when I’m not using it, so display sleep is set super-fast, 1 minute I think.
Darin Adler
Comment 2
2016-02-05 09:27:51 PST
Ideally we’d fix the tests so they don’t fail when the display is asleep but I don’t have the slightest idea why they fail so it’s much more practical to do the workaround. Testing it now to be sure it actually works.
Alexey Proskuryakov
Comment 3
2016-02-05 09:29:15 PST
We can easily prevent display sleep from LayoutTestHelper as well. Is preventing display sleep during layout tests an acceptable behavior for you?
Darin Adler
Comment 4
2016-02-05 09:37:16 PST
Yes, it would be better then the failing tests. Most of the time my computer is not running tests. And then perhaps some day we could figure out how to run tests while the display is asleep.
Alexey Proskuryakov
Comment 5
2016-02-05 09:40:59 PST
Yes, agreed. I think that using IOPMAssertionCreateWithDescription would probably be more robust that using caffeinate.
Darin Adler
Comment 6
2016-02-05 09:44:58 PST
I’m not sure what you mean by "robust" but I really don’t prefer one over the other. Just want to make sure that wherever we put the code we explain why we are doing it. Despite the fact that this is assigned to me, if someone else wants to tackle it, please feel free to grab the bug. I have a patch to address this, but currently it’s wrong and I don’t know when I will have time to make it right.
Aakash Jain
Comment 7
2016-02-05 10:31:59 PST
I'll work on it.
Alexey Proskuryakov
Comment 8
2016-02-05 17:00:09 PST
> I’m not sure what you mean by "robust"
If we were to spawn the caffeinate process, we would at least need to add it to Tools/BuildSlaveSupport/kill-old-processes, and might need special handling elsewhere. The reason is that when buildbot kills run-webkit-tests, that doesn't happen very cleanly. I'm not sure if that's an issue with buildbot, with our scripts, or even with process group leader support in Darwin. By adding this functionality to LayoutTestHelper, we avoid creating new special cases.
Aakash Jain
Comment 9
2016-02-05 19:02:47 PST
Created
attachment 270785
[details]
Proposed patch
WebKit Commit Bot
Comment 10
2016-02-05 23:01:00 PST
Comment on
attachment 270785
[details]
Proposed patch Clearing flags on attachment: 270785 Committed
r196212
: <
http://trac.webkit.org/changeset/196212
>
WebKit Commit Bot
Comment 11
2016-02-05 23:01:02 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12
2016-02-06 08:23:32 PST
Thanks. I’ll give this a try and see if it solves my problem at home.
Darin Adler
Comment 13
2016-05-27 08:30:00 PDT
I have been having the same old problem even with this code checked in. But when I instead do "caffeinate run-webkit-tests" from the command line, it solves the problem just as it always did. Maybe LayoutTestHelper is used only for WebKit 1 testing?
Darin Adler
Comment 14
2016-05-27 08:30:35 PDT
I’m going to reopen this bug because the problem is not fixed.
Aakash Jain
Comment 15
2016-05-27 15:02:52 PDT
I am not able to reproduce it. I tried running run-webkit-tests both for wk1 and wk2 (on mac). In another terminal window I did "pmset -g assertions .” to check the flags and in both cases I found that LayoutTestHelper is correctly setting the flags. [buildbot]$ pmset -g assertions . | grep -i webkit pid 90229(LayoutTestHelper): [0x000a751700058f5b] 00:00:01 PreventUserIdleDisplaySleep named: "WebKit LayoutTestHelper" Details: WebKit layout-test helper tool is preventing sleep.
Darin Adler
Comment 16
2016-05-27 15:33:35 PDT
To test this, did you set a Mac’s Energy Saver setting to turn the display off after 1 minute? We can see from our first unsuccessful attempt that it’s hard to know if we fixed this by just looking at the flag; we’ll need to test our solution by actually reproducing the problem I was seeing. In my testing run-webkit-tests with caffeinate works, and without caffeinate hundreds of tests fail. I am not disputing that the tool sets that flag on your computer. Maybe the flag isn’t set successfully on my computer (I will check tonight)? Maybe we need one of these other flags too? PreventSystemSleep PreventUserIdleSystemSleep
Aakash Jain
Comment 17
2016-05-27 15:48:27 PDT
ok. I was trying on a lab's bot machine. I will try it on my local machine by configuring the Mac’s Energy Saver setting and checking tests results.
Aakash Jain
Comment 18
2016-05-28 00:04:51 PDT
I am trying to reproduce this on my laptop, but not able to reproduce it yet. In both wk1 and wk2, I notice that "PreventUserIdleDisplaySleep" flag is being set properly by "WebKit LayoutTestHelper". Also my laptop's display doesn't turn off while tests are running. If I stop the tests, my laptop display turns off after a minute or two. I will debug it further. Meanwhile it would be great if you can check if the flag is being set on your computer or not.
Aakash Jain
Comment 19
2016-05-28 00:28:26 PDT
I checked caffeinate, it sets another flag "PreventUserIdleSystemSleep" (same as the one you mentioned above), while we were setting "PreventUserIdleDisplaySleep" flag. Since caffeinate works for you, setting this flag should also work. I'll upload a patch soon for you to try.
Aakash Jain
Comment 20
2016-05-28 00:38:01 PDT
Created
attachment 280028
[details]
Proposed patch Please give it a try.
Darin Adler
Comment 21
2016-05-28 09:56:30 PDT
Comment on
attachment 280028
[details]
Proposed patch I think we want to prevent both, right? Well I will try this as you posted it.
Darin Adler
Comment 22
2016-05-28 10:52:49 PDT
Comment on
attachment 280028
[details]
Proposed patch This patch didn’t work. The tests all still timed out.
Darin Adler
Comment 23
2016-05-28 10:54:54 PDT
Sorry, I meant to say "many tests timed out". Not all. New info coming soon.
Darin Adler
Comment 24
2016-05-28 10:55:44 PDT
There may have been something wrong with my experiment. I will try again.
Darin Adler
Comment 25
2016-05-28 11:05:51 PDT
Comment on
attachment 280028
[details]
Proposed patch My mistake. This seems to be working fine.
WebKit Commit Bot
Comment 26
2016-05-28 11:26:40 PDT
Comment on
attachment 280028
[details]
Proposed patch Clearing flags on attachment: 280028 Committed
r201489
: <
http://trac.webkit.org/changeset/201489
>
WebKit Commit Bot
Comment 27
2016-05-28 11:26:44 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 28
2016-05-29 10:54:23 PDT
Sorry for the mixed message. Many tests are still timing out. This is still not fixed.
Aakash Jain
Comment 29
2016-05-29 12:32:02 PDT
Is the display getting turned off or not? Is the behavior different than when executed with caffeinate?
Darin Adler
Comment 30
2016-05-31 09:46:13 PDT
I think the display does get turned off. And I don’t think caffeinate solves the problem either. I don’t think we are going to get this problem solved indirectly with me repeatedly testing different changes. I think the person who wants to this needs to reproduce the problem. I’ll be happy to test once we have a fix that works for someone else who has reproduced it and sees it fixed.
Aakash Jain
Comment 31
2016-05-31 10:55:38 PDT
ok. I was assuming that caffeinate solves the problem properly, so I was just trying to replicate the behavior. I will reproduce it properly and fix the underlying cause. I am at a conference currently, but should be able to work on it soon.
Darin Adler
Comment 32
2016-06-05 21:06:28 PDT
OK, I just tried this experiment: % caffeinate -dis run-webkit-tests When I did that, all the tests passed.
Darin Adler
Comment 33
2016-06-06 08:40:59 PDT
This also worked: % caffeinate -di run-webkit-tests Next time I will try just "caffeinate -d".
Aakash Jain
Comment 34
2016-06-06 11:00:21 PDT
That indicates that we need both the flags "PreventUserIdleSystemSleep" and "PreventUserIdleDisplaySleep". "-d" flag sets PreventUserIdleDisplaySleep, "-i" flag sets PreventUserIdleSystemSleep. I think running "caffeinate -d", should be equivalent to running just "caffeinate", which you have already tried. I am trying to reproduce the issue today to confirm it. Sorry for the delay, I was out last week.
Darin Adler
Comment 35
2016-06-06 11:25:30 PDT
I’ll keep trying different combinations.
Aakash Jain
Comment 36
2016-06-06 19:50:48 PDT
Created
attachment 280663
[details]
Updated patch We need both the flags (to make sure that neither display nor system goes to sleep). I was able to reproduce the issue. Settings these flags fixed the issue for me. This is equivalent of "caffeinate -di". Can you please try the patch?
Alexey Proskuryakov
Comment 37
2016-06-08 10:38:31 PDT
Comment on
attachment 280663
[details]
Updated patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280663&action=review
r=me, definitely worth landing since you confirmed that this fixes the problem for you.
> Tools/DumpRenderTree/mac/LayoutTestHelper.m:199 > -static void releaseDisplaySleepAssertion() > +static void releaseSleepAssertion()
Since there are two assertions here, the function name should be releaseSleepAssertions.
> Tools/DumpRenderTree/mac/LayoutTestHelper.m:245 > -void addDisplaySleepAssertion() > +void addSleepAssertion()
Ditto.
Aakash Jain
Comment 38
2016-06-08 11:37:27 PDT
Created
attachment 280817
[details]
Updated patch Incorporated above review comments.
WebKit Commit Bot
Comment 39
2016-06-08 12:16:24 PDT
Comment on
attachment 280817
[details]
Updated patch Clearing flags on attachment: 280817 Committed
r201821
: <
http://trac.webkit.org/changeset/201821
>
WebKit Commit Bot
Comment 40
2016-06-08 12:16:29 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug