Thanks for the report.  This could be lots of things, but my hunch is that this might be http://crbug.com/495806 (Safebrowsing analysis on URLs is ridiculously expensive on Android).  williamluh@, is there a trace event category for Safebrowsing that we could use to prove/disprove this theory?

There's no profiling that you can turn on to get performance numbers from the API directly. I believe that you can go to chrome://histograms/SB2 to get some metrics. Adding nparker@ to give more details.

There are safe_browsing events in the net-log traces.  That would tell you if it's waiting on GMSCore for URL-classification. Though those wait periods (sometimes 10's of ms) should not be hanging on to the UI thread.  It uses the UI thread mostly just for for IPC marshaling.

FYI: I turned off Safe Browsing, and went to the androidauthority.com site and also got scroll jank (not initially) as I scrolled down.

There's a LOT of reasons for scrolljank on androidauthority - the real test is to take a trace (with all categories enabled) and look for the long ViewHostMsg_UpdateState tasks.

To answer my question, the trace category for safebrowsing is "loader".  I can observe safebrowsing as expected causes a lot of jank, but on the browser IO thread within ResourceHostMsg_RequestResource, so this is something different.

I spent some time investigating this today.  The cause is that, on this androidauthority.com site, the PageState object is bloated and it just keeps growing in size the more you interact with the site.  At initial page load, the state.ToEncodedData().length() is about 500k and each ViewHostMsg_UpdateState takes 10ms, and after scrolling on the page for 2 minutes or so, it's 7.5MB and each ViewHostMsg_UpdateState takes 60ms.  The time is consumed by a few simple operations on the data such as serialization into the entry, and invoking state.GetReferencedFiles().

Do you know a navigation expert we could assign to for further investigation?

+a few extra folks

Casting an even wider net ;)

+creis for history and navigation

(I only vaguely remember that History.pushState() was noticed to be problematic for smoothness before)

Wow, that's awful.  PageState is just a serialized tree of WebHistoryItems, so the page must be storing an unbounded amount in there somehow.  Does anyone know which part of it is growing?  (Should be easier to watch in the renderer process before it's serialized in EncodePageState.)

A similar issue came up in  issue 523372 , where some sites were using enormous blocks of JavaScript as the frame's name, which get included in the PageState.  I think Daniel added some histograms on it.

Hard to tell what to do about it until we know which part is growing without bound.

Among other things, it looks like the size of the unique names (and frame names) is a problem. In a fresh session of chrome, here's what chrome://histograms looks like after loading the URL in question and scrolling up and down on my workstation:

Histogram: SessionRestore.FrameUniqueNameLength recorded 149 samples, average = 4381.1 (flags = 0x1)
0      -----------------------------------------------O                          (5 = 3.4%)
1      ... 
7      --------------O                                                           (3 = 2.0%) {3.4%}
9      -------------O                                                            (4 = 2.7%) {5.4%}
12     O                                                                         (0 = 0.0%) {8.1%}
16     ----O                                                                     (2 = 1.3%) {8.1%}
21     -----------O                                                              (6 = 4.0%) {9.4%}
28     ---------------O                                                          (8 = 5.4%) {13.4%}
37     --O                                                                       (1 = 0.7%) {18.8%}
49     ------------------------------------O                                     (19 = 12.8%) {19.5%}
65     ------------------------------------O                                     (19 = 12.8%) {32.2%}
86     ------------------------------------------------------------------------O (38 = 25.5%) {45.0%}
113    ------------------------------------------------------------------O       (35 = 23.5%) {70.5%}
149    --O                                                                       (1 = 0.7%) {94.0%}
196    ... 
63662  ---------------O                                                          (8 = 5.4%) {94.6%}
83848  ... 

I added some logging and the excessively long names start with this:

name (78284): 1-0-4;77573;<!doctype html><html><head><style><!--
unique name (78284): 1-0-4;77573;<!doctype html><html><head><style><!--

document.URL for these bad frames:
"http://tpc.googlesyndication.com/safeframe/1-0-4/html/container.html#xpc=sf-gdn-exp-1&p=http%3A//www.androidauthority.com"
"http://tpc.googlesyndication.com/safeframe/1-0-4/html/container.html#xpc=sf-gdn-exp-2&p=http%3A//www.androidauthority.com"

I haven't figured out which frame is responsible for setting this yet, but there are some hits on internal codesearch (sorry, Googlers only): https://goto.google.com/rttgs

Thanks for sharing this Daniel.
I'll start a thread for this aspect.

