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

Issue metadata

Status: Fixed
Owner:
Closed: Aug 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug-Regression



Sign in to add a comment
link

Issue 786211: history.replaceState throttling is too blunt

Reported by joh...@chromium.org, Nov 17 2017 Project Member

Issue description

Chrome Version: 61+
OS: all

Several legitimate use cases for history.replaceState have been broken since https://codereview.chromium.org/2972073002/ landed in  issue 394296  (m61), since the throttling introduced there is rather blunt:

1. It's limited to 5 calls per second, which is likely to be exceeded by use cases such as restoring scroll/pan position (for websites that do their own scrolling/panning, e.g. Google Maps). This is particularly a problem with mousewheel scrolls, where there isn't a clear end to the scroll gesture, so websites need to call history.replaceState for every wheel increment unless they do complex batching with timeouts.

2. When the limit is exceeded, rather than postponing the call until there is quota again (overwriting intermediate states if several calls to replaceState are queued, as only the last one matters), calls are simply dropped on the floor, so often the last few states passed to replaceState will never be persisted. This is particularly unfortunate if a website calls replaceState for a mixture of less important reasons (e.g. scrolling) and infrequent but important reasons (e.g. WYSIWYG form data).

3. When the limit is exceeded, rather than regaining quota after 200ms (since the limit is currently 5 calls per second), the patch discards state changes for entire 10 second periods, risking meaningful data loss when the page is later restored.

4. Even websites that make far fewer than 5 calls per second can still get throttled, since the patch just counts up to 50 then imposes a 10 second ban, without checking over how long a period those 50 calls were made (they might have taken minutes or even hours to accumulate).

Solutions:

For #1 I'd suggest raising the limit to 60+ calls per second.
For #2 a proper solution may be more complex, so we may have to live with that one after mitigating the others (whilst we wait for the clean solution to IPC throttling in  issue 672370 ).
For #3 and #4 a lazy token bucket algorithm should neatly solve these problems; e.g. something like the following:

struct {
  int tokens = 0;
  TimeTicks last_token_grant;
} state_flood_guard;

bool History::ShouldThrottleStateObjectChanges() {
  if (state_flood_guard.tokens > 0) {
    state_flood_guard.tokens--;
    return false;
  }

  const auto now = TimeTicks::Now();
  TimeDelta elapsed = now - state_flood_guard.last_token_grant;
  const int millis = base::saturated_cast<int>(elapsed.InMilliseconds());
  static constexpr int kMillisPerToken = 16;
  static constexpr int kMaxTokens = 50;
  const int tokens_earned = std::min(millis / kMillisPerToken, kMaxTokens);

  if (tokens_earned > 0) {
    state_flood_guard.tokens = tokens_earned - 1;  // One consumed immediately.
    state_flood_guard.last_token_grant = now;
    return false;
  }

  // Drop this event (though ideally it would be delivered once there are tokens
  // again, unless obsoleted by a subsequent state object change).
  return true;
}
 

Comment 1 by palmer@chromium.org, Nov 20 2017

Cc: elawrence@chromium.org
If you want/need to revert https://codereview.chromium.org/2972073002/, I'd understand. It's terrible, and doesn't really solve the problem.

But that would mean we'd need to prioritize https://bugs.chromium.org/p/chromium/issues/detail?id=394296 more highly. And I think we should anyway; it's an old bug, and scammers are using it as part of their (e.g.) tech support scams.

Comment 2 by bauerb@chromium.org, Feb 20 2018

Labels: -Pri-1 android-fe-triaged Pri-2
Owner: palmer@chromium.org
Status: Assigned (was: Untriaged)
Chris: Assigning to you to decide how to proceed here.

Comment 3 by palmer@chromium.org, May 4 2018

Issue 838887 has been merged into this issue.

Comment 4 by palmer@chromium.org, May 14 2018

Issue 842400 has been merged into this issue.

Comment 5 by palmer@chromium.org, Aug 2

Status: Started (was: Assigned)

Comment 6 by bugdroid1@chromium.org, Aug 3

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f637a1a25fc806655189b8acfe992736d02db6c

commit 1f637a1a25fc806655189b8acfe992736d02db6c
Author: Chris Palmer <palmer@chromium.org>
Date: Fri Aug 03 04:50:03 2018

Soften the history.{push,replace}State throttling mechanism.

johnme@ suggests a better way to handle it; let's do that.

Bug:  786211 
Change-Id: I446c70117451d2ef49ce13f5884ebb858d9ee392
Reviewed-on: https://chromium-review.googlesource.com/1161400
Commit-Queue: Chris Palmer <palmer@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580455}
[modify] https://crrev.com/1f637a1a25fc806655189b8acfe992736d02db6c/third_party/blink/renderer/core/frame/history.cc
[modify] https://crrev.com/1f637a1a25fc806655189b8acfe992736d02db6c/third_party/blink/renderer/core/frame/history.h

Comment 7 by palmer@chromium.org, Aug 3

Labels: OS-Fuchsia
Status: Fixed (was: Started)
Thanks johnme!

Comment 8 by bugdroid1@chromium.org, Aug 11

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

commit ad2f5b9fd0e6fb2462ca7e67e6c6c0da44f4d07e
Author: Peter Kasting <pkasting@chromium.org>
Date: Sat Aug 11 06:57:05 2018

Revert "Soften the history.{push,replace}State throttling mechanism."

This reverts commit 1f637a1a25fc806655189b8acfe992736d02db6c.

Reason for revert: Part 2 of trying to revert history throttling changes to see if https://bugs.chromium.org/p/chromium/issues/detail?id=870861 improves.

Original change's description:
> Soften the history.{push,replace}State throttling mechanism.
> 
> johnme@ suggests a better way to handle it; let's do that.
> 
> Bug:  786211 
> Change-Id: I446c70117451d2ef49ce13f5884ebb858d9ee392
> Reviewed-on: https://chromium-review.googlesource.com/1161400
> Commit-Queue: Chris Palmer <palmer@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580455}

TBR=palmer@chromium.org,dcheng@chromium.org,kinuko@chromium.org

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

Bug:  786211 
Change-Id: Ia6a35add136361834c3afb40295b950c05cb9169
Reviewed-on: https://chromium-review.googlesource.com/1171709
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582434}
[modify] https://crrev.com/ad2f5b9fd0e6fb2462ca7e67e6c6c0da44f4d07e/third_party/blink/renderer/core/frame/history.cc
[modify] https://crrev.com/ad2f5b9fd0e6fb2462ca7e67e6c6c0da44f4d07e/third_party/blink/renderer/core/frame/history.h

Sign in to add a comment