New issue
Advanced search Search tips

Issue 872139 link

Starred by 1 user
Status: Fixed
Owner:
charleszhao@chromium.org
Closed: Nov 6
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

A discarded tab closing or reactivation event is not recorded.

Project Member Reported by charleszhao@chromium.org, Aug 8 
<b>Chrome Version: <From about:version: Google Chrome x.x.x.x></b>
M68+

Please specify Cr-* of the system to which this bug/feature applies (add
the label below).

Steps To Reproduce:
(1) open chrome with several tabs.
(2) open chrome://discards, manually discard a tab
(3) reactivate that tab
(4) open chrome://ukm check ForegroundedOrClosed, 

Expected Result:
No record.
Actual Result:
One record of the reactivation.
How frequently does this problem reproduce? (Always, sometimes, hard to
reproduce?)
Always.
What is the impact to the user, and is there a workaround? If so, what is it?
Only influence the logging.

Comment 1 by charleszhao@chromium.org, Aug 8 

When a backgrounded tab is discarded,

(1) A new WebContents is created,
(2) TabActivityWatcher::TabReplacedAt(tab_strip_model, old_contents,new_contents, int index), the new_web_contents_data does not have a valid source_id.

when the tab is reactivated:
(1) OnVisibilityChanged is called which leads to TabActivityWatcher::WebContentsData::WasShown(),

(2) Then LogBackgroundTabShown is called, but with source_id==0; so the logging doesn't happen.

Comment 2 by charleszhao@chromium.org, Aug 8 

Solution:

Copying old source_id to new web_contents_data in TabActivityWatcher::WebContentsData::DidReplace.

Comment 3 by charleszhao@chromium.org, Aug 8 

We also want to record the discarded state for further analysis.

Comment 4 by charleszhao@chromium.org, Aug 14 

Some investigation:

In prod

When tab_a is discarded, 

(1) TabActivityWatcher::OnDiscardedStateChange(tab_webcontents_a, true)
(2) A new webcontents_new is constructed.
(3) TabActivityWatcher::TabReplacedAt(webcontents_a, webcontents_new) is called.
(4) new webcontets_data_new is constructed from webcontents_new.
(5) webcontets_data_new->DidReplace(webcontents_data_a) is called.

When tab_a is reactivated,
(1) webcontets_data_new->WasShown() is called.
(2) LogBackgroundTabForegroundedOrClosed for the webcontets_data_new is called. (but not logged because ukm_source_id for webcontets_data_new is currently 0)
(3) TabActivityWatcher::OnDiscardedStateChange(webcontents_new, false)
(4) webcontets_data_new->DidFinishNavigation() is called, and a new_ukm_source_id is assigned to webcontets_data_new.

In test
When tab_a is discarded, 

(1) A new webcontents_new is constructed. 
(2) TabActivityWatcher::TabReplacedAt(webcontents_a, webcontents_new) is called.
(3) new webcontets_data_new is constructed from webcontents_new.
(4) webcontets_data_new->DidReplace(webcontents_data_a) is called.
(5) TabActivityWatcher::OnDiscardedStateChange(webcontents_new, true)

When tab_a is reactivated,
(1) webcontets_data_new->WasShown() is called.
(2) LogBackgroundTabForegroundedOrClosed for the webcontets_data_new is called. (but not logged because ukm_source_id for webcontets_data_new is currently 0)
(3) TabActivityWatcher::OnDiscardedStateChange(webcontents_new, false)
(4) webcontets_data_new->DidFinishNavigation() is NEVER called.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 17 

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

commit 241c941fd99ae34671ce1cb800b214889acb2a38
Author: Charles Zhao <charleszhao@chromium.org>
Date: Fri Aug 17 22:15:52 2018

Record discarded state in TabActivityWatcher.

(1) TabLifecycleObserver is added for TabActivityWatcher to track
    OnDiscardedStateChange event.

(2) In DidReplace(replaced_tab), the ukm_source_id and is_discarded
    state is copied to new_tab so that we can track discarded state
    after reload or closing.
(3) A browser test is added.

Change-Id: I9777b515d28d215f3409525b0201dc5916d35941

Bug:  872139 
Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
Reviewed-on: https://chromium-review.googlesource.com/1166763
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Charles . <charleszhao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584217}
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_activity_watcher.h
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_metrics_logger.cc
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_metrics_logger.h
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc
[modify] https://crrev.com/241c941fd99ae34671ce1cb800b214889acb2a38/tools/metrics/ukm/ukm.xml
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 20 

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

