FirstRunDialogControllerTest.LayoutWithLongStrings is <https://chromium-review.googlesource.com/c/chromium/src/+/1316113>

Looking at ServiceProcessStateFileManipulationTest.DeleteFile, it appears that the executable_fsref_ in ExecFilePathWatcherCallback is at issue here: in DeleteBundle, we get file:///.file/id=x.y, which we try to delete trailing components from and get nil. In DeleteFile, we get file:///.file/id=x.y and try to delete components from it, and end up getting the .app bundle.

That means that good_bundle is 1 for DeleteFile and 0 for DeleteBundle, so for DeleteBundle, needs_shutdown is 1 and need_restart is 0. For DeleteFile, since good_bundle, needs_shutdown is 0 and needs_restart is 1. That leads the rest of the test logic to be wrong.

I do not think that good_bundle should be 1 since the executable got removed, but I think perhaps the logic that checks whether the file is in NSTrashDirectory is wrong (now?). Still looking a bit at this.

rsesek@, do you know if 10.14 changed things relevant to ServiceProcessStateFileManipulationTest.DeleteFile?

I think it's an error to try and operate on file reference URLs like real filepaths. I'm surprised treating it as such even works today. Maybe need to call -filePathURL on it to go back to the actual path.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5100866c9bad39e3075bf1e064f89a9fac76a57

commit c5100866c9bad39e3075bf1e064f89a9fac76a57
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Sat Nov 03 00:21:16 2018

mac: update ViewTreeValidator for 10.14

This change has ViewTreeValidator account for the changed interior structure
of NSButton on 10.14. This fixes FirstRunDialogControllerTest's
LayoutWithLongStrings.

Bug:  892176 
Change-Id: Icc9e977034a98fbb587719770c326d082adb3743
Reviewed-on: https://chromium-review.googlesource.com/c/1316113
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605105}
[modify] https://crrev.com/c5100866c9bad39e3075bf1e064f89a9fac76a57/ui/base/test/view_tree_validator.mm

#3: I've fixed that, but the test seems to be flaky on 10.14 still. In both passing and failing cases the order of events seems to be the same:

1) PostTask in the test
2) Run in the test
3) DeleteFunc is called
4) ExecFilePathWatcherCallback::NotifyPathChanged is called on the same path
5) The test's assert is called

But in the failing case, in NotifyPathChanged, [executable_fsref_ filePathURL] returns a path, and in the passing case, it returns nil!

Do you understand if this test is expecting [executable_fsref_ filePathURL] to return nil inside NotifyPathChanged? I sort of expect that it is but that we are racing some other process (maybe a cache invalidation). Adding any sort of delay at the start of NotifyPathChanged (like base::debug::StackTrace().Print() for example) causes the test to always pass.

Current theory, developed in consultation with mark@: the kqueue sometimes delivers the "file deleted" event *before* the file is actually deleted off disk. It isn't a user-level cache issue - I added code to check whether base::PathExists(...) in NotifyPathChanged() and in the passing case that returns false but in the failing case that returns true. Also, switching in ServiceProcessState::StateData::WatchExecutable() from the kqueue event backend to the FSEvents backend (by passing true for recursive) makes this bug go away, so I'm starting to strongly suspect kqueue.

Unfortunately there are no really palatable fixes:

1) Mark the test as flaky (probably what I will actually do)
2) Have FilePathWatcherKQueue work around this problem internally, probably by PostTask()ing to check again in "a while" if the file is still there - mark@ correctly points out that this is still racy, though, just less often.
3) Switch to always using FilePathWatcherFSEvents for the executable, although mark@ says that FSEvents have their own deficiencies, like events sometimes disappearing into the formless void.
4) Change the API of FilePathWatcher so that it passes along what the FilePathWatcher thinks happened (probably by adding a bitmask of events, or an event type), and then have ExecFilePathWatcherCallback::NotifyPathChanged() trust a "delete" event and not check on its own. There are not too many clients of FilePathWatcher but this would involve some cross-platform //base hacking.

For now I will mark this test as flaky and file a bug for that. :(

+cc mark@

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af2a90f55444d56c9529eb20ab499c4ccc5e6bfd

commit af2a90f55444d56c9529eb20ab499c4ccc5e6bfd
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Mon Nov 12 13:15:14 2018

mac: mark ServiceProcessStateFileManipulationTest DeleteFile as flaky

Bug: 903823,  892176 
Change-Id: I30a2f92478a4b260191deb7faf4aa2a21acc2609
Reviewed-on: https://chromium-review.googlesource.com/c/1329346
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607208}
[modify] https://crrev.com/af2a90f55444d56c9529eb20ab499c4ccc5e6bfd/chrome/common/service_process_util_mac_unittest.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f63790e2a15d7f6c94ed12871826116c05dec790

commit f63790e2a15d7f6c94ed12871826116c05dec790
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Tue Nov 13 19:01:40 2018

ui: fix ViewTreeValidatorTest on 10.14

This test uses NSButton as its dummy class for some test cases, but there is
now a 10.14-specific behavior in ViewTreeValidator to ignore children of
buttons because of the linked bug, which causes this test to fail on 10.14.

Bug:  892176 
Change-Id: I35bac3065ed1b2ded9466e63b6c27cd804b7180c
Reviewed-on: https://chromium-review.googlesource.com/c/1333140
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607685}
[modify] https://crrev.com/f63790e2a15d7f6c94ed12871826116c05dec790/ui/base/test/view_tree_validator_unittest.mm

These are green on recent runs.