CC dmurph@ for triage, as I think he probably knows what components this is  relevant to and who works in this area

+twellington as FYI for android - Does anyone specifically own this area of android settings? It would be great if we had metrics here, but I don't think we do.

+finnur@ and I split preferences ownership, with Finnur taking site settings when newt@ left the team.

Email report from klobag@ as well

Crash from this:
https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20ReportID%3D%27ff44be6e40000000%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=&index=0


I'm going to up this to a P1, it sounds like it's hitting more people. I'll try to sync w/ Finnur about strategy here.

 Issue 733484  has been merged into this issue.

Initially assigning to finnur as they own site settings. I have no idea what's causing this - I added important site integration here, but idk if that's what is causing this.

We can disable fetching important site information for this list - but again I'm unsure what is causing this. This may require metrics to be added?

+wez, do you think that the important sites code could be causing this with all of its maps and stuff?

Re #8: Is there a desktop equivalent to this, or is it an Android-only surface?  (Looking for an easier way to try to repro).

My gut answer would be that yes, this could be due to adding Important Sites, possibly because of all the map-wrangling that code has to do, but also because it touches lots of ContentSettings, so is presumably reading from the user profile, which itself may cause a substantial burst of memory usage.  That's almost pure conjecture, though - hope we can repro!

It's Andri-only.

Possible issue with new local storage, adding Marijn

of course none of the local storage changes landed back in 58, so if this really started that long ago, and hasn't gotten worse in 61 hopefully it's at least not the cause of it

I'm pretty sure this has been happening for a while, at least for as long as I've been occasionally attempting to visit "Site Settings -> All Sites", which is probably at least 56 or earlier.

Hopefully this will help:
https://codereview.chromium.org/2953853002/

If it doesn't then it's something other than important sites.

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

commit 226c79833104ddfbceeb6e866a3f532c7c1fec77
Author: dmurph <dmurph@chromium.org>
Date: Fri Jun 23 01:08:29 2017

[Android Site Settings] Disable fetching important sites when not needed.

Important sites could be a performance hit, especially because it might
hit disk w/ content settings querying, enumerating all bookmarks, etc.
This change lets you disable that.

BUG= 732907 

Review-Url: https://codereview.chromium.org/2953853002
Cr-Commit-Position: refs/heads/master@{#481758}

[modify] https://crrev.com/226c79833104ddfbceeb6e866a3f532c7c1fec77/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java
[modify] https://crrev.com/226c79833104ddfbceeb6e866a3f532c7c1fec77/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleCategoryPreferences.java
[modify] https://crrev.com/226c79833104ddfbceeb6e866a3f532c7c1fec77/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
[modify] https://crrev.com/226c79833104ddfbceeb6e866a3f532c7c1fec77/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
[modify] https://crrev.com/226c79833104ddfbceeb6e866a3f532c7c1fec77/chrome/browser/android/preferences/website_preference_bridge.cc

 Issue 742066  has been merged into this issue.

Is the fix in the latest Dev, which was updated on 6/27? 61.0.3142.0. 

If this was linked to important sites, then yes. If that was not the cause, then this would require more detailed analysis.

I tried to repro myself but could not. With important sites out of the picture then the other ways to make this better would be to batch the usage fetching maybe?

finnur@ have you looked at this at all?

 Issue 757983  has been merged into this issue.

+permissions component. Permissions people - this is probably something to do with loading all the content settings and then reducing them down, or doing something naive like that.

Tim / Patti - this might be a good one for one of you to look at if you have time.

dmurph@: no I've had time. I've been on vacation and am working with the London team at the moment on a Windows 10 project.

I don't just get this for 'all sites' - I get this hang when I just open site settings for *any* site - see  issue 757983  - I think this might raise the priority of the issue?

For the record, as the original reporter of the all-sites-freezes-chrome issue (which still happens to me), I don't see a meaningful lag / hang when opening the (i) page info menu.  I'm running Chrome 60 (current stable release).

in which case I'm not sure these bugs ( issue 757983  and this) should have been merged since I consistently get a full app hang when following steps in  issue 757983 . See screenshot. I am running Chrome Canary 62.0.3194.0

Deleted: app_hang_site_settings.png 37.4 KB

	app_hang_site_settings.png 37.4 KB View Download

Oh no, you're right.  I only clicked on the (i), not the "site setting" in the popup that displays.  When I click that, I see the same hang/freeze as before.

Sorry for the distraction.

I would be pretty suspicious of WebsitePermissionsFetcher() -
 https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java?l=61

I'm pretty sure this is called when you tap the "Site Settings" link. It fetches every permission for every origin. It has a TODO linking to the archived  Issue 459222 , which suggests fetching only permissions for the exact URL that's asked for.

Assigning to Patti to have a look at for M63.

To more concisely report here what's in  issue 763446 :

On Android, when opening the site settings screen, I get the title of the window but it's kept blank for a seconds while it's "loading" what should be displayed there. I can't tell if that characterizes "freezing" or not but as there's no feedback to the user that something is going on in the background my first impulse is to conclude there's really nothing there to see.

Taking this one off Patti and assigning to me. I'm exploring what can be done for M64 as a stopgap before a revamp of all sites.

I inserted 100 urls with settings for geolocation, notification, MIDI sysex, protected media, mic, and camera permissions. When triggering site settings for this, the call to WebsitePermissionsFetcher#fetchAllPreferences() takes 24 seconds in total in a release build, which is really bad. So there are two problems:

a) trying to show site settings for 1 URL fetches every single content setting that exists, and then filters on the Java side
b) fetching 100 urls and sending them over the JNI takes 24 seconds....

