Make history navigations asynchronous
Categories
(Core :: DOM: Navigation, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: neha, Assigned: smaug)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(3 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
1.21 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68-
|
Details | Review |
For Fission, we need to make session history navigations (history.back, forward, go(), etc.) asynchronous.
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
A simple change (but almost what the spec says, but the spec may be totally broken here).
Let's see how many tests are broken
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f278651f77a84e18939600e67111fe7c75a8e143
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
And to clarify, this bug is not about popstate handling. That is rather different thing and handled in bug 1281952.
Quite a few of the wpt tests fail because of sync popstate.
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd2d4f4244247b46ca2a067050efcf0d6b259a7
(Trying to make GeckoView's fragment navigation less broken in general)
Assignee | ||
Comment 5•5 years ago
|
||
The main part of the change is the change to ChildSHistory - make it possible to have Go() to be called asynchronously
and also let one to cancel pending history navigations. History object (window.history) can then use either the sync or
async Go(), depending on the dom.window.history.async pref.
LoadDelegate, which is used by GeckoView, needs special handling, since
it spins event loop nestedly. With session history loads and same-document loads we can just
bypass it.
To deal with same-document case, MaybeHandleSameDocumentNavigation is split to IsSameDocumentNavigation,
which collects relevant information about the request and returns true if same-document navigation should happen,
and then later HandleSameDocumentNavigation uses that information to trigger the navigation.
SameDocumentNavigationState is used to pass the information around.
Enabling the pref explicitly in dir.ini so that if we need to disable the pref on Nightly, we can
still run the tests.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
(Need to update the patch, I was missing making some parts async :/)
Updated•5 years ago
|
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a365d3c43261 Make history.back/forward/go asynchronous, r=farre
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18401 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/18401 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/LDH8QFxuT169A_RvUiiohA)
Assignee | ||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d07f2e7d6f5e Fix lint issue. r=lint-fix
Comment 12•5 years ago
|
||
Backed out 2 changesets (bug 1563587) for junit failure
Backout: https://hg.mozilla.org/integration/autoland/rev/8d2d3b3fb7e27f0bd238985c265b4f50dc10bc05
Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a365d3c4326127be0d85de3c7027cfd4174a4177
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261401491&repo=autoland&lineNumber=1779
Assignee | ||
Updated•5 years ago
|
Upstream PR was closed without merging
Comment 14•5 years ago
|
||
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/214fac6eb1c0 Make history.back/forward/go asynchronous, r=farre
Comment 15•5 years ago
|
||
bugherder |
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/18401 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Ij8t_4qNRDyGBlXP59uPrg)
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/18401 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Ij8t_4qNRDyGBlXP59uPrg)
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/18401 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Ij8t_4qNRDyGBlXP59uPrg)
Upstream PR merged
Comment 20•5 years ago
|
||
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd47d1e1ed5 MANUAL PUSH: remove debugging left-over dump() calls, r=wpt-bustage
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18493 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 23•5 years ago
|
||
bugherder |
Comment 24•5 years ago
|
||
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0fffc6007d disable iframe navigation parts of nested-context-navigations-iframe.html, r=wpt-failure
Comment 25•5 years ago
|
||
bugherder |
Comment 26•5 years ago
|
||
This is not just a refactor; it may affect websites that relied on the synchronous behavior (which would be incompatible with the web).
For example:
history.pushState(1, "");
history.pushState(2, "");
history.back();
console.log(history.state);
history.forward();
console.log(history.state);
// Result (Firefox 69): "1", "2" and returned to the initial history entry
// Result (Firefox 70, Chromium 76, Safari 12.1.2): "2", "2" and ends at the previous history entry (history.forward did not appear to work).
Shouldn't this change be listed on MDN (dev-doc-needed) or be added to https://www.fxsitecompat.dev ?
I didn't look at the full patch in detail, but at the top of the patch setTimeout
was added after history.back()
. Shouldn't the popstate
event be used instead, to make sure that the history navigation has been processed?
Assignee | ||
Comment 27•5 years ago
•
|
||
(In reply to Rob Wu [:robwu] from comment #26)
Shouldn't this change be listed on MDN (dev-doc-needed) or be added to https://www.fxsitecompat.dev ?
yes
I didn't look at the full patch in detail, but at the top of the patch
setTimeout
was added afterhistory.back()
. Shouldn't thepopstate
event be used instead, to make sure that the history navigation has been processed?
I don't know which setTimeout you're talking about. In some cases popstate would have worked, in some case not.
And wpt tests got modified some more on github.
Assignee | ||
Updated•5 years ago
|
Comment 28•5 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #27)
(In reply to Rob Wu [:robwu] from comment #26)
I didn't look at the full patch in detail, but at the top of the patch
setTimeout
was added afterhistory.back()
. Shouldn't thepopstate
event be used instead, to make sure that the history navigation has been processed?I don't know which setTimeout you're talking about. In some cases popstate would have worked, in some case not.
And wpt tests got modified some more on github.
The top of [ the patch ] ( https://hg.mozilla.org/mozilla-central/rev/214fac6eb1c0 ), i.e.
--- a/browser/base/content/test/sanitize/browser_purgehistory_clears_sh.js
+++ b/browser/base/content/test/sanitize/browser_purgehistory_clears_sh.js
@@ -27,16 +27,19 @@ add_task(async function purgeHistoryTest
ok(backButton.hasAttribute("disabled"), "Back button is disabled");
ok(forwardButton.hasAttribute("disabled"), "Forward button is disabled");
await ContentTask.spawn(browser, null, async function() {
let startHistory = content.history.length;
content.history.pushState({}, "");
content.history.pushState({}, "");
content.history.back();
+ await new Promise(function(r) {
+ setTimeout(r);
+ });
... but also the many (indirect) setTimeout
s that are called after history.back
or history.forward
. I would expect those tests to fail intermittently if the implementation of history navigation changes (to the extent that it takes longer than one macrotask).
Comment 29•5 years ago
|
||
Just to make confirm before I make the change that I'm not missing something important. Is the main change (where the docs are concerned) that history.back, history.forward, and history.go are no asynchronous and return a promise?
Assignee | ||
Comment 30•5 years ago
|
||
No Promises around anywhere.
The API just work asynchronously.
Comment 31•5 years ago
|
||
They are asynchronous indeed, but they don't return a promise. To detect completion of history.back
/forward
/go
, the popstate event has to be used instead.
Comment 32•5 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #31)
They are asynchronous indeed, but they don't return a promise. To detect completion of
history.back
/forward
/go
, the popstate event has to be used instead.
Thank you. I'm not terribly good with asynchronous programming. I can't seem to get it into my head properly. I though that in JavaScript, asynchronous implied promises.
Comment 33•5 years ago
|
||
Update made to the go, back, and forward methods to include the information that the methods are now asynchronous. Also added a statement to the effect to the Firefox 70 release notes:
The back(), forward(), and go() methods are now asynchronous. Add a listener to the popstate event to get notification that navigation has completed bug 1563587.
Comment 34•5 years ago
|
||
Posted site compatibility note: https://www.fxsitecompat.dev/en-CA/docs/2019/history-navigation-methods-are-now-asynchronous/
Updated•5 years ago
|
Assignee | ||
Comment 35•5 years ago
|
||
Assignee | ||
Comment 36•5 years ago
|
||
There might be need for this on esr68
esr68 try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc64dc5b70a40d86b2ecf33b13d43f86dbd4c834
Assignee | ||
Comment 37•5 years ago
|
||
Apparently tryserver had some issues unrelated to the patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f2f8f61c907b529e6462a8c34bd12182562cca
Assignee | ||
Comment 38•5 years ago
|
||
And need also bug 1396309 for esr
Assignee | ||
Comment 39•5 years ago
|
||
Comment on attachment 9114908 [details]
Bug 1563587, Make history.back/forward/go asynchronous, esr68, r=farre
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: To have consistent history.* handling also in ESR, and especially because of
https://bugzilla.mozilla.org/show_bug.cgi?id=1528587#c45 - User impact if declined: See bug 1528587
- Fix Landed on Version: 70
- Risk to taking this patch: Medium
- Why is the change risky/not risky? (and alternatives if risky): Changes to history API behavior tend to cause regressions.
The original patch did regress wikipedia (largely because they had Chrome-only code to deal with asynchronousness). - String or UUID changes made by this patch:
Comment 40•5 years ago
|
||
Is there a chance this will affect fennec particularly when we uplift? (Any specific testing we can/should do around that?)
Assignee | ||
Comment 41•5 years ago
|
||
The patch would affect all 68. But if we want to excluded Fennec, dom.window.history.async could be set to false there.
Comment 42•5 years ago
|
||
Comment on attachment 9114908 [details]
Bug 1563587, Make history.back/forward/go asynchronous, esr68, r=farre
I discussed this with Dan and don't think this risk of taking this patch (with at least one known regression since it shipped on release users) is justified against the severity of the bug being fixed.
Comment 43•5 years ago
|
||
This bug appears to have caused a regression - https://bugzilla.mozilla.org/show_bug.cgi?id=1605556
Description
•