This was broken by https://chromium-review.googlesource.com/c/594713

 Issue 768911  has been merged into this issue.

Oh crap... I messed that up pretty bad. Thought it was a bogus check and was about to remove it but I just realized it's in fact highlighting a big problem with https://chromium-review.googlesource.com/c/594713.

That change is based on the assumption that RunLoop::Run() is the *only* thing to ever cause nesting (or at least nesting that causes chrome tasks to run, i.e. system loops are okay because they don't re-enter chrome).

But... MessageLoopForIO works the other way around. The OS is in charge of the loop and MessagePumpForIO merely hands a callback to the OS to be called for work requests when the IOCompletionPort fires.

So when there's a system loop on a MessageLoopForIO, it can actually end up processing a MessagePumpForIO callback and re-entering chrome without being in nested RunLoop.

The aforementioned makes it such that NestableTasksAllowed() will now return true when this happens (per not being in a nested RunLoop) which throws off all of the MessageLoop::DoWork() logic w.r.t deferring nestable tasks =S.

If this was only an issue with printing I wouldn't worry too much, but it's actually an issue with any call that generates a system loop on a MessageLoopForIO =S.

And that as of M62 when that CL landed.

RunLoop::Delegate::Client::ProcessingTasksAllowed() needs a notion of scope of the task it allowed to run or something so that it only allows one task at a time in scenarios (rather than always return true for top-level RunLoop).

Thinking of a simple/mergeable solution...

I'll pick this up as gab@ is OOO.

I updated the description with the DCHECKing stack. We definitely have a RunLoop on it (stack frame 16). 

0:151> ?? this->outer_->active_run_loops_.c._Mypair._Myval2
class std::_Vector_val<std::_Simple_types<base::RunLoop *> >
   +0x000 _Myproxy         : 0x2d7871c8 std::_Container_proxy
   +0x004 _Myfirst         : 0x2d322380  -> 0x2afffed0 base::RunLoop
   +0x008 _Mylast          : 0x2d322384  -> 0xfdfdfdfd base::RunLoop
   +0x00c _Myend           : 0x2d322384  -> 0xfdfdfdfd base::RunLoop

ProcessingTasksAllowed() is
  return outer_->active_run_loops_.size() == 1U ||
         outer_->active_run_loops_.top()->type_ == Type::kNestableTasksAllowed;

Since the active_run_loops_.size() == 1 in this case (32-bit machine), then this function says nesting is allowed.

Incidentally

0:151> ?? this->outer_
class base::RunLoop::Delegate * 0x2d7815fc
   +0x000 __VFN_table : 0x103cc654 
   +0x004 allow_nesting_   : 1 
   +0x008 active_run_loops_ : std::stack<base::RunLoop *,std::vector<base::RunLoop *,std::allocator<base::RunLoop *> > >
   +0x018 nesting_observers_ : base::ObserverList<base::RunLoop::NestingObserver,0>
   +0x034 allow_running_for_testing_ : 1
   +0x035 bound_           : 1
   +0x038 bound_thread_checker_ : base::ThreadChecker
   +0x04c client_interface_ : base::RunLoop::Delegate::Client

Reassigning back to gab@ after clarifications.

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

commit b030a4a0884a87640ad65e51b20847c6213b8eab
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Oct 26 01:04:40 2017

Give NestableTasksAllowed() control back to MessageLoop.

While I still think RunLoop::Type::kNestableTasksAllowed is the right
API to allow nesting of application tasks in the usual case:
RunLoop::Delegate::Client::ProcessingTasksAllowed() wasn't the right
level of abstraction for the allowance (even without the bug it suffered).

When I hit https://bugs.chromium.org/p/chromium/issues/detail?id=754112#c6
I initially thought the solution was for Client::ProcessingTasksAllowed()
to return a scoped token to allow only a single application task at once
per RunLoop instance. But implementing it (see PS1) it became apparent
that this wasn't generally something most RunLoop::Delegates cared about.
Rather it was a fallout of some MessageLoops which permit reentrancy
without a nested RunLoop being involved (e.g. MessageLoop::Run() ->
MessageLoop::RunTask() can go through an OS-driven MessagePump which can
in some scenarios trigger reentrancy without a second call to Run()).

This CL keeps the API in RunLoop to declare allowance but then tells
RunLoop::Delegate::Run() via param whether it should allow application
tasks for that layer. Leaving it up to the impl to determine how to
control that.

This ends up getting rid of the duality of the previous check in 
NestableTasksAllowed() as well. Now centralizing the logic in MessageLoop.

Much simpler than the alternative (again, see PS1).

While PS9 ended up keeping usage of a bool to control reentrancy (usage
of racing counters (allowed vs used) proved useless as they were either 
equal or one off), it was renamed away from referring to "nestable tasks" 
as this makes the nomenclature very confusing with "nested loops" which 
are not the same thing at all. Instead we just name it 
"task_execution_allowed_" and leave it up to MessageLoop logic 
(DeferOrRunPendingTask()) to know to skip non-nestable tasks when inside 
a nested loop.

For reference, see PS16-19 for attempts at making RunLoop be the sole
controller of nesting/nestable tasks allowance. This was dropped for now as
it proved to be ugly in conjunction with deprecated MessageLoop::*Nestable*
APIs that still need to be supported until callers are migrated.

Bug:  754112 
Change-Id: I9f3a8431c00f69b0b5bb696e1e86e32bba6e6ffe
Reviewed-on: https://chromium-review.googlesource.com/730864
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511673}
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/message_loop/message_loop.cc
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/message_loop/message_loop.h
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/run_loop.cc
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/run_loop.h
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/run_loop_unittest.cc
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/test/test_mock_time_task_runner.cc
[modify] https://crrev.com/b030a4a0884a87640ad65e51b20847c6213b8eab/base/test/test_mock_time_task_runner.h

