1804140 - Require SHIP on desktop even when Fission is disabled

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently code which has to deal with session history is considerably more complex, due to needing to support both SHIP and non-SHIP configurations. We haven't managed to ship SHIP everywhere yet (bug 1736121), and Android still runs with SHIP and Fission disabled by default, however we have been shipping Fission (which requires SHIP) by default on Desktop for the last year.

We could consider forcing SHIP on for Desktop even when Fission is disabled in order to allow us to remove the non-SHIP codepath from desktop-only code such as sessionstore, which could be a substantial reduction in complexity for that code.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP: Bug 1804140 - Part 1: Enable SHIP by default on desktop, whether or not Fission is disabled, r=peterv!

This will allow us to stop handling the non-SHIP case in desktop-only code. It
shouldn't impact release users, as we have been shipping Fission (which
requires SHIP) on desktop by default for a year. SHIP is not enabled on
Android, as supporting it is still WIP.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP: Bug 1804140 - Part 2: Remove non-SHIP SessionStore code, r=farre!

In part 1, SHIP was enabled by-default on desktop, meaning that the
SessionStore code no longer needs to handle SHIP being disabled. This allows us
to rip out a large chunk of sessionstore code which was non-SHIP-only.

Depends on D163880

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP: Bug 1804140 - Part 3: Respect new SHIP config for tests, ???

Depends on D163881

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP: Bug 1804140 - Part 4: Stop handling SHIP being disabled in browser mochitests, r=peterv!,gijs!

In part 1, SHIP was enabled by default outside of Android. As browser
mochitests are only run on desktop, this means that we no longer need to handle
non-SHIP cases when running browser mochitests.

Depends on D163882

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Posted some patches doing an initial version of this. Not sure if they're good enough to land (especially the test runner python, which I'm not super familiar with), and just doing a first-pass try push right now. https://treeherder.mozilla.org/jobs?repo=try&revision=65a3af70a8b902f5d2f6722e6c6ce0407cf66fd7

Comment 6

[Peter Van der Beken [:peterv]]

This will fail a bunch of tests which I have actual fixes for (chrome tests, xpcshell tests, WPT annotations, …). Here's ùy try push from last week with the relevant fixes: https://treeherder.mozilla.org/jobs?repo=try&revision=14cd27e001b451f487e72730bcd1f09bbdd6eac9

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Bug 1804140 - Change wpt annotations for tests from Fission to SHIP. r?smaug!

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Bug 1804140 - Enable SHIP by default on desktop, whether or not Fission is disabled. r?smaug!

Depends on D172277

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f4ae1ab430d
Change wpt annotations for tests from Fission to SHIP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/14243ef3e860
Enable SHIP by default on desktop, whether or not Fission is disabled. r=smaug,jgraham

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing multiple failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/c6a53f72103446c7268d949b93bff949e4b931ea

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | gfx/tests/mochitest/test_acceleration.html | uncaught exception - NS_ERROR_XPC_SECURITY_MANAGER_VETO: at get

Failure log -> TEST-UNEXPECTED-FAIL | dom/tests/mochitest/chrome/test_focus.xhtml | Test timed out

Failure log wd - >TEST-UNEXPECTED-FAIL | /webdriver/tests/navigate_to/navigate.py | test_cross_origin[capabilities0] - webdriver.error.StaleElementReferenceException: stale element reference (404):

Failure log remote -> TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/runtime/browser_executionContextEvents.js | Received Runtime context events in expected order

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbe9d2f66fcc
Change wpt annotations for tests from Fission to SHIP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/974558fd2790
Enable SHIP by default on desktop, whether or not Fission is disabled. r=smaug,jgraham,webdriver-reviewers,whimboo

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for causing webdriver failures

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=wd&tochange=c338dcd28e27eb35a9d79f0d2d869558735799d5&fromchange=974558fd27906363c747f44265c4562215cd97af&selectedTaskRun=FE3XpfV5Ql6QQ2mcywRlHg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=412417980&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/67e0f79fcc88f4d84794e41238544efe636c9993