commit d38f4e3c1225c36574911f6fdeecdb1b236dd054
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Mon Aug 20 19:46:46 2018

Revert "Record discarded state in TabActivityWatcher."

This reverts commit 241c941fd99ae34671ce1cb800b214889acb2a38.

Reason for revert: Causes crash: crbug.com/875697

Original change's description:
> Record discarded state in TabActivityWatcher.
> 
> (1) TabLifecycleObserver is added for TabActivityWatcher to track
>     OnDiscardedStateChange event.
> 
> (2) In DidReplace(replaced_tab), the ukm_source_id and is_discarded
>     state is copied to new_tab so that we can track discarded state
>     after reload or closing.
> (3) A browser test is added.
> 
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> 
> Bug:  872139 
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> Reviewed-on: https://chromium-review.googlesource.com/1166763
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Charles . <charleszhao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584217}

TBR=chrisha@chromium.org,jam@chromium.org,michaelpg@chromium.org,holte@chromium.org,charleszhao@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  872139 
Change-Id: Ic32c319d1a75ebf3237677ccb8935744f95dfae2
Reviewed-on: https://chromium-review.googlesource.com/1181586
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584540}
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_activity_watcher.h
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_metrics_logger.cc
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_metrics_logger.h
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc
[modify] https://crrev.com/d38f4e3c1225c36574911f6fdeecdb1b236dd054/tools/metrics/ukm/ukm.xml
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 20

Labels: merge-merged-3528
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57743e338cadaab764cf142886dbd47b323288a5

commit 57743e338cadaab764cf142886dbd47b323288a5
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Mon Aug 20 21:53:24 2018

Revert "Record discarded state in TabActivityWatcher."

This reverts commit 241c941fd99ae34671ce1cb800b214889acb2a38.

Reason for revert: Causes crash: crbug.com/875697

Original change's description:
> Record discarded state in TabActivityWatcher.
> 
> (1) TabLifecycleObserver is added for TabActivityWatcher to track
>     OnDiscardedStateChange event.
> 
> (2) In DidReplace(replaced_tab), the ukm_source_id and is_discarded
>     state is copied to new_tab so that we can track discarded state
>     after reload or closing.
> (3) A browser test is added.
> 
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> 
> Bug:  872139 
> Change-Id: I9777b515d28d215f3409525b0201dc5916d35941
> Reviewed-on: https://chromium-review.googlesource.com/1166763
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Charles . <charleszhao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584217}

TBR=chrisha@chromium.org,jam@chromium.org,michaelpg@chromium.org,holte@chromium.org,charleszhao@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  872139 
Change-Id: Ic32c319d1a75ebf3237677ccb8935744f95dfae2
Reviewed-on: https://chromium-review.googlesource.com/1181586
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584540}(cherry picked from commit d38f4e3c1225c36574911f6fdeecdb1b236dd054)
Reviewed-on: https://chromium-review.googlesource.com/1182207
Cr-Commit-Position: refs/branch-heads/3528@{#6}
Cr-Branched-From: 2d67fa3c43ccc16edae0a742d902dd437795959b-refs/heads/master@{#584349}
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_activity_watcher.h
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_metrics_logger.cc
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_metrics_logger.h
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc
[modify] https://crrev.com/57743e338cadaab764cf142886dbd47b323288a5/tools/metrics/ukm/ukm.xml

Comment 8 by charleszhao@chromium.org, Aug 28

Status: Verified (was: Started)

Comment 9 by charleszhao@chromium.org, Sep 10

Status: Started (was: Verified)
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 17 

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

commit d72f99d8e5a73db82814d7f671cd6c258c24ccc6
Author: Charles Zhao <charleszhao@chromium.org>
Date: Mon Sep 17 14:18:27 2018

Record discarded state in TabActivityWatcher.

(1) Add GetWasDiscarded to WebContents

(2) Get discarded state in TabActivityWatcher::WebContentsData::WebContentsData

(3) A browser test is added.

Bug:  872139 
Change-Id: I1a646ed7419fded7846ee80a166beeaee9fbf989
Reviewed-on: https://chromium-review.googlesource.com/1215275
Commit-Queue: Charles . <charleszhao@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591665}
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_metrics_logger.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_metrics_logger.h
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/d72f99d8e5a73db82814d7f671cd6c258c24ccc6/content/public/browser/web_contents.h

Comment 11 by charleszhao@chromium.org, Nov 6

Status: Fixed (was: Started)
►

Sign in to add a comment