I'm going to try and solve a) as a stopgap for M64. But I'll need to also solve b) to make All Sites viable too.

Correction: I was using 500 URLs to get that 24 second runtime. But chopping the number of URLs down to 50 still takes just as long.

Narrowing: the slowness is all in the Java-side processing of the data sent from native. Specifically, it seems to be in WebsitePermissionsFetcher#findOrCreateSite() when we're looking up the origin/embedder pair in the hashmap of sites. These are used as the key into the hashmap in an android.util.Pair object.

Thanks to some help from timloh@ and sammc@, we discovered that android.util.Pair object's hashCode method is first XOR second. A number of permissions use the *same* origin and embedder in the pair... which makes XOR return 0 and voila, multiple hash collisions. That's where the slowness happens - having a hashmap where loads of the keys hash identically. :|

I think the easiest fix is to subclass android.util.Pair and replace its hashCode() method with something better.

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

commit 2cd55a1304342c5d9a1de8961b24fa8b1817c0bd
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Oct 25 11:19:23 2017

Override android.util.Pair's hashCode for the WebsitePermissionsFetcher.

crbug.com/636330 fixed a NullPointerException in
WebsitePermissionsFetcher by using a pair of (origin, origin) in place
of (origin, null). Unfortunately, android.util.Pair#hashCode() simply
XORs the two items in the Pair together. This means that the
(origin, origin) always hashes to 0, resulting in pathological linear
lookup performance. For ~50 URLs with permissions data, it takes ~25
seconds for a Nexus 5 to run
WebsitePermissionsFetcher#fetchAllPreferences, which is run to show Site
Settings and All Sites.

This CL addresses the performance issue by subclassing android.util.Pair
and overriding hashCode() with the implementation of Arrays.hashCode. It
also includes null checks, meaning that we can remove the fix for
crbug.com/636330 without reintroducing the NullPointerException crashes
in Jelly Bean. This has been manually tested and the ~25 second runtime
is eliminated completely.

BUG=636330, 732907 

Change-Id: I5876f7af641244a1d9f4d7c2e1224c1393c98bee
Reviewed-on: https://chromium-review.googlesource.com/737610
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511417}
[modify] https://crrev.com/2cd55a1304342c5d9a1de8961b24fa8b1817c0bd/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java

A subtle bug.  Good catch!

How hard would it be to add a test (telemetry?) to detect/prevent regressions like this in the future?

Great catch. Thanks for looking at this Dom!

WebsitePermissionsFetcher is actually entirely untested at the moment. I'm looking into how complex adding tests would be.

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

commit d3ad98419256e69d625b9ad221fb87cbbe763eb1
Author: Dominick Ng <dominickn@chromium.org>
Date: Thu Oct 26 23:47:50 2017

Add a regression performance test for WebsitePermissionsFetcher.

This CL adds a smoke test for major performance regressions in
WebsitePermissionsFetcher#fetchAllPreferences to guard against issues
like  crbug.com/732907  in the future. This test fails prior to
crrev.com/c/737610.

BUG= 732907 

Change-Id: Ief553b96c7e7684affded5eb04d925ebdb49b6c6
Reviewed-on: https://chromium-review.googlesource.com/737496
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512024}
[modify] https://crrev.com/d3ad98419256e69d625b9ad221fb87cbbe763eb1/chrome/android/java_sources.gni
[add] https://crrev.com/d3ad98419256e69d625b9ad221fb87cbbe763eb1/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcherTest.java

Now fixed and we should see it roll out in M64.

Issue 709852 has been merged into this issue.