Assuming it's too late for M62 (this bug is likely bad but we don't have hard proof of something bad it causes)?

But we should at least merge for M63.

This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

FYI, I have verified this fix (requires a DCHECK build to easily verify so can't be verified on Canary), but the improper runtime behavior resulting from continuing when the DCHECK doesn't fire is bad and we should fix it in the earliest milestone possible (the DCHECK also happens to be in printing that this affects much more than printing in practice, see comment #6 above).

Issue 778260 has been merged into this issue.

You can verify this in the SyzyASAN canary, make sure the chrome://flags/#dcheck-is-fatal flag is "Enabled".

I verified locally already in a dcheck build, a live build provides no further evidence and I'm not setup for SyzyASAN at the moment (intentionally because it borks my performance evaluations for scheduling purposes; despite loving Albatros builds... keep it up!).

Approving merge to M63 branch 3239 based on comment #17, #20 and per offline chat with gab@.

+ abdulsyed@ for M62 merge review

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

commit 8661c6c3eacf31fbbc5c202a844b06bd0381e571
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Oct 31 18:52:19 2017

[Merge to M63] Give NestableTasksAllowed() control back to MessageLoop.

While I still think RunLoop::Type::kNestableTasksAllowed is the right
API to allow nesting of application tasks in the usual case:
RunLoop::Delegate::Client::ProcessingTasksAllowed() wasn't the right
level of abstraction for the allowance (even without the bug it suffered).

When I hit https://bugs.chromium.org/p/chromium/issues/detail?id=754112#c6
I initially thought the solution was for Client::ProcessingTasksAllowed()
to return a scoped token to allow only a single application task at once
per RunLoop instance. But implementing it (see PS1) it became apparent
that this wasn't generally something most RunLoop::Delegates cared about.
Rather it was a fallout of some MessageLoops which permit reentrancy
without a nested RunLoop being involved (e.g. MessageLoop::Run() ->
MessageLoop::RunTask() can go through an OS-driven MessagePump which can
in some scenarios trigger reentrancy without a second call to Run()).

This CL keeps the API in RunLoop to declare allowance but then tells
RunLoop::Delegate::Run() via param whether it should allow application
tasks for that layer. Leaving it up to the impl to determine how to
control that.

This ends up getting rid of the duality of the previous check in
NestableTasksAllowed() as well. Now centralizing the logic in MessageLoop.

Much simpler than the alternative (again, see PS1).

While PS9 ended up keeping usage of a bool to control reentrancy (usage
of racing counters (allowed vs used) proved useless as they were either
equal or one off), it was renamed away from referring to "nestable tasks"
as this makes the nomenclature very confusing with "nested loops" which
are not the same thing at all. Instead we just name it
"task_execution_allowed_" and leave it up to MessageLoop logic
(DeferOrRunPendingTask()) to know to skip non-nestable tasks when inside
a nested loop.

For reference, see PS16-19 for attempts at making RunLoop be the sole
controller of nesting/nestable tasks allowance. This was dropped for now as
it proved to be ugly in conjunction with deprecated MessageLoop::*Nestable*
APIs that still need to be supported until callers are migrated.

TBR=danakj@chromium.org, robliao@chromium.org

(cherry picked from commit b030a4a0884a87640ad65e51b20847c6213b8eab)

Bug:  754112 
Change-Id: I9f3a8431c00f69b0b5bb696e1e86e32bba6e6ffe
Reviewed-on: https://chromium-review.googlesource.com/730864
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#511673}
Reviewed-on: https://chromium-review.googlesource.com/747281
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#319}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/message_loop/message_loop.cc
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/message_loop/message_loop.h
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/run_loop.cc
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/run_loop.h
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/run_loop_unittest.cc
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/test/test_mock_time_task_runner.cc
[modify] https://crrev.com/8661c6c3eacf31fbbc5c202a844b06bd0381e571/base/test/test_mock_time_task_runner.h

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

commit a21187a5dca9fb05d0697c50c08788796127dd3e
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Oct 31 20:35:07 2017

Revert "[Merge to M63] Give NestableTasksAllowed() control back to MessageLoop."

This reverts commit 8661c6c3eacf31fbbc5c202a844b06bd0381e571.

Reason for revert: known to cause  crbug.com/778562 , will reland merge + fix when we have it.

Original change's description:
> Give NestableTasksAllowed() control back to MessageLoop.
>
> While I still think RunLoop::Type::kNestableTasksAllowed is the right
> API to allow nesting of application tasks in the usual case:
> RunLoop::Delegate::Client::ProcessingTasksAllowed() wasn't the right
> level of abstraction for the allowance (even without the bug it suffered).
>
> When I hit https://bugs.chromium.org/p/chromium/issues/detail?id=754112#c6
> I initially thought the solution was for Client::ProcessingTasksAllowed()
> to return a scoped token to allow only a single application task at once
> per RunLoop instance. But implementing it (see PS1) it became apparent
> that this wasn't generally something most RunLoop::Delegates cared about.
> Rather it was a fallout of some MessageLoops which permit reentrancy
> without a nested RunLoop being involved (e.g. MessageLoop::Run() ->
> MessageLoop::RunTask() can go through an OS-driven MessagePump which can
> in some scenarios trigger reentrancy without a second call to Run()).
>
> This CL keeps the API in RunLoop to declare allowance but then tells
> RunLoop::Delegate::Run() via param whether it should allow application
> tasks for that layer. Leaving it up to the impl to determine how to
> control that.
>
> This ends up getting rid of the duality of the previous check in
> NestableTasksAllowed() as well. Now centralizing the logic in MessageLoop.
>
> Much simpler than the alternative (again, see PS1).
>
> While PS9 ended up keeping usage of a bool to control reentrancy (usage
> of racing counters (allowed vs used) proved useless as they were either
> equal or one off), it was renamed away from referring to "nestable tasks"
> as this makes the nomenclature very confusing with "nested loops" which
> are not the same thing at all. Instead we just name it
> "task_execution_allowed_" and leave it up to MessageLoop logic
> (DeferOrRunPendingTask()) to know to skip non-nestable tasks when inside
> a nested loop.
>
> For reference, see PS16-19 for attempts at making RunLoop be the sole
> controller of nesting/nestable tasks allowance. This was dropped for now as
> it proved to be ugly in conjunction with deprecated MessageLoop::*Nestable*
> APIs that still need to be supported until callers are migrated.
>
> Bug:  754112 
> Change-Id: I9f3a8431c00f69b0b5bb696e1e86e32bba6e6ffe
> Reviewed-on: https://chromium-review.googlesource.com/730864
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#511673}

TBR=danakj@chromium.org, gab@chromium.org, robliao@chromium.org


Bug:  754112 
Change-Id: I7dcc7521072f867f9513feafc8fb4ffacab1fad9
Reviewed-on: https://chromium-review.googlesource.com/747529
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#324}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/message_loop/message_loop.cc
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/message_loop/message_loop.h
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/run_loop.cc
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/run_loop.h
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/run_loop_unittest.cc
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/test/test_mock_time_task_runner.cc
[modify] https://crrev.com/a21187a5dca9fb05d0697c50c08788796127dd3e/base/test/test_mock_time_task_runner.h

Adding back "Merge-Review-63" per comment #23.

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

commit f6a6fe5953ce2c84ebab19b90f43c92493460861
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Nov 02 19:55:48 2017

Reland "[Merge to M63] Give NestableTasksAllowed() control back to MessageLoop."

This is a reland of 8661c6c3eacf31fbbc5c202a844b06bd0381e571
Original change's description:
> [Merge to M63] Give NestableTasksAllowed() control back to MessageLoop.
> 
> While I still think RunLoop::Type::kNestableTasksAllowed is the right
> API to allow nesting of application tasks in the usual case:
> RunLoop::Delegate::Client::ProcessingTasksAllowed() wasn't the right
> level of abstraction for the allowance (even without the bug it suffered).
> 
> When I hit https://bugs.chromium.org/p/chromium/issues/detail?id=754112#c6
> I initially thought the solution was for Client::ProcessingTasksAllowed()
> to return a scoped token to allow only a single application task at once
> per RunLoop instance. But implementing it (see PS1) it became apparent
> that this wasn't generally something most RunLoop::Delegates cared about.
> Rather it was a fallout of some MessageLoops which permit reentrancy
> without a nested RunLoop being involved (e.g. MessageLoop::Run() ->
> MessageLoop::RunTask() can go through an OS-driven MessagePump which can
> in some scenarios trigger reentrancy without a second call to Run()).
> 
> This CL keeps the API in RunLoop to declare allowance but then tells
> RunLoop::Delegate::Run() via param whether it should allow application
> tasks for that layer. Leaving it up to the impl to determine how to
> control that.
> 
> This ends up getting rid of the duality of the previous check in
> NestableTasksAllowed() as well. Now centralizing the logic in MessageLoop.
> 
> Much simpler than the alternative (again, see PS1).
> 
> While PS9 ended up keeping usage of a bool to control reentrancy (usage
> of racing counters (allowed vs used) proved useless as they were either
> equal or one off), it was renamed away from referring to "nestable tasks"
> as this makes the nomenclature very confusing with "nested loops" which
> are not the same thing at all. Instead we just name it
> "task_execution_allowed_" and leave it up to MessageLoop logic
> (DeferOrRunPendingTask()) to know to skip non-nestable tasks when inside
> a nested loop.
> 
> For reference, see PS16-19 for attempts at making RunLoop be the sole
> controller of nesting/nestable tasks allowance. This was dropped for now as
> it proved to be ugly in conjunction with deprecated MessageLoop::*Nestable*
> APIs that still need to be supported until callers are migrated.
> 
> TBR=danakj@chromium.org, robliao@chromium.org
> 
> (cherry picked from commit b030a4a0884a87640ad65e51b20847c6213b8eab)
> 
> Bug:  754112 
> Change-Id: I9f3a8431c00f69b0b5bb696e1e86e32bba6e6ffe
> Reviewed-on: https://chromium-review.googlesource.com/730864
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#511673}
> Reviewed-on: https://chromium-review.googlesource.com/747281
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3239@{#319}
> Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}

TBR=danakj@chromium.org,robliao@chromium.org

Bug:  754112 
Change-Id: I8edbeb07e3dd2eb94029965aa4192e462316fd62
Reviewed-on: https://chromium-review.googlesource.com/752041
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#352}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/message_loop/message_loop.cc
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/message_loop/message_loop.h
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/run_loop.cc
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/run_loop.h
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/run_loop_unittest.cc
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/test/test_mock_time_task_runner.cc
[modify] https://crrev.com/f6a6fe5953ce2c84ebab19b90f43c92493460861/base/test/test_mock_time_task_runner.h

Is there anything left for M63? If not, pls remove "Merge-Review-63" label. Thank you.

I think we're good to go here for now. I'll remove the label. Thanks!