New issue
Advanced search Search tips

Issue 822275 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Implement an auto-advance-off mode for NavigationSimulator

Project Member Reported by csharrison@chromium.org, Mar 15 2018

Issue description

Sophisticated NavigationThrottle tests may want to use another task runner for async tests other than the one the main thread runs on.

Right now, this is not possible since the NavigationThrottle internally spins a RunLoop, which blocks the test from making progress on any other task runners.

Since most NavigationSimulator consumers prefer the auto-advance functionality, this could be an optional parameter SetAutoAdvance(bool). If false, the client can call Wait() on the simulator to manually advance after they've done the processing they need.
 
See ActivationStateComputingThrottleSubFrameTest.SpeculativeSubframesWithDelay for an example test which would be improved with this sort of API.
Here is a simple example test we could write with the new system. In general this approach avoids a callback-driven design (like what was implemented before, an OnDeferred callback), in favor of simpler linear tests. The ability to use e.g. mock time task runners is very powerful. This test just does something silly but you could start testing things like cancellation or timeouts using mock time.

// Defers a navigation in some cases, via some async work on another task runner.
class DelayingThrottle;

auto task_runner = base::MakeRefCounted<TestMockTimeTaskRunner>();
TestNavigationThrottleInserter inserter(base::BindOnce(&CreateThrottle, task_runner));
auto simulator = NavigationSimulator::CreateRendererInitiated(url, main_rfh());
simulator->SetAutoAdvance(false);

simulator->Start();
EXPECT_TRUE(simulator->IsDeferred());

// Calling Wait() here would hang, since we need to fast forward time manually
// on the mock time task runner, which  should triggle the throttle to post a
// task to resume the navigation.
task_runner->FastForwardBy(kDelayMs);
simulator->Wait();

simulator->ReadyToCommit();
EXPECT_FALSE(simulator->IsDeferred());
simulator->Commit();
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 27 2018

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

commit a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8
Author: Charlie Harrison <csharrison@chromium.org>
Date: Tue Mar 27 18:44:04 2018

NavigationSimulator: Refactor of WaitForThrottleChecksComplete

The end goal is to get NavigationSimulator to a state where we can call:
simulator->SetAutoAdvance(false);
simulator->Start();

And have the simulator *not* spin the run loop in the background. This
will help in writing more sophisticated NavigationThrottle tests which
use multiple task runners to execute async tasks.

This particular CL does a few things:
1. Removes base::RunLoop().RunUntilIdle call. This is not great practice
   to have in test infra, since it can cause strange interactions with
   tests and they may start relying on unintentional behavior.

2. Collects various after-throttle checks into closures, to be called
   when the throttle check complete closure is called (rather than
   relying on base::RunLoop to keep everything on the stack). This
   isn't so useful now, but it will if we stop *always* waiting.

3. Slightly modifies checks in various places that relied on some of
   the behavior that we've changed. Two things changed here:
   a. "Complete" code internal to the NavigationThrottle now runs
      directly after throttle checks are complete (when
      complete_callback_for_testing_ is called on the nav handle),
      rather than when the RunLoop quits. This caused us to remove
      one check checking that we've finished the navigation.

   b. Because we no longer RunUntilIdle() in
      WaitForThrottleChecksComplete, code that relies on the nav finishing
      after the post-task in NavigationRequest::OnStartChecksComplete
      will need to either wait or spin the run loop to see the end of the
      navigation. This is a pretty small edge case and it is pretty
      easy to either write a helper to make this explicit or use a WCO.
      The only test impacted by this change is a NavigationSimulator test,
      and a test which incidentally relied on this behavior for unrelated
      reasons.

Bug:  822275 

Change-Id: Ice6b215fa285a6a944fdf85108303515d109be77
Reviewed-on: https://chromium-review.googlesource.com/961819
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546184}
[modify] https://crrev.com/a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8/chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc
[modify] https://crrev.com/a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8/components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc
[modify] https://crrev.com/a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8/content/public/test/navigation_simulator.h
[modify] https://crrev.com/a4fdbbaf8b99ba70b29c1f9192ba598c2e1c58f8/content/test/navigation_simulator_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 27 2018

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

commit 5cc740dc16144680a94bcba70721d837364343e4
Author: Charlie Harrison <csharrison@chromium.org>
Date: Tue Mar 27 20:52:46 2018

NavigationSimulator: Implement Wait() and SetAutoAdvance(false)

This CL augments the NavigationSimulator to allow tests to drive it
manually through its stages. This is implemented via a method
SetAutoAdvance(bool).

The CL also implements a Wait() method which spins the run loop waiting
for throttle checks to complete, and an IsDeferred() method which returns
whether some throttle is currently deferring the navigation.

This is used to implement one test outside of //content.

Bug:  822275 
Change-Id: I394e33cf89ee298c92d2c6554ca0c591e080a695
Reviewed-on: https://chromium-review.googlesource.com/964389
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546248}
[modify] https://crrev.com/5cc740dc16144680a94bcba70721d837364343e4/components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc
[modify] https://crrev.com/5cc740dc16144680a94bcba70721d837364343e4/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/5cc740dc16144680a94bcba70721d837364343e4/content/public/test/navigation_simulator.h
[modify] https://crrev.com/5cc740dc16144680a94bcba70721d837364343e4/content/test/navigation_simulator_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment