1865124 - [wdspec] Enable tests for "browsingContext.traverseHistory" for "Session History in Parent" (SHIP) variant

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P3)

Description

[Alexandra Borovova [:Sasha]]

The implementation of browsingContext.traverseHistory, introduced in the scope of the bug1841018, uses browsingContext.sessionHistory property, which is not available on Android with fission disabled. In the scope of this bug, we should either investigate why browsingContext.sessionHistory is not available or use another way to get information about current history index and the amount of entries.

Comment 1

[Alexandra Borovova [:Sasha]]

Hi Olivia,
as mentioned in this bug, we're currently implementing browsingContext.traverseHistory and we noticed that browsingContext.sessionHistory property on Android with fission disabled equals null, it works fine with fission enabled. Do you maybe know anything about it? Is it maybe a bug or just something which is not implemented?

Comment 2

[Olivia Hall [:olivia]]

Hi Alexandra,

Thanks for taking a look into this area!

My understanding is that there are a lot of considerations with session history on GeckoView and we are actively making changes. Let me redirect this question to either owlish or kaya, who are both working on Fission and have background on session history.

Comment 3

[Kaya [:kaya]]

Hi :Sasha,

Thank you very much for bringing this up and :olivia for letting me know of this :)

browsingContext.sessionHistory will be a part of enabling SHIP on Android regardless of fission being enabled/disabled. It is used in GeckoViewSessionStore's SHistoryListener for collecting the history from parent. We will ship it without Fission. Worth to note, enabling Fission automatically makes use of SHIP iirc.

Currently I am working on enabling SHIP on Android. After this work, GeckoViewSessionStore will be in use instead of SessionStateAggregator.

If there's anything else I can help related to session history on Android, please let me know. I am not clearing ni request for :owlish in case there's anything else worth mentioning related to Fission.

Comment 4

[Alexandra Borovova [:Sasha]]

Hi :kaya,

That's great news! I've tried to run the wpt tests, which are now disabled for Android with fission disabled, on top of the revision, and it seems to be working as we expect! So I'll block then this bug on bug1677190.

Thanks a lot for the context and your work :)

Comment 5

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

With bug 1677190 fixed we now only have to wait until we ship for 100% of our users, or can we already get the tests enabled in CI? How is SHIP handled there? Owlish or Kaya, do you have some info for us? Thanks.

The wdspec tests to enable:``
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/webdriver/tests/bidi/browsing_context/traverse_history

Comment 6

[Kaya [:kaya]]

Hi Henrik,

We are currently testing for all fission, no-fission-no-ship, and no-fission-ship cases. fission case has ship auto-enabled.
Considering the disabled tests for condition if (os == "android") and not fission, I'd say we'll have to keep it for now until we stop caring about no-fission-no-ship variant. But I believe we should be able to revise this condition by replacing not fission with not sessionHistoryInParent to only disable it for no-fission-no-ship and start covering no-fission-ship case.
(ref condition).

Did I understand your question correctly, give a feasible suggestion for now? Let me know if I misunderstood it.

Comment 7

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

Thank you for the reply Kaya. I think that we should update the summary of the bug accordingly so that it is really a question about the case when SHIP is not enabled. Most likely we are going to wontfix this bug given that I do not see a reason why to have to support a mode that we are going to disable first but will also remove in the near future.

I've pushed a try build for an update of the metadata to enable the browsingContext.traverseHistory tests for SHIP. Hereby I also had to enable this variant for specifically wdspec tests (which wasn't done yet):

https://treeherder.mozilla.org/jobs?repo=try&revision=5548566c40fe47f94e3f3a086dd2a20682b93e2a

Comment 8

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

Actually it doesn't look like that browsingContext.sessionHistory is available. The tests are still failing for nofis-ship jobs.

Kaya, this doesn't match what you said in comment 3. So maybe you can have a look at it again?

Comment 9

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

(In reply to Alexandra Borovova [:Sasha] from comment #4)

That's great news! I've tried to run the wpt tests, which are now disabled for Android with fission disabled, on top of the revision, and it seems to be working as we expect! So I'll block then this bug on bug1677190.

Thanks a lot for the context and your work :)

As it looks like something might have changed for the implementation of SHIP after this comment was made?

Comment 10

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

Attachment: Bug 1865124 - [wdspec] Enable "browsingContext.traverseHistory" tests for SHIP on Android.

Comment 11

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

Hi Kaya, do you have an update for us why this test is still failing accessing the history info with SHIP enabled? Just apply my patch from this bug for an updated expectation. Thanks.

Comment 12

[Kaya [:kaya]]

quick mid-way update:

I locally tested this out. The tests are passing when Fission is enabled (which auto-enables SHIP), but they are failing in the nofis-ship case. I am now trying to understand if these tests expect some specific behavior that's tied to fission's content process logic that we cannot serve in nofis-ship case. I am still not sure how these were passing some time ago when Alexandra was testing it out but started failing afterwards. I'll continue investigating this, not clearing the NI for me, I'll do it once I resolve this.

Comment 13

[Kaya [:kaya]]

:whimboo,
I can confirm locally that enabling the SHIP pref while running the wpt tests over commandline
(--setpref="fission.disableSessionHistoryInParent=false") do not actually enable the SHIP, and the code still follows up non-SHIP path. That's why the tests are not passing. When the Fission pref is enabled over commandline, the SHIP is enabled correctly and that's why it is passing in that case.
Haven't checked the main cause yet, but wondering it could be anyhow related to Bug 1919837? I'll check out the root cause of this issue.

Comment 14

[Kaya [:kaya]]

:whimboo,
I can confirm that, this is a regression caused by Bug 1919837. By reverting that the SHIP tests pass.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1919837

Comment 16

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

As Kaya mentioned on Matrix we need bug 1936989 fixed to correctly run ship with wdspec!

Comment 17

[Jens Stutte [:jstutte]]

IIUC this is "only" about testing and probably not worth an uplift to 134.

Comment 18

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

Now that the preference issue with ship builds is fixed I pushed a new try build to check if these traverse history tests are passing:
https://treeherder.mozilla.org/jobs?repo=try&revision=dbed8311fd194b0e61d32136f23cfc833d3d7454

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82926264856a
[wdspec] Enable "browsingContext.traverseHistory" tests for SHIP on Android. r=webdriver-reviewers,Sasha

Comment 20

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/82926264856a