Issue metadata
Sign in to add a comment
|
window.history.{replace,push}State silently fail on throttling
Reported by
der.pi...@googlemail.com,
Dec 14 2017
|
||||||||||||||||||||
Issue descriptionUserAgent: 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
,
Dec 14 2017
@krajshree: Why "needs-bisect"? I already pointed out the exact change where this happened (https://crrev.com/485498)
,
Dec 15 2017
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...!!
,
Dec 15 2017
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...!!
,
Jan 3 2018
,
Jan 3 2018
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.
,
Jan 5 2018
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
,
Jan 5 2018
,
Jan 5 2018
@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.
,
Mar 14 2018
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
,
Aug 2
Started a pull request to change the spec to call for throwing SecurityError: https://github.com/whatwg/html/pull/3878. We'll see how that goes. If it doesn't fly, we'll have to consider the console message to be warning enough. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by krajshree@chromium.org
, Dec 14 2017