Closed Bug 1563587 Opened 3 months ago Closed 2 months ago

Make history navigations asynchronous

Categories

(Core :: Document Navigation, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: neha, Assigned: smaug)

References

(Depends on 1 open bug, Blocks 1 open bug, Regressed 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

For Fission, we need to make session history navigations (history.back, forward, go(), etc.) asynchronous.

Type: defect → task
Priority: -- → P2
Assignee: nobody → bugs
Status: NEW → ASSIGNED

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

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd2d4f4244247b46ca2a067050efcf0d6b259a7
(Trying to make GeckoView's fragment navigation less broken in general)

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.

Attachment #9083993 - Attachment is obsolete: true

(Need to update the patch, I was missing making some parts async :/)

Attachment #9083993 - Attachment is obsolete: false
Depends on: 1572932
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)
Attached patch tmp.diffSplinter Review
Flags: needinfo?(bugs)
Upstream PR was closed without merging
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/214fac6eb1c0
Make history.back/forward/go asynchronous, r=farre
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
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
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.
Regressions: 1575437
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

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?

Flags: needinfo?(bugs)

(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 after history.back(). Shouldn't the popstate 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.

Flags: needinfo?(bugs)
Keywords: dev-doc-needed

(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 after history.back(). Shouldn't the popstate 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) setTimeouts 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).

Regressions: 1573647

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?

Flags: needinfo?(bugs)

No Promises around anywhere.
The API just work asynchronously.

Flags: needinfo?(bugs)

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.

(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.

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.

You need to log in before you can comment on or make changes to this bug.