FWIW, after scrolling up and down, you can see the iframes with wonky names by opening inspector with Ctrl+Shift+J and doing this:

document.querySelectorAll('iframe')

So the iframes with bad names are getting embedded into the main frame, but I'm not sure how these frames get inserted (I'm guessing they're inserted with document.write).

A little snippet to get the exact JS stack:

window.callbackFunc = function(elem, args) {
  if (elem.tagName == "IFRAME") {
    if (elem.name.length > 500) {
      debugger;
    }
  }
}
window.f = Element.prototype.appendChild;
Element.prototype.appendChild = function() {
    window.callbackFunc.apply(this, arguments);
    return window.f.apply(this, arguments);
};

It's minified, but hopefully there's enough from there to debug.

One strawman for mitigating the impact of this on the Chromium side:

If a unique name is greater than a threshhold (say, 512 chars), just hash the generated unique name and use the hash as the unique name.

Advantages:
- Backwards compatible. Newer versions of Chrome can easily update the serialized PageState as needed so history entries still match up.

Disadvantages:
- Non-OOPIF still sends window.name from the renderer to the browser. Sending ~100KB in an IPC is probably a good way to jank IPC.
- OOPIF also replicates window.name to all the renderers for a page, which means more potential jank.

Given the constraints of the SafeFrame spec [1], we're a bit constrained in what we can do: in SafeFrame, it's used as a way to push data cross-origin, so we can't just truncate window.name if it's too long. We could consider a hack like only propagating the full window.name to the renderer hosting that browsing context, but it'd be quite a bit of complexity.

Finally, note that there's a reasonable argument that using this mechanism is an info leak in site-per-process mode: only main-page.com and ad.com should be privy to the ad content, but using this mechanism means the string gets replicated into all renderers. We'd have to better understand the contraints around SafeFrame to understand why they didn't choose an alternate route like embedding the ad parameters into the iframe src URL.

[1] http://www.iab.com/wp-content/uploads/2014/08/SafeFrames_v1.1_final.pdf

I'm working on some patches to mitigate the impact of overly long window.names in Blink based on the idea in #20.

Thanks for the analysis dcheng@, I'm glad the issue is well-understood now.

 Issue 523372  has been merged into this issue.

Good job following through on this. It is amazing what some web pages do, and it will be great to get this resolved.

 Issue 633019  has been merged into this issue.

Daniel, can you post an update for everyone here? Thanks!

BTW, other side of long unique names is increased memory usage in the browser process, see  issue 645123  where similar strings cause ~900 KiB allocations (and also potentially are distributed across all user devices).

BTW, one temporary workaround is to serialize the string as std::string (UTF-8 / WTF-8 encoded) instead of NullableString16. That should cut payload size in half in most cases.

I'm working on moving the unique name code into //content. The only question here is how aggressively we'll try to write code to preserve backwards compatibility. We suspect that we may be able to omit the complexity of trying to maintain backwards compatibility WRT history navigations, and creis@ is adding a UMA to confirm.

Once we have some data from the UMA, we'll land the updated unique name generation strategy which can avoid this degenerate case.

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

commit e20a1c11a51eb321964caa3db2196805fe9744f7
Author: creis <creis@chromium.org>
Date: Thu Oct 06 02:19:34 2016

Record UMA stats for subframes history navigations.

This CL tracks how often subframe history navigations go to a different
URL than the default frame src URL, and how long the unique names are
in these cases when they include frame paths.  The latter is useful for
knowing whether a plan to truncate unique names would affect session
history behavior.

This should be reverted after sufficient data is collected.

BUG= 626202 
TEST=UMA stats reported when going back after navigating an unnamed
     child frame, particularly unnamed grandchildren of subframes
     with long names.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator.h
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/tools/metrics/histograms/histograms.xml

The data appears to be good - so there is a chance that this will be fixed for M-55? (Curious because this bloats TabRestoreService, see  issue 645123 ).

Comment 32: It's unlikely that any fix for this will be mergeable, so M56 seems like the best case scenario.  (That may also be tight, given the other issues we're facing).

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

commit e20a1c11a51eb321964caa3db2196805fe9744f7
Author: creis <creis@chromium.org>
Date: Thu Oct 06 02:19:34 2016

Record UMA stats for subframes history navigations.

This CL tracks how often subframe history navigations go to a different
URL than the default frame src URL, and how long the unique names are
in these cases when they include frame paths.  The latter is useful for
knowing whether a plan to truncate unique names would affect session
history behavior.

