Skip to content
  • Gabriel Charette's avatar
    Give NestableTasksAllowed() control back to MessageLoop. · b030a4a0
    Gabriel Charette authored
    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: default avatarRobert Liao <robliao@chromium.org>
    Reviewed-by: default avatardanakj <danakj@chromium.org>
    Commit-Queue: Gabriel Charette <gab@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#511673}
    b030a4a0