New issue
Advanced search Search tips

Issue 828400 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Android: Reloading after a killed page races with a subsequent load.

Project Member Reported by mattcary@chromium.org, Apr 3 2018

Issue description

Googlers see b/74022057 for additional context. Most of the details of this bug are public, however, and I'll try to explain everything on this bug.

If a renderer on url A is killed right when a load of url B occurs in the same renderer, the reload of page A can happen after the load of B starts, causing the renderer to end up on url A instead of the expected B.

This bug was discovered in the context of custom tabs. If a background (speculation) renderer to a url A is started, commits, and then is killed at the same time that a load to a new url B that matches A, the now-killed speculative renderer will be used for the load of B. It can happen that the reload scheduled after the kill by TabWebContentsObserver.renderProcessGone will occur after the load to url B starts, replacing that pending load and causing the background tab to display url A instead of the expected B.

See also CustomTabsConnection.launchUrlInHiddenTab and takeHiddenTab for additional context.

A fix for this seems to be for didStartPageLoad to unset mNeedsReload. As all the relevant Tab methods are run on the UI thread, this is enough to break the race.
 
Status: Started (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 10 2018

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

commit 00fc4acd9fb7d947babd659b24150c4af41035ce
Author: Matthew Cary <mattcary@chromium.org>
Date: Tue Apr 10 11:40:28 2018

Clank: Stop reloading after a new load.

This prevents a reload scheduled after TabWebContentsObserver.renderProcessGone
from racing with a subsequent page load.

Bug:  828400 
Change-Id: I5743736aded76a227a1cda0a4fa1824751df9a3f
Reviewed-on: https://chromium-review.googlesource.com/992612
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549488}
[modify] https://crrev.com/00fc4acd9fb7d947babd659b24150c4af41035ce/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
[modify] https://crrev.com/00fc4acd9fb7d947babd659b24150c4af41035ce/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/00fc4acd9fb7d947babd659b24150c4af41035ce/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
[modify] https://crrev.com/00fc4acd9fb7d947babd659b24150c4af41035ce/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java
[modify] https://crrev.com/00fc4acd9fb7d947babd659b24150c4af41035ce/chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java

FYI: search is running an experiment on M67, which this fix is in beginning with 67.0.3394.0.
Status: Fixed (was: Started)
Search experiment hasn't shown more crashes. Will close this bug.

Sign in to add a comment