Bug 156115 - Change frequency limit on replaceState to fail without throwing an exception
Summary: Change frequency limit on replaceState to fail without throwing an exception
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: Safari 9
Hardware: All All
: P2 Major
Assignee: Nobody
URL: http://bl.ocks.org/jameskeane/raw/e0b...
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-01 13:54 PDT by James Keane
Modified: 2016-06-10 11:21 PDT (History)
3 users (show)

See Also:


Attachments
Chat example - must be served over http for security restrictions (3.78 KB, text/html)
2016-04-01 13:54 PDT, James Keane
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description James Keane 2016-04-01 13:54:32 PDT
Created attachment 275427 [details]
Chat example - must be served over http for security restrictions

An arbitrary 100 maximum calls to 'pushState' and 'replaceState' was recently added to WebKit, which I have been informed was recently softened to 100 calls per 30 second interval (https://trac.webkit.org/changeset/198687) in #155901.

I can find no reasonable justification for diverging from the spec (https://html.spec.whatwg.org/multipage/browsers.html#the-history-interface). I assume it is related to "crashsafari.com", but I am "not authorized" to view the ticket (#153435) which introduced this change.

I was asked on twitter by @beidson, how 3 updates per second is not enough; my use case is updating scroll state at 60fps, but this example of a chat app: http://bl.ocks.org/jameskeane/raw/e0b145e6287b3f12f5e47e6b1d313308/ also fails and is less easily coalesced.
Comment 1 Brady Eidson 2016-04-02 12:26:56 PDT
(In reply to comment #0)

> I was asked on twitter by @beidson, how 3 updates per second is not enough;
> my use case is updating scroll state at 60fps, 

Here's our thinking on this:
The use case for replaceState is to make sure that the current history item has the most recent state possible before the current history item changes.

The current history item really only needs to be updated exactly once - Right before the current history item moves to a new history item.

What is the difference between "updating the current history item 60 times during a 1 second scroll operation" versus "updating the current history item once at the end of a 1 second scroll operation"?

If the user is scrolling at 60fps for 1 second, for example, there's no need to update the state of the current history item 60 times. Only once at the end of the scrolling... or even better, only once right before the user "navigates"

In theory, I can't actually think of a reason you'd ever actually have to call "replaceState" other than "right before you ever call pushState, or once inside of your pagehide handler"

> but this example of a chat app:
> http://bl.ocks.org/jameskeane/raw/e0b145e6287b3f12f5e47e6b1d313308/
> also fails and is less easily coalesced.

Again, I'm confused why this is true - replaceState literally only needs to be called once before pushState, or once during pagehide.

I don't see what makes this chat app any different.

> I can find no reasonable justification for diverging from the spec
> (https://html.spec.whatwg.org/multipage/browsers.html#the-history-interface).

WebKit's current behavior is definitely inline with the HTML spec.

The WebApp section of the spec gives browsers leeway to prevent abuse of system resources by throwing the QuotaExceededError exception, which is precisely what we're doing here.
(https://html.spec.whatwg.org/multipage/webappapis.html#killing-scripts)

Also, the the History spec is clear about the flexibility user agents can take with regard to the state APIs in section 7.7.4. (https://html.spec.whatwg.org/multipage/browsers.html#history-notes)

While that section specifically calls out pushState, I assure you the intention was to call out replaceState also. I know this because I was directly involved in that conversation years ago when the APIs were new.

I've filed https://github.com/whatwg/html/issues/982 to update this.
Comment 2 James Keane 2016-04-04 09:58:12 PDT
(In reply to comment #1)

> The current history item really only needs to be updated exactly once - Right before the current history item moves to a new history item.

While that may be the case, it goes against the spirit of the API i.e. of allowing the state or URL to be replaced when convenient, otherwise why provide a method at all?

What you are describing may in fact be a better way to fix this issue in WebKit itself while hiding platform specific memory limitations.

Not being familiar with WebKit myself, how is this:
> for(var i = 0; i < 1000; i++) document.location.hash = String(i);
limited, protected, or otherwise made 'safe' in a similar manner?

As for Section 7.7.4 it states (emphasis mine):
> In addition, a user agent could *ignore calls* to pushState() that are invoked on a timer, or from event listeners that are not triggered in response to a clear user action, or that are invoked in rapid succession.

Which is *not* WebKit's behaviour in this case, as it is not merely ignoring those calls.

Also in Section 7.7.4, the paragraph you reference, labelled 'Security', is discussing history hijacking, in which a nefarious page prevents a user from using the back button to leave a page; which does not appear relevant to this discussion.
Comment 3 Brady Eidson 2016-04-04 21:49:16 PDT
(In reply to comment #2)
> (In reply to comment #1)
> 
> > The current history item really only needs to be updated exactly once - Right before the current history item moves to a new history item.
> 
> While that may be the case, it goes against the spirit of the API i.e. of
> allowing the state or URL to be replaced when convenient, otherwise why
> provide a method at all?

I'm suggesting that replaceState only needs to be called once once.

You're suggesting it's necessary to call it 60 fps.

"When convenient" is somewhere in between and would likely be allowed by the current policy.
 
> What you are describing may in fact be a better way to fix this issue in
> WebKit itself while hiding platform specific memory limitations.

WebKit cannot possibly know when the page is about to "fake navigate" via a pushState. Analyzing a program to try to figure that out is subject to the halting problem. Only the web author can know this.

> Not being familiar with WebKit myself, how is this:
> > for(var i = 0; i < 1000; i++) document.location.hash = String(i);
> limited, protected, or otherwise made 'safe' in a similar manner?

A browser engine can't stop JS programmers from doing *all* unreasonably inefficient things.

It can step in to stop particularly nefarious ones, though.

> 
> As for Section 7.7.4 it states (emphasis mine):
> > In addition, a user agent could *ignore calls* to pushState() that are invoked on a timer, or from event listeners that are not triggered in response to a clear user action, or that are invoked in rapid succession.
> 
> Which is *not* WebKit's behaviour in this case, as it is not merely ignoring
> those calls.

So you're suggesting it's the exception being thrown that is the problem, and not the policy itself?

If so, we should probably close this bug and restart that conversation with a clean slate, because that's a different conversation.

> Also in Section 7.7.4, the paragraph you reference, labelled 'Security', is
> discussing history hijacking, in which a nefarious page prevents a user from
> using the back button to leave a page; which does not appear relevant to
> this discussion.

Apologies for suggesting that I was only referencing that paragraph - I'm references the entire section.

If we're citing sections of the spec with emphasis, though, I'll emphasize 2.2.1 (https://html.spec.whatwg.org/multipage/infrastructure.html#conformance-classes):
> User agents *may impose implementation-specific limits on otherwise unconstrained inputs*, e.g. to prevent denial of service attacks, to guard against running out of memory, or to work around platform-specific limitations.

And 8.1.3.5 (https://html.spec.whatwg.org/multipage/webappapis.html#killing-scripts):
> ...it's sometimes necessary to abort a running script.
> ...
> User agents may impose resource limitations on scripts, for example CPU quotas, memory limits, total execution time limits, or bandwidth limitations. When a script exceeds a limit, the user agent may either throw a QuotaExceededError exception

QuotaExceededError when a script exceeds a limit is what we're doing.

Again, if the objection is to the exception as opposed to the limit itself, we should discuss in a clean slate bug.
Comment 4 James Keane 2016-04-05 08:50:49 PDT
(In reply to comment #3)
You've convinced me, that the spec is flexible enough to allow user agents to impose limits on unreasonable usage. I will concede that it is on your authority to define what is and what isn't 'unreasonable' based on, as Section 2.2.1 calls it, 'implementation-specific limits'.

Brady, I would also like to personally apologize, if my initial tweets offended you; I was frustrated at what, at the time, I believed was a spec violating change causing hundreds of thousands of users to see a completely broken site.

I will leave it to you to decide whether throwing a QuotaExceededError, or as the spec _suggests_ ignoring calls is the preferred behaviour; my opinion is the latter. 

Either way, I think we can both agree that finding a solution which is not subject to these limits is preferable. 
> WebKit cannot possibly know when the page is about to "fake navigate" via a pushState.
It can, and it is definitely *not* a halting problem; it knows when the page is about to fake navigate, as it knows when pushState is called.

Again, not knowing the specifics, how could 'replaceState' create an OOM situation? Is it not deallocating the previous state object?
Comment 5 Brady Eidson 2016-04-05 16:14:20 PDT
Re-ordering my reply a bit to make a better narrative:

(In reply to comment #4)
> (In reply to comment #3)
> Brady, I would also like to personally apologize, if my initial tweets
> offended you; I was frustrated at what, at the time, I believed was a spec
> violating change causing hundreds of thousands of users to see a completely
> broken site.

Apology noted, but not necessary; I wasn't offended, I just wanted to probe the use case and get to the bottom of the problem.

> I will leave it to you to decide whether throwing a QuotaExceededError, or
> as the spec _suggests_ ignoring calls is the preferred behaviour; my opinion
> is the latter.

We will definitely consider ignoring the call, but logging to the console anyways.

> Again, not knowing the specifics, how could 'replaceState' create an OOM
> situation? Is it not deallocating the previous state object?

Yes, replaceState deallocates the previous state objects, but this policy is not about OOM.

Calling replaceState at a high frequency, even with tiny payloads, does a surprising amount of work. It burns through a surprising amount of CPU, which means a surprising amount of battery.

And if the frequency is high enough it actually quickly turns into a DOS attack. Not only on the current web page but on the entire browser, as the browser itself is necessarily involved with session history management.

> > WebKit cannot possibly know when the page is about to "fake navigate" via a pushState.
> It can, and it is definitely *not* a halting problem; it knows when the page
> is about to fake navigate, as it knows when pushState is called.

That's absolutely true.

Let me rephrase: WebKit can't do analysis about each replaceState to see if it is the "last replaceState that is needed before a pushState"

I believe what you're considering an appropriate fix in WebKit would be remember the most recent "replaceState" in a lightweight manner, then only do the heavyweight work when pushState is called or a navigation happens.

We considered that at first, but there are significant problems with it, both technical and architectural.
Comment 6 Brady Eidson 2016-04-05 16:16:10 PDT
Retitling to:
Change frequency limit on replaceState to fail without throwing an exception

The goal being:
- Both the size and frequency limits on pushState will remain, with exception.
- The size limit on replaceState will remain, with exception.
- The frequency limit on replaceState will cause replaceState to silently fail (with a one-time log to the console)