This should be reverted after sufficient data is collected.

BUG= 626202 
TEST=UMA stats reported when going back after navigating an unnamed
     child frame, particularly unnamed grandchildren of subframes
     with long names.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator.h
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/e20a1c11a51eb321964caa3db2196805fe9744f7/tools/metrics/histograms/histograms.xml

[Automated comment] removing mislabelled merge-merged-2840

dcheng@: Let's review the UMA data for this and proceed with your design if we can.  Sounds like this is leading to issue 680732 as well.

Yeah, we urgently need something done about this for WebView..

Is there any update?

In one of my profiling runs (where I just browse bunch of sites with heap profiler on) I see  that content::NavigationEntryImpl::AddOrUpdateFrameEntry allocated 1,272.0 KiB:

1. FrameNavigationEntry::FrameNavigationEntry allocated 355.3 KiB copying 37 frame_unique_names
2. FrameNavigationEntry::SetPageState allocated 535.2 KiB copying 27 PageStates
3. FrameNavigationEntry::UpdateEntry allocated 380.0 KiB copying 8 frame_unit_names / PageStates

Then there was FrameReplicationState::FrameReplicationState() which allocated 374.7 KiB copying 26 unique_names / names.

Is the UMA data referred to in comment 36 collected now? It'd be really helpful to WebView if something can get done about this in time for M58, since this has become a much more serious issue on Android 7.0 (which now crashes apps if their saved state is too large instead of just discarding the state) - see issue 680732 for more details of how this impacts WebView.

dcheng@ mentioned that he's looking into this today.

I'm currently working on moving some of this code out of Blink into Chromium to make followups easier.

Even though we have some UMAs, it's difficult to ascertain how much of an impact breaking session restore will be: SessionRestore.FrameUniqueNameLength is the main UMA we were looking at.

It is possible to try to preserve backwards compatibility, but it's non-trivial and somewhat brittle (it requires trying to parse some frame structure out of the unique name). But if this is something we definitely need for M58, then it might be the safest approach.

One other thing we could consider is making the behavior change controlled by an experiment (which would require keeping around both code paths), or running a short-term experiment to gauge the impact of breaking session restore on unique names that would be affected.

Is there a patch implementing proposed changes? I'm interested in memory impact (for FrameNavigationEntry / TabRestoreService). I don't need backwards compatibility, just having shorter names would be enough.

No, but if you just want to test that theory in a fresh profile, it should be sufficient to take the last 512 characters of things returned by blink::FrameTree::uniqueName(). 

Note that this won't affect any data persisted in storage though. For that, you could try grabbing the last 512 characters of content::ExplodedFrameState::target.

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

commit 999698bd4862a5ef634b7f8aa45039038b76e87f
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Mar 22 04:56:37 2017

Move unique name generation and tracking into //content.

Blink doesn't really consume this anymore, so move the
concept out of Blink and simplify the plumbing. Future
CLs will move it wholly into the browser process.

BUG= 626202 , 645123 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=creis@chromium.org, rdevlin.cronin@chromium.org, thestig@chromium.org

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

[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/browser/site_per_process_browsertest.cc
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/public/test/test_runner_support.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/BUILD.gn
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/history_entry.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/render_frame_impl.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/render_frame_proxy.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/render_view_browsertest.cc
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/unique_name_helper.cc
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/renderer/unique_name_helper.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/shell/test_runner/BUILD.gn
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/shell/test_runner/DEPS
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/shell/test_runner/layout_dump.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/shell/test_runner/web_frame_test_client.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/test/BUILD.gn
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/content/test/test_runner_support.cc
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/extensions/renderer/script_context_browsertest.cc
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
[add] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name.html
[rename] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/LayoutTests/fast/history/resources/frameset-dest.html
[delete] https://crrev.com/8773b38c48cf8b27ed4dd1c8ca249944929d3d16/third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/loader/HistoryItem.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/loader/HistoryItem.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/core/page/FrameTree.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/LocalFrameClientImpl.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebFrame.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebFrameImplBase.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebHistoryItem.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebRemoteFrameImpl.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/tests/FrameTestHelpers.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/Source/web/tests/WebViewTest.cpp
[delete] https://crrev.com/8773b38c48cf8b27ed4dd1c8ca249944929d3d16/third_party/WebKit/Source/web/tests/data/frameset-repeated-name.html
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/public/web/WebFrame.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/public/web/WebHistoryItem.h
[modify] https://crrev.com/999698bd4862a5ef634b7f8aa45039038b76e87f/third_party/WebKit/public/web/WebRemoteFrame.h

BTW, do we serialize PageStates on Android? In fact, do we serialize anything that contains unique names on Android?

Maybe we can enable the new way of generating names on Android first? It would greatly help low-end devices.

I have a CL which I'll send out for review when I'm back.

The Android WebView serializes PageStates: see issue 680732 for the issue where we run into the limit for a binder transaction and crash when it's too large, which is more than just a memory/performance issue :)

dcheng@, friendly ping :) Is there something we can help with?

