New issue
Advanced search Search tips
Starred by 1 user
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 Back to list
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
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
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
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
@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.
Sign in to add a comment