Comment 14

[Peter Van der Beken [:peterv]]

Henrik, here's some more failures when trying to enable SHIP without Fission. It looks like they all have a similar cause:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/navigate_to/navigate.py#60-80
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/back/back.py#151-169
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/forward/forward.py#176-195

Looking at the navigate.py failure:

• With Fission we switch process for the cross-origin load, so we send a "browsing-context-discarded" notification for the previous page which clears the NodeCache. The element is removed from the node cache, which makes https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/navigate_to/navigate.py#77-78 work.

• Without Fission and without SHIP, we don't BFCache because we attempt to process switch for the cross-origin load (but we don't). That destroys the frameloader, resulting in a "browsing-context-discarded" notification and so we clear the NodeCache. Things work mostly like in the Fission case.

• Without Fission but with SHIP, we do BFCache because the process switch code determines that we can put the frameloader in the BFCache. The NodeCache isn't cleared, because we don't destroy the frameloader and we don't process switch. The specific error changes to error.StaleElementReferenceException instead of the expected error.NoSuchElementException, because we now fall into the code in https://searchfox.org/mozilla-central/source/remote/marionette/element.sys.mjs#508. I can make the test pass by forcing the page to not be BFCached (with Cache-Control: no-store).

We can fix the test to expect both exceptions, or we can force the page to not be BFCached. Henrik, any suggestion?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, this is actually a known regression that needs to be fixed and is covered by bug 1822466. I would suggest that you add an exception for those tests for non-Fission in a newly created meta file and reference this bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ac74c309223
Change wpt annotations for tests from Fission to SHIP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c437272e68c3
Enable SHIP by default on desktop, whether or not Fission is disabled. r=smaug,jgraham,webdriver-reviewers,whimboo

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5b86968f797
Enable SHIP by default on desktop, whether or not Fission is disabled, followup to fix bustage. CLOSED TREE

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88de19897599
fix wpt annotation to disable back.py test on Windows opt 64-bit. CLOSED TREE

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b510f9863597
set reason for disabled test. DONTBUILD CLOSED TREE

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3ac74c309223
https://hg.mozilla.org/mozilla-central/rev/c437272e68c3
https://hg.mozilla.org/mozilla-central/rev/a5b86968f797
https://hg.mozilla.org/mozilla-central/rev/88de19897599
https://hg.mozilla.org/mozilla-central/rev/b510f9863597

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39685 for changes under testing/web-platform/tests

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 23

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1804140 - Part 1: Update tests for SHIP. r?#dom-core

Comment 24

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1804140 - Part 2: Remove SessionHistoryInParent conditionals. r?#dom-core

Comment 25

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1804140 - Part 3: Remove disableSessionHistoryInParent pref. r?#dom-core

Comment 26

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1804140 - Part 4: Remove nsSHEntry and nsSHEntryShared. r?#dom-core

Comment 27

[Phabricator Automation]

Comment on attachment 9397697 [details]
WIP: Bug 1804140 - Part 1: Update tests for SHIP. r?#dom-core

Revision D208060 was moved to bug 1892551. Setting attachment 9397697 [details] to obsolete.

Comment 28

[Phabricator Automation]

Comment on attachment 9397698 [details]
WIP: Bug 1804140 - Part 2: Remove SessionHistoryInParent conditionals. r?#dom-core

Revision D208061 was moved to bug 1892551. Setting attachment 9397698 [details] to obsolete.

Comment 29

[Phabricator Automation]

Comment on attachment 9397699 [details]
WIP: Bug 1804140 - Part 3: Remove disableSessionHistoryInParent pref. r?#dom-core

Revision D208062 was moved to bug 1892551. Setting attachment 9397699 [details] to obsolete.

Comment 30

[Phabricator Automation]

Comment on attachment 9397700 [details]
WIP: Bug 1804140 - Part 4: Remove nsSHEntry and nsSHEntryShared. r?#dom-core

Revision D208063 was moved to bug 1892551. Setting attachment 9397700 [details] to obsolete.