Throttle calls to "window.history.replaceState" to fix Mobile Safari error message #5613

lucaswoj · 2017-11-07T23:05:11Z

As of iOS 9.3, Mobile Safari throws an error if window.history.replaceState is called more than 100 times per 30 seconds. This PR throttles all calls to window.history.replaceState to avoid triggering this error message.

Benchmark scores: https://bl.ocks.org/anonymous/raw/9a02ed1c37970d9d6c697f667b56527a/

fixes #5612

Next Steps

briefly describe the changes in this PR
Fix failing unit tests
(Debug custom util.throttle & write tests for util.throttle) or find acceptable throttle implementation
Post benchmark scores
Debug moveend deboucing
manually test the debug page

Test Harness

<!DOCTYPE html>
<html>
<head>
    <title>Mapbox GL JS debug page</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
    <link rel='stylesheet' href='/dist/mapbox-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>

<script src='/dist/mapbox-gl-dev.js'></script>
<script src='/debug/access_token_generated.js'></script>
<script>

var map = window.map = new mapboxgl.Map({
    container: 'map',
    zoom: 12.5,
    center: [-77.01866, 38.888],
    style: 'mapbox://styles/mapbox/streets-v10',
    hash: true
});

setInterval(() => {
    map.flyTo({
        bearing: map.getBearing() + 1
    });
}, 100)


</script>
</body>
</html>

…error message fixes #5612

mourner

I wonder, why is it called so often in the first place? _updateHash is called on moveend. If moveend has been called 100 times in 30 seconds, and given that moveend itself is debounced (called only once in case of many quick successive movements of the map), there might be a bug lurking there of which this is only a symptom.
Also, I'd prefer bringing back the little util.throttle we had before but removed at some point than including a part of lodash for the first time in the GL JS build.

lucaswoj · 2017-11-08T20:35:17Z

I wonder, why is it called so often in the first place? _updateHash is called on moveend. If moveend has been called 100 times in 30 seconds, and given that moveend itself is debounced (called only once in case of many quick successive movements of the map), there might be a bug lurking there of which this is only a symptom.

The provided test harness demonstrates a realistic use case that would trigger the error message even if we revamp the panning moveend debouncing. I will investigate further to figure out why panning produces so many moveend events.

Also, I'd prefer bringing back the little util.throttle we had before but removed at some point than including a part of lodash for the first time in the GL JS build.

I experienced a bug in our util.throttle implementation (replaceState was still being invoked more often that it should've) which went away when I switched lodash.throttle. I couldn't find any tests for util.throttle.

I will put more time into debugging util.throttle but we may want to consider a not-invented-here solution.

lucaswoj · 2017-11-08T20:42:11Z

I must confess I'm disappointed by the size of lodash.throttle: 16 KB, 430 LOC (unminified and with comments).

lucaswoj · 2017-11-09T00:28:22Z

I wonder, why is it called so often in the first place? _updateHash is called on moveend. If moveend has been called 100 times in 30 seconds, and given that moveend itself is debounced (called only once in case of many quick successive movements of the map), there might be a bug lurking there of which this is only a symptom.

The problem is that DragPanHandler calls Camera#panBy which calls Camera#panTo which calls Camera#easeTo which calls Camera#fire('moveend', ...) in an unthrottled way. My view is that the real root issue here is #3357.

mourner · 2017-11-09T23:08:43Z

src/util/throttle.js

+            unthrottledFunction();
+        } else if (!pending) {
+            pending = true;
+            // eslint-disable-next-line no-use-before-define


The ESLint docs on the rule state that:

In ES6, block-level bindings (let and const) introduce a “temporal dead zone” where a ReferenceError will be thrown with any attempt to access the variable before its declaration.

So if this code weren't transpiled (which may happen in future) it would throw an error in theory. So perhaps let's rewrite to either use function declaration instead of arrows, or declare one of the vars at the top with let?

Whoa! I've been out in the woods too long. Fixed in 20f3fe0.

Throttle calls to "window.history.replaceState" to fix Mobile Safari …

753b37c

…error message fixes #5612

lucaswoj added the under development label Nov 7, 2017

mourner reviewed Nov 8, 2017

View reviewed changes

lucaswoj mentioned this pull request Nov 8, 2017

"history.replaceState" error on Mobile Safari #5612

Closed

Lucas Wojciechowski added 2 commits November 8, 2017 14:27

Fix unit tests

45cf158

Use custom throttle function

3ac5097

lucaswoj force-pushed the throttle-hash-5612 branch from 44f6aa6 to 3ac5097 Compare November 9, 2017 00:03

Appease the linter gods

7c0da67

lucaswoj removed the under development label Nov 9, 2017

Lucas Wojciechowski added 2 commits November 9, 2017 14:24

Revert changes to debug/index.html

3eb7a25

Revert changes to util.js

ebc326e

mourner reviewed Nov 9, 2017

View reviewed changes

Lucas Wojciechowski added 2 commits November 9, 2017 15:31

Switch to function declarations

20f3fe0

Use @mourner's throttle implementation

6eb096b

lucaswoj force-pushed the throttle-hash-5612 branch from c93e2c2 to 6eb096b Compare November 10, 2017 00:40

mourner approved these changes Nov 10, 2017

View reviewed changes

lucaswoj merged commit cfc8503 into master Nov 15, 2017

lucaswoj deleted the throttle-hash-5612 branch November 15, 2017 23:47

renovate bot mentioned this pull request Feb 2, 2018

Update dependency mapbox-gl to ^0.44.0 - autoclosed oSoc17/rideaway-frontend#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttle calls to "window.history.replaceState" to fix Mobile Safari error message #5613

Throttle calls to "window.history.replaceState" to fix Mobile Safari error message #5613

lucaswoj commented Nov 7, 2017 •

edited

Loading

mourner left a comment

lucaswoj commented Nov 8, 2017 •

edited

Loading

lucaswoj commented Nov 8, 2017

lucaswoj commented Nov 9, 2017

mourner Nov 9, 2017 •

edited

Loading

lucaswoj Nov 9, 2017

Throttle calls to "window.history.replaceState" to fix Mobile Safari error message #5613

Throttle calls to "window.history.replaceState" to fix Mobile Safari error message #5613

Conversation

lucaswoj commented Nov 7, 2017 • edited Loading

Next Steps

Test Harness

mourner left a comment

Choose a reason for hiding this comment

lucaswoj commented Nov 8, 2017 • edited Loading

lucaswoj commented Nov 8, 2017

lucaswoj commented Nov 9, 2017

mourner Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

lucaswoj Nov 9, 2017

Choose a reason for hiding this comment

lucaswoj commented Nov 7, 2017 •

edited

Loading

lucaswoj commented Nov 8, 2017 •

edited

Loading

mourner Nov 9, 2017 •

edited

Loading