New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 645123 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

TabRestoreService should not allocate much

Project Member Reported by dskiba@chromium.org, Sep 8 2016

Issue description

In a test where I open one site from NTP, close its tab, and open another one, TabRestoreServiceHelper::PopulateTab allocates 950 KiB in 3 std::string allocations:

1,336.4 KiB	[Thread: oid.apps.chrome]
1,312.6 KiB	</data/app/com.google.android.apps.chrome-3/oat/arm/base.odex>
951.9 KiB	TabAndroid::CreateHistoricalTabFromContents(content::WebContents*)
951.8 KiB	sessions::TabRestoreServiceHelper::CreateHistoricalTab(sessions::LiveTab*, int)
950.7 KiB	sessions::TabRestoreServiceHelper::PopulateTab(sessions::TabRestoreService::Tab*, int, sessions::LiveTabContext*, sessions::LiveTab*)
949.3 KiB	std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::operator=(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)

We need to understand why TabRestoreServiceHelper saves anything at all in this situation, what those strings are exactly, and whether we can optimize them (either globally or svelte-only).


 
Owner: dskiba@chromium.org

Comment 2 by dskiba@chromium.org, Sep 12 2016

OK, so PopulateTab() saves many strings, but the big one is sessions::SerializedNavigationEntry::encoded_page_state_. In my debugging session it was 140 KiB, but I also saw 400 KiB in one of the traces (not to mention 900 KiB one from above).

In the end we can have as many as 25 (TabRestoreServiceHelper::kMaxEntries) Tab entries, so I'm thinking of adding UMA to monitor total size of all the entries.

Comment 3 by dskiba@chromium.org, Sep 14 2016

More information about encoded_page_state_. That's the string created by EncodePageState() from ExplodedPageState structure. I dumped content of ExplodedPageState into JSON file, see attachment. The original SerializedNavigationEntry::encoded_page_state_ was 840 KiB, and in the JSON we can see the following long strings:

/top/children[14]/children[1]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[2]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[3]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[4]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[5]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[6]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[7]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/children[8]/target: 44758 chars, "<!--framePath /z/1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" ma..."
/top/children[14]/target: 44724 chars, "1-0-4;44035;<!doctype html><html><head></head><body leftMargin="0" topMargin="0" marginwidth="0" mar..."

Those "target" strings are values of ExplodedFrameState::target, and of type NullableString16, i.e. each char is 2 bytes. So we have 9 strings ~44K chars each, 2 bytes per char = ~800 KiB. I.e. those strings are responsible for most of encoded_page_state_ (during encoding strings are written as is).

ExplodedFrameState::target is assigned from from WebHistoryItem::target, which is just HistoryItem::target, which is assigned from "m_frame->tree().uniqueName()" in [1]. This leads us to FrameTree::calculateUniqueNameForChildFrame(), which has a nice comment about how unique names are generated, see [2]. 

So this is what seems to be happening:

1. It all starts with "top/children[14]/target", which looks like manually assigned unique name ('assignedName' in terminology from [2]). It's 44K chars long, and almost entirely consists of javascript, see attachment.

2. Unique names for all 8 children are generated using 'oldGeneratedName ::= "<!--framePath //" ancestorChain "/<!--frame" childCount "-->-->"' rule from [2], where 'ancestorChain' is "concatenated unique names of ancestor chain". So they all inherit that long unique name from #1.

Ironically, the change that introduced all this was the fix for issue 558680, which was reported by me.

The thing that sets root giant unique name seems to be our DoubleClick - url_strings for those frames are either "https://googleads.g.doubleclick.net/..." or "http://tpc.googlesyndication.com/...".



Questions to knowledgeable parties:

1. Do we really need to save ExplodedFrameState::target as part of encoded page state?
2. I can open closed tabs on another device - does it mean we send encoded page state to the server, and then back to my other devices?




[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?l=384
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FrameTree.cpp?l=307
page-state.json
467 KB View Download
top_children14_target.txt
43.7 KB View Download

Comment 4 by dskiba@chromium.org, Sep 14 2016

Cc: lukasza@chromium.org
Cc: dcheng@chromium.org creis@chromium.org
This seems to be caused by the excessively long frame unique names - this is being discussed in more detail in  issue 626202 .

Comment 6 by dskiba@chromium.org, Sep 14 2016

Answer to (2) from #3:

Recently closed tabs don't seem to be synced across devices (I don't see them on my other devices), so that huge encoded page state stays local to the device.

