Re crrev.com/573620, I think the issue there was that WM_QUIT is the expected quit signal (graceful teardown, equivalent to SIGTERM) in that case.

Re. #3, right, but it has a 50/50 chance that the WM_QUIT is caught by the nested ::GetMessage() loop and then wouldn't result in quitting the RunLoop anyways... Unless it wasn't inside a ::GetMessage() loop (in which case reposting the WM_QUIT was irrelevant).

So at least one portion of that change was not necessary.

If reposting to the native subpump is the necessary bit, then my change fixes that too and we can remove EnableWmQuit().

If handling WM_QUIT like a QuitClosure() from the main RunLoop is the necessary bit (and there isn't a native ::GetMessage() loop), then we need to keep EnableWmQuit() (as my CL currently does).

The problem is that no printing tests were added in that CL, so I have no way to tell if keeping EnableWmQuit() is necessary...

EnableWmQuit() is likely still necessary. Would it help if I wrote a test now?

Yes it would, thanks

Sure, I got a test. Will clean it up and send it out soon.

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

commit 6b9a2faf89e9b67fb6cb8d0986c431923f757b98
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Sep 28 21:14:29 2018

Ensure that WM_QUIT is visible to nested ::GetMessage() loops.

When a kMsgHaveWork task is processed by the pump, allowing it a
timeslice in which to process tasks, we pull the next MSG from the
thread's queue and dispatch that, to minimize impact of kMsgHaveWork on
MSG scheduling. WM_QUIT must be observed directly by GetMessage in order
for a nested loop to exit, though, so we must re-post it to the thread
queue, for GetMessage to report.

Re-posting was removed by crrev.com/553653, which caused WM_QUIT to be
ignored by MessagePumpForUI, missing that we must still re-post the
message, in case we're inside a nested loop, to allow it to quit.

To address this:
- When pulling a MSG to replace kMsgHaveWork, we unconditionally re-post
  WM_QUIT messages, in case ::GetMessage() needs to see them.
- Only actually handle WM_QUIT when it is observed directly by
  DoRunLoop(), i.e. if WM_QUIT was the kMsgHaveWork replacement, don't
  process it until we see it re-posted.
- Continue ignoring WM_QUIT when handled iff |!enable_wm_quit_| to
  properties of crrev.com/573620.
- Add comments and tests to properly explain and verify the special-case
  handling of WM_QUIT.
- Document a another edge case bug uncovered by this investigation.

Bug:  888559 , 890016
Change-Id: I548165b6103b1c7454f90335ba6dfb4f8cf149d9
Reviewed-on: https://chromium-review.googlesource.com/1240158
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595217}
[modify] https://crrev.com/6b9a2faf89e9b67fb6cb8d0986c431923f757b98/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/6b9a2faf89e9b67fb6cb8d0986c431923f757b98/base/message_loop/message_pump_win.cc