mac: Fix TestGTMSMJobSubmitRemove on Yosemite.

mac: Fix TestGTMSMJobSubmitRemove on Yosemite.

On OSX 10.10, using launch_msg() to remove a job does not guarantee that the
task is synchronously killed. The unit test needs to wait for the task to be
killed before proceeding.

BUG=390276
Committed: https://crrev.com/37bc1c78f8efac04c98c7f2635dd1cd737e44843
Cr-Commit-Position: refs/heads/master@{#314181}

[erikchen, 2014-10-20 20:07:31]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2014-10-20 20:07:32]

mark: Please review.

[Mark Mentovai, 2014-10-20 20:24:38]

Yes, I’ve definitely seen this behavior change in 10.10.

https://codereview.chromium.org/664863003/diff/1/chrome/browser/service_proce...
File chrome/browser/service_process/service_process_control_mac_unittest.mm
(right):

https://codereview.chromium.org/664863003/diff/1/chrome/browser/service_proce...
chrome/browser/service_process/service_process_control_mac_unittest.mm:73:
ASSERT_TRUE(GTMSMJobRemove(label_cf, &error));
Shouldn’t we fix GTMSMJobRemove() instead?

[erikchen, 2014-10-21 00:13:25]

https://codereview.chromium.org/664863003/diff/1/chrome/browser/service_proce...
File chrome/browser/service_process/service_process_control_mac_unittest.mm
(right):

https://codereview.chromium.org/664863003/diff/1/chrome/browser/service_proce...
chrome/browser/service_process/service_process_control_mac_unittest.mm:73:
ASSERT_TRUE(GTMSMJobRemove(label_cf, &error));
On 2014/10/20 20:24:38, Mark Mentovai wrote:
> Shouldn’t we fix GTMSMJobRemove() instead?

I looked more into this. GTMSMJobRemove() is a thin wrapper around launch_msg(),
which is deprecated in Yosemite. I imagine the correct solution is to move to
supported, modern OSX APIs such as XPC services and Launch Agents. 

Are you expecting to see a migration to the more modern APIs, or were you
thinking more along the line of hacks to GTMSMJobRemove() to get it to not
return an error message for this specific use case?

[Mark Mentovai, 2014-10-21 03:25:54]

There are some things that can’t become XPC services or have installed
LaunchDaemon/LaunchAgent plists. For those things, a ServiceManagement-like API
is still appropriate.

I was only expecting to fix GTMSMJobRemove() to behave on 10.10 as it does on
all previous releases. On previous releases, it would return success if it had
requested job removal, even if the job was not actually removed. The change on
10.10 with this API is that you will receive EINPROGRESS in this case. We can
just catch EINPROGRESS and treat it as a successful return.

GTMSMJobRemove() never implemented the block-while-waiting-for-actual-exit
behavior that you’d get with SMJobRemove() and |wait| set to TRUE.

[erikchen, 2015-01-28 04:14:42]

On 2014/10/21 03:25:54, Mark Mentovai wrote:
> There are some things that can’t become XPC services or have installed
> LaunchDaemon/LaunchAgent plists. For those things, a ServiceManagement-like
API
> is still appropriate.
> 
> I was only expecting to fix GTMSMJobRemove() to behave on 10.10 as it does on
> all previous releases. On previous releases, it would return success if it had
> requested job removal, even if the job was not actually removed. The change on
> 10.10 with this API is that you will receive EINPROGRESS in this case. We can
> just catch EINPROGRESS and treat it as a successful return.
> 
> GTMSMJobRemove() never implemented the block-while-waiting-for-actual-exit
> behavior that you’d get with SMJobRemove() and |wait| set to TRUE.

mark: PTAL

I've updated the CL and the description to fix the test on Yosemite, which
requires that the unit test wait for the task to exit.

[Mark Mentovai, 2015-01-28 05:22:44]

LGTM

[erikchen, 2015-01-28 18:09:53]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-01-28 18:10:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/664863003/20001

[commit-bot: I haz the power, 2015-01-28 18:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-28 18:17:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2015-01-29 22:03:38]

erikchen@chromium.org changed reviewers:
+ scottbyer@chromium.org

[erikchen, 2015-01-29 22:03:39]

scottbyer: Looking for an OWNER review.

[Scott Byer, 2015-01-31 00:18:15]

https://codereview.chromium.org/664863003/diff/20001/chrome/browser/service_p...
File chrome/browser/service_process/service_process_control_mac_unittest.mm
(right):

https://codereview.chromium.org/664863003/diff/20001/chrome/browser/service_p...
chrome/browser/service_process/service_process_control_mac_unittest.mm:64:
base::TimeDelta timeout_in_ms = base::TimeDelta::FromMilliseconds(1000);
IIRC, there are some testing time constants hanging around somewhere - see if
there's an appropriate one. They're there because they get bigger on the
(slow/overloaded) bots.

[erikchen, 2015-01-31 00:52:05]

scottbyer: PTAL

https://codereview.chromium.org/664863003/diff/20001/chrome/browser/service_p...
File chrome/browser/service_process/service_process_control_mac_unittest.mm
(right):

https://codereview.chromium.org/664863003/diff/20001/chrome/browser/service_p...
chrome/browser/service_process/service_process_control_mac_unittest.mm:64:
base::TimeDelta timeout_in_ms = base::TimeDelta::FromMilliseconds(1000);
On 2015/01/31 00:18:15, Scott Byer wrote:
> IIRC, there are some testing time constants hanging around somewhere - see if
> there's an appropriate one. They're there because they get bigger on the
> (slow/overloaded) bots.

Found the class "TestTimeouts" and updated the code.

[Scott Byer, 2015-02-01 18:40:14]

lgtm

[erikchen, 2015-02-02 17:53:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-02 17:53:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/664863003/40001

[commit-bot: I haz the power, 2015-02-02 20:30:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-02 20:31:17]

Patchset 3 (id:??) landed as
https://crrev.com/37bc1c78f8efac04c98c7f2635dd1cd737e44843
Cr-Commit-Position: refs/heads/master@{#314181}