New issue
Advanced search Search tips

Issue 858912 link

Starred by 3 users
Status: Fixed
Owner:
tiborg@chromium.org
Closed: Jul 5
Cc:
yfried...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression
Team-Security-UX



Sign in to add a comment

Android PageInfo not properly recording action histograms

Project Member Reported by cthomp@chromium.org, Jun 29 2018 
Chrome Version: 69.0.3475.0
OS: Android 8.1.0; Nexus 6P

What steps will reproduce the problem?
(1) Open chrome://histograms in one tab
(2) Open https://example.com in another tab
(3) Open the Page Info popup (click the (i) in the omnibox or Chrome menu)
(4) Click "Site Settings", then go back to the page
(5) Open chrome://histograms, let it load, and then "Find In Page" for "Security.PageInfo.Action.HttpsUrl.ValidNonEv" to get to the histogram.

What is the expected result?

Bins 0 (PAGE_INFO_OPENED) and 9 (PAGE_INFO_SITE_SETTINGS_OPENED) should have 1 count.

What happens instead?

Only bin 0 (PAGE_INFO_OPENED) has a count of 1.

My guess is that this is happening because the action is being recorded after the controller has already been destroyed:

1. In PageInfoPopup.java, the recordAction call happens in a callback on the button being clicked, inside a |runAfterDismiss|. [1], [2]
1. In recordAction [3], there is a check for the controller still existing, but by this point it has been destroyed (because clicking the button causes the PageInfo popup to be replaced, and the callback waits until dismissal).

[1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java?q=PageInfoController.java&sq=package:chromium&dr&l=245
[2] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java?q=PageInfoController.java&sq=package:chromium&dr&l=556
[3] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java?q=PageInfoController.java&sq=package:chromium&dr&l=594

Looking at blame/history, it looks both the check for the controller still existing and the `runAfterDismiss` setup was added in April 2018 by tiborg@, so adding to cc.

I've uploaded a CL with a quick test to verify a small subset of PageInfo action histogram behavior on Android: https://chromium-review.googlesource.com/c/chromium/src/+/1119633. |testPageInfoActionOpen| should currently pass, and |testPageInfoActionSiteSettings| should currently fail.

Comment 1 by cthomp@chromium.org, Jun 29 2018

Cc: -tiborg@chromium.org
Labels: -Type-Bug Type-Bug-Regression
Owner: yfried...@chromium.org
Status: Assigned (was: Untriaged)
Looking further at the history for PageInfoController/PageInfoPopup shows the git-blame is wrong (tiborg@ just renamed a lot in those changes) and this may go back further. I'm not sure what the best way to bisect Android builds are as I've never done that before.

Assigning to yfriedman@: Can you help triage this? Thanks!

Comment 2 by cthomp@chromium.org, Jun 29 2018

Cc: tsergeant@chromium.org
Dug through blame/history logs a bit more.

+cc tsergeant, who it looks like originally added the recordAction logic (in https://codereview.chromium.org/1714383002/) and added the check for mNativeWebsiteSettingsPopup (in https://codereview.chromium.org/1750463002), in case they have ideas for why this is no longer working.

Comment 3 by cthomp@chromium.org, Jun 29 2018

Cc: -tsergeant@chromium.org yfried...@chromium.org
Labels: FoundIn-68
Owner: tiborg@chromium.org
[-tsergeant who is no longer on Chrome.]

I ran bisect-archived-builds.py to manually bisect the regression. It appears to be introduced in r551437 (crrev.com/c/998409), so assigning this back to tiborg@ who made that CL.

tiborg@ Could you help figure out why the refactor to PageInfoView would have affected recordAction()? Thanks!

Comment 4 by tiborg@chromium.org, Jul 3 

I'll take a look.

Comment 5 by tiborg@chromium.org, Jul 3

Labels: Target-69
Status: Started (was: Assigned)
Yep, you are exactly right. My refactor changed the order of destroying native and running the on dismiss tasks. Will upload a fix for that shortly.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 5 

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

commit 8dfcfd9a3b9a9ca206d051258c5c1690142a4f39
Author: Tibor Goldschwendt <tiborg@chromium.org>
Date: Thu Jul 05 02:29:14 2018

Run page info dialog after dismiss task before destroying native

crrev/c/998409 changed the order to
1. Destroy native.
2. Run after dismiss runnables.

Restore the old older. Otherwise, UMA logging may not be performed
correctly.

Bug:  858912 
Change-Id: I8e983843745cc7aa61a1c8434b756863df62e8fe
Reviewed-on: https://chromium-review.googlesource.com/1124985
Reviewed-by: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572693}
[modify] https://crrev.com/8dfcfd9a3b9a9ca206d051258c5c1690142a4f39/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java

Comment 7 by tiborg@chromium.org, Jul 5

Status: Fixed (was: Started)
►

Sign in to add a comment