New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Started
Merged: issue 769592
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression



Sign in to add a comment

window.history.pushState() silently fails on throttling

Reported by der.pi...@googlemail.com, Dec 14

Issue description

UserAgent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

Steps to reproduce the problem:
1. Call window.history.pushState() very often

What is the expected behavior?
If the function returns and does not throw an error, it should work.

What went wrong?
pushState() silently does nothing when it gets called too often. This was introduced in https://crrev.com/485498.

It took us hours to figure out that sleeping for 3 secons in our test case helps, and about three man-days to figure out *why*. An API should not silently fail, if pushState() runs into throttling it should throw an exception. At the very least it should log an error on the console so that web developers know *why* their code/tests are behaving strangely, but there is no reason to let this silently succeed: valid web sites call pushState() for a reason, and not actually applying it will break their logic anyway later on. So it's much better to be explicit about it and throw an error.

Did this work before? Yes 

Does this work in other browsers? Yes

Chrome version: 62.0.3202.89  Channel: stable
OS Version: Fedora 27
Flash Version: None
 
Labels: Needs-Bisect Needs-Milestone
@krajshree: Why "needs-bisect"? I already pointed out the exact change where this happened (https://crrev.com/485498)
der.pitti@ - It is a process that we follow in order to get the issue assigned to the culprit(dev) and also the behaviour was good earlier. So, Needs-Bisect label is added.

Thanks...!!
Components: Internals>Core
Labels: -Pri-2 -Needs-Bisect Triaged-ET M-65 hasbisect OS-Mac OS-Windows Pri-1
Owner: palmer@chromium.org
Status: Assigned (was: Unconfirmed)
As per comment #0, change landed in https://chromium.googlesource.com/chromium/src/+/95558ee76640b366e8b775bd93483b99af379f55
Review-Url: https://codereview.chromium.org/2972073002

palmer@ - Could you please check whether this is caused with respect to your change.

Thanks...!!
Mergedinto: 769592
Status: Duplicate (was: Assigned)
Please unmerge this bug, this is *not* a duplicate. This report is about pushState() being broken in the *normal* operation case, i. e. the API silently failing. The throttling must make the function call fail with an exception, possibly log an error, and also get documented, so that application authors don't need to spend days on figuring out what happens.

Bug 769592 is about adding a flag to disable the throttling, but first this isn't discoverable until you actually know that it's pushState() that causes errors somewhere later in your code, and second this might just be an actual bug in your code - not every case of this is a test suite.
Labels: OS-Android OS-Chrome OS-Fuchsia
Status: Assigned (was: Duplicate)
I'll un-duplicate it, but I think the same CL can fix both aspects of the problem. You can follow along here:

https://chromium-review.googlesource.com/c/chromium/src/+/695645/2/third_party/WebKit/Source/core/frame/History.cpp
Status: Started (was: Assigned)
@palmer: Ah, thank you! I read through the patch and the discussion, and it seems to go into the right direction (raising an exception), but the initial version with a simple log message is enough too for now. Thanks! So, fine to keep this as merged then.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 14

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

commit 917bc057cb5c399d853f1e4ac159b973b58e0294
Author: Chris Palmer <palmer@chromium.org>
Date: Wed Mar 14 19:28:31 2018

Log a message on the console when the {push,replace}State quota is exceeded.

Previously, history.{push,replace}State spam failed silently, causing developer
confusion.

This change also introduces the --disable-pushstate-throttle flag to allow users
to disable the throttling.

This work was primarily authored by Trent Willis <trentmwillis@gmail.com>.

R=creis@chromium.org, dcheng@chromium.org

Bug: 769592, 794923
Change-Id: I969456b190d535f8dee3e03b5c4f4f36730d6b38
Reviewed-on: https://chromium-review.googlesource.com/851000
Commit-Queue: Chris Palmer <palmer@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543154}
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/AUTHORS
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/chrome/browser/about_flags.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/public/common/content_switches.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/public/common/content_switches.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/public/common/web_preferences.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/public/common/web_preferences.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/content/renderer/render_view_impl.cc
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/third_party/WebKit/Source/core/exported/WebSettingsImpl.cpp
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/third_party/WebKit/Source/core/exported/WebSettingsImpl.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/third_party/WebKit/Source/core/frame/History.cpp
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/third_party/WebKit/Source/core/frame/Settings.json5
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/third_party/WebKit/public/web/WebSettings.h
[modify] https://crrev.com/917bc057cb5c399d853f1e4ac159b973b58e0294/tools/metrics/histograms/enums.xml

Sign in to add a comment