Bug 153919

Summary: tests fail if display sleeps while run-webkit-tests is running
Product: WebKit Reporter: Darin Adler <darin>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, jbedard, lforschler, simon.fraser
Priority: P2    
Version: WebKit Local Build   
Hardware: Macintosh   
OS: All   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Updated patch
ap: review+, ap: commit-queue-
Updated patch none

Description Darin Adler 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.
Comment 1 Darin Adler 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.
Comment 2 Darin Adler 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.
Comment 3 Alexey Proskuryakov 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?
Comment 4 Darin Adler 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.
Comment 5 Alexey Proskuryakov 2016-02-05 09:40:59 PST
Yes, agreed.

I think that using IOPMAssertionCreateWithDescription would probably be more robust that using caffeinate.
Comment 6 Darin Adler 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.
Comment 7 Aakash Jain 2016-02-05 10:31:59 PST
I'll work on it.
Comment 8 Alexey Proskuryakov 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.
Comment 9 Aakash Jain 2016-02-05 19:02:47 PST
Created attachment 270785 [details]
Proposed patch
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2016-02-05 23:01:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2016-02-06 08:23:32 PST
Thanks. I’ll give this a try and see if it solves my problem at home.
Comment 13 Darin Adler 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?
Comment 14 Darin Adler 2016-05-27 08:30:35 PDT
I’m going to reopen this bug because the problem is not fixed.
Comment 15 Aakash Jain 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.
Comment 16 Darin Adler 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
Comment 17 Aakash Jain 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.
Comment 18 Aakash Jain 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.
Comment 19 Aakash Jain 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.
Comment 20 Aakash Jain 2016-05-28 00:38:01 PDT
Created attachment 280028 [details]
Proposed patch

Please give it a try.
Comment 21 Darin Adler 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.
Comment 22 Darin Adler 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.
Comment 23 Darin Adler 2016-05-28 10:54:54 PDT
Sorry, I meant to say "many tests timed out". Not all. New info coming soon.
Comment 24 Darin Adler 2016-05-28 10:55:44 PDT
There may have been something wrong with my experiment. I will try again.
Comment 25 Darin Adler 2016-05-28 11:05:51 PDT
Comment on attachment 280028 [details]
Proposed patch

My mistake. This seems to be working fine.
Comment 26 WebKit Commit Bot 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>
Comment 27 WebKit Commit Bot 2016-05-28 11:26:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Darin Adler 2016-05-29 10:54:23 PDT
Sorry for the mixed message. Many tests are still timing out. This is still not fixed.
Comment 29 Aakash Jain 2016-05-29 12:32:02 PDT
Is the display getting turned off or not? Is the behavior different than when executed with caffeinate?
Comment 30 Darin Adler 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.
Comment 31 Aakash Jain 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.
Comment 32 Darin Adler 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.
Comment 33 Darin Adler 2016-06-06 08:40:59 PDT
This also worked:

    % caffeinate -di run-webkit-tests

Next time I will try just "caffeinate -d".
Comment 34 Aakash Jain 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.
Comment 35 Darin Adler 2016-06-06 11:25:30 PDT
I’ll keep trying different combinations.
Comment 36 Aakash Jain 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?
Comment 37 Alexey Proskuryakov 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.
Comment 38 Aakash Jain 2016-06-08 11:37:27 PDT
Created attachment 280817 [details]
Updated patch

Incorporated above review comments.
Comment 39 WebKit Commit Bot 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>
Comment 40 WebKit Commit Bot 2016-06-08 12:16:29 PDT
All reviewed patches have been landed.  Closing bug.