New issue
Advanced search Search tips

Issue 829381 link

Starred by 2 users
Status: Fixed
Owner:
mattcary@chromium.org
Closed: May 2018
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Tab.java needsReload duplicates navigation_controller needs_reload_.

Project Member Reported by mattcary@chromium.org, Apr 5 2018 
As stated in the summary.

It appears that the native needs_reload is only used when the tab is explicitly discarded [1]. The android version of needsReload is used when the tab is killed by the system, which is common on android (in other platforms it will just sad-tab).

It looks like since Tab.java:mNeedsReload only ends up kicking off a call to navigation_controller_->SetNeedsReload(), it would be equivalent to just set that directly. This would avoid the problem of  crbug.com/828400  where needs_reload is not reset when a new page load comes in.

However, there is a comment in tab_lifecycle_unit.cc suggesting that the native needs_reload is changing/going away [2]. This should be understood before tearing out the android path.


[1] https://cs.chromium.org/chromium/src/chrome/browser/resource_coordinator/tab_manager.cc?rcl=7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6&l=305

[2] https://cs.chromium.org/chromium/src/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc?rcl=b3b1d34a763ebf8a5153c74d4917fcb09b40a2e5&l=63
Project Member

Comment 1 by bugdroid1@chromium.org, May 2 2018 

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

commit e7da4b19953078f923b3e2359db9b199f73463fe
Author: Matthew Cary <mattcary@chromium.org>
Date: Wed May 02 16:14:16 2018

Android: remove separate reload tracking from Tab.java

Removes the mNeedsReload flag in Tab.java which was just a thin
and partial wrapper around the navigation controller's needs_reload
mechanism. This duplication caused at least one race
condition ( crbug.com/828400 ) and will also complicate future
LifecycleManager work.

All of the checking in Tab.java of mNeedsReload was duplicated by the
navigation controller's needs_reload flag, as used in
NavigationControllerImpl::LoadIfNecessary.

Bug:  829381 
Change-Id: Id17c9d9f89088c718ae4a927ea5b8dbe95126e68
Reviewed-on: https://chromium-review.googlesource.com/1006958
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555414}
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/e7da4b19953078f923b3e2359db9b199f73463fe/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Comment 2 by mattcary@chromium.org, May 4 2018

Status: Fixed (was: Started)
►

Sign in to add a comment