Comment 7 by dskiba@chromium.org, Sep 19 2016

Once  issue 626202  is fixed I'll check and close this issue.

Comment 8 by dskiba@chromium.org, Oct 24 2016

Labels: Performance-Memory-Q4

Comment 9 by dskiba@chromium.org, Jan 23 2017

Labels: -Performance-Memory-Q4 Performance-Memory-Q1
Status: Assigned (was: Untriaged)
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 9 2017

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

commit c72a640b0c237266f370c88c5f16e1fe30cbabb0
Author: dskiba <dskiba@chromium.org>
Date: Thu Feb 09 23:21:04 2017

[memory-infra] Whitelist TabRestoreService MDP.

This CL whitelists TabRestoreService MDP to make it available in
slow reports.

BUG= 645123 

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

[modify] https://crrev.com/c72a640b0c237266f370c88c5f16e1fe30cbabb0/base/trace_event/memory_infra_background_whitelist.cc
[modify] https://crrev.com/c72a640b0c237266f370c88c5f16e1fe30cbabb0/components/sessions/core/tab_restore_service_helper.cc

Comment 11 by ssid@chromium.org, Feb 16 2017

Labels: -Pri-3 Pri-2
Summary: TabRestoreService should listen to memory pressure signals (was: TabRestoreServiceHelper allocates 950 KiB)
Tab restore service keeps state of last 25 tabs in memory on Android. This could use upto 20 MiB of memory over 15 hours of session. There seems to be lot of cases with Pixel and Nexus devices that such duration is possible.

Trace with 18MiB of tab restore state:
https://crash.corp.google.com/browse?stbtiq=853fa03580000000#6?OpenedMetric=memory:chrome:browser_process:reported_by_chrome:tab_restore:effective_size_max

1% of users (from a small sample space) have more than 4MiB.

We should also think about reducing the number of tabs kept for Android. Or drop them if they are very old.

Summary: TabRestoreService should not allocate much (was: TabRestoreService should listen to memory pressure signals)

Comment 13 Deleted

Comment 14 Deleted

Labels: -Hotlist-EM
Labels: EM
Labels: -EM LowMemory

Comment 18 by ssid@chromium.org, Mar 1 2017

Cc: w...@chromium.org ajwong@chromium.org
Labels: -OS-Android OS-All
Trace with 230MB of Tab restore in Windows
https://crash.corp.google.com/browse?stbtiq=ee57bcd300000000#6?OpenedMetric=memory:chrome:browser_process:reported_by_chrome:tab_restore:effective_size_max
Cc: sky@chromium.org
What if we dump TabRestoreService entries do individual CLOSE_ON_EXEC files? I.e. instead of keeping entries in memory we will keep them in files, dumping asynchronously  after some age.

For example in #18 there are two huge entries, one is ~120 MiB closed 87 mins ago, and another is ~100 MiB closed 5 mins ago.

Comment 20 by sky@chromium.org, Mar 2 2017

It is certainly possible to do that. In fact we could probably dump everything to disk after n minutes and read back when necessary. The trick will be making the code that expects restore to be synchronous no asynchronous.
Project Member

Comment 21 by bugdroid1@chromium.org, Mar 22 2017

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

Project Member

Comment 22 by bugdroid1@chromium.org, May 25 2017

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

Project Member

Comment 23 by bugdroid1@chromium.org, Aug 9 2017

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

Comment 24 by ssid@chromium.org, Aug 17 2017

This is great. We see a 2MB reduction in browse_news_toi and 1.8Mb reduction in browse_news_cricbuzz stories.

https://chromeperf.appspot.com/report?sid=630926194f0e4c379b51c1f49e7cea71acbfc31bb6e35911eaf9ea717b93f163&start_rev=481403
Status: Fixed (was: Assigned)
I'm going to mark this as fixed (unless there's other low hanging fruit to be had)
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 22 2017

Labels: merge-merged-3163
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

Sign in to add a comment