torne@ it's true that we serialize PageStates, but we don't write them to disk, right? I.e. there is no risk of losing data if we change PageState format.

I'm working on tests and will have a PS uploaded on Wednesday.

Technically there is: we serialise them into ActivityManager's state. If there's a WebView update then it's possible for the app using WebView to get an old state back from ActivityManager.

I don't think this is a huge deal, though, as a one-time thing.

dcheng@, friendly ping :) Is there anything I can help with to speed this up?

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

commit 61b2c92224f1bb6db51b0d7b9a84a08dd451f658
Author: dcheng <dcheng@chromium.org>
Date: Thu May 25 23:10:11 2017

Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't easily
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so it can't
simply manufacture on-the-fly by-value wrappers for the return
values of these methods--the storage would need to be owned by each
RenderFrameImpl *and* RenderFrameProxy.

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG= 626202 , 645123 

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

[modify] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/common/BUILD.gn
[add] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/common/unique_name_helper.cc
[add] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/common/unique_name_helper.h
[modify] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/renderer/BUILD.gn
[modify] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/61b2c92224f1bb6db51b0d7b9a84a08dd451f658/content/renderer/render_frame_impl.h
[delete] https://crrev.com/9e244b6ad9c1c3f073a93274f6f6f83adef322cb/content/renderer/unique_name_helper.cc
[delete] https://crrev.com/9e244b6ad9c1c3f073a93274f6f6f83adef322cb/content/renderer/unique_name_helper.h

Woohoo! Does it actually change the name generating algorithm to generate smaller names, or is it a prerequisite for that?

No, that's just a prereq CL.  But dcheng@ is actively working on it.

FYI, in one of our EM user stories frame_navigation_entry.cc allocates 2.5 MiB (go/onqby).

> FYI, in one of our EM user stories frame_navigation_entry.cc allocates 2.5 MiB (go/onqby).

not sure we can do anything about it, maybe open a separate issue? +clamy@, +arthursonzogni@ FYI

I'm pretty sure fixing this bug will help with that. Right now PageState can swell pretty big because unique iframe ids include their names, and some ad networks put 64KiB of stuff into iframe names.

BTW I filed issue 733704 to discuss possibility of writing MemoryDumpProvider for frame_navigation_entry and friends. Please comment there if you have thoughts on the matter.

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

commit 6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Aug 09 21:45:41 2017

Limit the maximum length of frame unique names.

The unique name is a semi-stable identifier used to identify the target
frame for back/forward and session restore. The original unique name
generation algorithm included the browsing context name: unfortunately,
certain sites use window.name to transport large amounts of data,
causing session restore data to balloon in size.

The original plan was to strictly limit the length of unique names;
however, ensuring backwards compatibility was complex and difficult to
understand. Instead, this patch enforces a weaker guarantee: if a frame
provides a hint for the unique name that is over 80 characters, hash
the requested name and use the result as if it were the requested name
instead. It's still possible to get fairly long names with deeply
nested frames, but this should be a large improvement over the current
situation with no limit at all.

Note that even the simpler version of this algorithm does not result in
perfect backwards compatibility: a malicious page can intentionally
pick browsing context names that only collide once the name is hashed.
Since this only affects the page itself, the algorithm retains the
current best effort collision avoidance strategy of picking a name that
is unlikely to collide, without guaranteeing full collision resistance.

Browsing a small assortment of control pages shows that unique name
length is reduced from an average of ~1260 characters to 70 characters.
Note that this metric was originally implemented incorrectly: for the
purpose of comparison, the new metric was recorded in the exact same
way. Actual numbers in the field are probably somewhat lower than this.

Bug:  626202 ,  645123 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I63c481feaf708c5e0d4087dafc8fcbf59b9091a6
Reviewed-on: https://chromium-review.googlesource.com/579031
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493153}
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/page_state_serialization.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/page_state_serialization.h
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/page_state_serialization_unittest.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/unique_name_helper.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/unique_name_helper.h
[add] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/common/unique_name_helper_unittest.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/renderer/render_frame_impl.h
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/test/BUILD.gn
[add] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/content/test/data/page_state/serialized_v24.dat
[modify] https://crrev.com/6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28/tools/metrics/histograms/histograms.xml

