1524072 - remove chromium's WaitableEvent code

Status: NEW

(Core :: IPC, enhancement, P1)

Description

[Nathan Froyd [:froydnj]]

See the commit message for part 4 for why this seems like a reasonable thing to do.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make MessagePumpDefault::event_ private

There's no need for subclasses to be twiddling this variable, and it
makes our life easier in the next patch.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move MessagePumpDefault away from using WaitableEvent

This patch is complicated, but that's mostly because waiting for condition
variables on timeouts is complicated if you want to be robust about it.

This patch is also *possible* justification for keeping WaitableEvent, as the
code with WaitableEvent is significantly nicer, but I think we're better off
without WaitableEvent (and the Chromium lock code, see part 4).

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - use a regular Monitor/bool combination in chromium Thread

Longer than using WaitableEvent, but less magical.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove ipc/chromium WaitableEvent

Chromium's WaitableEvent is, on Windows, a thin wrapper around Windows
event objects.  On POSIX systems, it's something rather more
complicated.  The primary advantages of it seem to be twofold:

1. You can wait on multiple events with ease.
2. It's shorter than using a monitor + boolean variable.

The latter capability is not used in our codebase (and seems to only be
rarely used in Chromium's present codebase).  The latter advantage is
certainly true, but the implementation of WaitableEvent on POSIX systems
is complicated enough (and involves things like careful manipulation of
lock acquisition/release in non-LIFO order) that it seems reasonable to
just write out the monitor/boolean logic in the two places we use
WaitableEvent.  Previous patches have done just that, and this patch
removes the now-unused WaitableEvent code.

(Doing this also means that it's significantly easier to get rid of the
Chromium lock code, as the POSIX WaitableEvent implementation was the
last client of the Chromium lock code.  Replacing the Chromium locks
there with mozilla::Mutex was complicated by the aforementioned non-LIFO
lock manipulation, as the deadlock detector was quite unhappy about said
non-LIFO shenanigans.)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:froydnj, could you have a look please?