• Gabriel Charette's avatar
    Reland "[MessageLoop] Lock-free ScheduleWork() scheme" · c8a1dc54
    Gabriel Charette authored
    This is a reland of f7da13a2
    (original change in PS1)
    
    The original change was missing acquire ordering in
    DisconnectFromParent(). This is necessary in order for
    the disconnecting thread to see all memory side-effects
    previously made by other threads (or some side-effects
    of message_loop_->ScheduleWork() could racily come in
    after ~MessageLoop()).
    
    Also removed the DCHECK that
    |operations_state_ == kDisconnectedBit| at the end of
    DisconnectFromParent() as it was incorrect. A racy
    BeforeOperation() call can make it 1->3 (and no-op) after
    DisconnectFromParent() made it 0->1.
    
    And lastly, added a scoped allowance to always allow the
    very fast wait instead of requiring callers to know about
    this implementation detail of MessageLoop (and reverted
    changes to //net).
    
    Original change's description:
    > [MessageLoop] Lock-free ScheduleWork() scheme
    >
    > The Lock is causing hangs because of priority inversion
    > mixed with priority boosting (ScheduleWork() tends to
    > boost the destination thread which may deschedule the
    > posting thread; if the posting thread is a background
    > thread this boost-induded-desched-while-holding-lock
    > can cause a livelock). See https://crbug.com/890978#c10
    > for example crashes catching this.
    >
    > The Lock was only necessary for startup/shutdown and is
    > being replaced by a lock-free atomic scheme in this CL.
    >
    > MessagePump::ScheduleWork() itself was already thread-safe
    > (but the Android impl did unnecessarily check a non-atomic bool)
    >
    > This adds a WaitableEvent in ~MessageLoop(); hence the requirement
    > for a wait-allowance in net's EmbeddedTestServer.
    >
    > TBR=zhongyi@chromium.org (embedded_test_server.cc side-effects)
    >
    > Bug: 890978, 874237
    > Change-Id: I0916e5a99035a935b0a23a770af256f334e78c43
    > Reviewed-on: https://chromium-review.googlesource.com/c/1278631
    > Commit-Queue: Gabriel Charette <gab@chromium.org>
    > Reviewed-by: François Doray <fdoray@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#601600}
    
    Bug: 890978, 874237, 897925
    Change-Id: I17c515f9a3169bbdfc303a4b259f34097e31730d
    Reviewed-on: https://chromium-review.googlesource.com/c/1297129Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
    Commit-Queue: Gabriel Charette <gab@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#602166}
    c8a1dc54
thread_restrictions.h 17.6 KB