New issue
Advanced search Search tips

Issue 897925 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 24
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Flaky-Test: SecurityKeyMessageReaderImplTest.MultipleMessages



Sign in to add a comment

SecurityKeyMessageReaderImplTest.MultipleMessages is flaky

Project Member Reported by Findit, Oct 22

Issue description

Labels: -Sheriff-Chromium
Owner: gab@chromium.org
Status: Assigned (was: Untriaged)
Reverted the culprit at https://chromium-review.googlesource.com/c/chromium/src/+/1294717

Hi gab@, PTAL. Thanks!

(p.s. I forgot to list this bug in the revert patch)
FYI: Test is no longer flaky after revert.

Comment 3 Deleted

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 23

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

commit c8a1dc548deacfd28e32e5f1c54b02ec59dfc911
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Oct 23 23:10:55 2018

Reland "[MessageLoop] Lock-free ScheduleWork() scheme"

This is a reland of f7da13a28c3799819bd39c4d95dc02a805b27e4f
(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/1297129
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602166}
[modify] https://crrev.com/c8a1dc548deacfd28e32e5f1c54b02ec59dfc911/base/message_loop/message_loop.cc
[modify] https://crrev.com/c8a1dc548deacfd28e32e5f1c54b02ec59dfc911/base/message_loop/message_loop.h
[modify] https://crrev.com/c8a1dc548deacfd28e32e5f1c54b02ec59dfc911/base/message_loop/message_pump.h
[modify] https://crrev.com/c8a1dc548deacfd28e32e5f1c54b02ec59dfc911/base/message_loop/message_pump_android.cc
[modify] https://crrev.com/c8a1dc548deacfd28e32e5f1c54b02ec59dfc911/base/threading/thread_restrictions.h

Status: Fixed (was: Assigned)

Sign in to add a comment