Talked with dskiba@ and ssid@. There's not a lot of data in the slow memory reports from the Android dev with this change, but the perf benchmarks show a non-trivial improvement with this change: https://chromeperf.appspot.com/report?sid=cc0df528c997f7337f695aa14b85cfbba34669330c8e0ad7056ee3e69d61729e&start_rev=492000&end_rev=494000

This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Had you merge requested this a week or two ago instead of today, it may have been accepted, but as is we only have 2 betas left for M61 and that's a huge merge to pick up right now.  While impressive, I don't think the perf win is sufficient enough to warrant the additional risk of a regression.  Ping me ASAP if you disagree and we can discuss in more detail.

There is data from Dev version in go/slow-memory-reports:
~60% of the users have memory usage in Tab restore service.

The histograms of memory usage of Tab restore service in version 62.0.3185.0 vs 61.0.3163.13 is attached in as graphs.

The % of users with more than 4MB for tab restore came down from 1.5% to 0.
The highest usage of tab restore was more than 200MB and now the highest seen is 5MB.

The 95th percentile usage of Tab restore came down from 1.5MB to 0.5MB. The median usage of tab restore is less than 100KB and did not change much. So, it only affects the top 10 percent users.

Deleted: tab_restore_histogram.png 75.8 KB

	tab_restore_histogram.png 75.8 KB View Download

I spoke with dcheng@ and a number of other folks on this for a while.  The performance gains here are very large where present (up to 400MB / page on some sites!), and the code has not diverged between M61 and M62.  dcheng@ has reviewed all bugs filed against beta with applicable tags and has found no fallout related to this bug.  Additionally, since this code is used in so many places, and failures likely would have been caught by automated testing.

Thus, will approve for M61.  We should strive to fix issues like this sooner where possible in the future, however.

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

commit 7bc05d67c48febe21ea8d5621d2c27dcba5acc90
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Aug 22 23:23:25 2017

Limit the maximum length of frame unique names.

The unique name is a semi-stable identifier used to identify the target
frame for back/forward and session restore. The original unique name
generation algorithm included the browsing context name: unfortunately,
certain sites use window.name to transport large amounts of data,
causing session restore data to balloon in size.

The original plan was to strictly limit the length of unique names;
however, ensuring backwards compatibility was complex and difficult to
understand. Instead, this patch enforces a weaker guarantee: if a frame
provides a hint for the unique name that is over 80 characters, hash
the requested name and use the result as if it were the requested name
instead. It's still possible to get fairly long names with deeply
nested frames, but this should be a large improvement over the current
situation with no limit at all.

Note that even the simpler version of this algorithm does not result in
perfect backwards compatibility: a malicious page can intentionally
pick browsing context names that only collide once the name is hashed.
Since this only affects the page itself, the algorithm retains the
current best effort collision avoidance strategy of picking a name that
is unlikely to collide, without guaranteeing full collision resistance.

Browsing a small assortment of control pages shows that unique name
length is reduced from an average of ~1260 characters to 70 characters.
Note that this metric was originally implemented incorrectly: for the
purpose of comparison, the new metric was recorded in the exact same
way. Actual numbers in the field are probably somewhat lower than this.

TBR=dcheng@chromium.org

(cherry picked from commit 6ca7f1c95ea0346e53d00c24c940f6d2a52b3d28)

Bug:  626202 ,  645123 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I63c481feaf708c5e0d4087dafc8fcbf59b9091a6
Reviewed-on: https://chromium-review.googlesource.com/579031
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#493153}
Reviewed-on: https://chromium-review.googlesource.com/624817
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#788}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/page_state_serialization.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/page_state_serialization.h
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/page_state_serialization_unittest.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/unique_name_helper.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/unique_name_helper.h
[add] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/common/unique_name_helper_unittest.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/renderer/render_frame_impl.h
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/test/BUILD.gn
[add] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/content/test/data/page_state/serialized_v24.dat
[modify] https://crrev.com/7bc05d67c48febe21ea8d5621d2c27dcba5acc90/tools/metrics/histograms/histograms.xml

I've filed issue 758320 to follow up on the WebView issue that was related to this bug (WebView.saveState creating too-large bundles) so that we can verify that this brings the webview state down to a reasonable size which is less likely to crash apps.