1786639 - Failure in Andriod Fission builds: /webdriver/tests/element_click/navigate.py | test_link_unload_event - assert False is True

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: istorozhko [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=387807093&repo=try
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/C5aVgiY5R7uEiGW345suqg/runs/0/artifacts/public/logs/live_backing.log

---

Here is the link to the try push: https://treeherder.mozilla.org/jobs?repo=try&revision=912cffe26463c17323a7b00ac5c07ceff88409a7

This bug blocks Fission on Android. The tests were run on Fission with `isolateNothing` strategy (so when debugging locally or making new try pushes, please add `pref("fission.webContentIsolationStrategy", 0);` line to the `geckoview-prefs.js`)

Comment 1

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

In this test a click on a link happens which at the same time does not only navigate but also should set a checkbox's state to checked before. After navigating back we expect the checkbox to be still checked, which is not the case here. We haven't seen this problem on desktop yet. As such I wonder if this might be an underlying issue with the Fission implementation in GeckoView? I will have to take a closer look at this next week.

Comment 2

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

Here a link to the test:
https://searchfox.org/mozilla-central/rev/f36f074acc09f2dab0e9ffaba34b5f6714dec81d/testing/web-platform/tests/webdriver/tests/element_click/navigate.py#44

Olli, maybe you have an idea why the checked state on the original page is not kept after navigating back to it on Android with Fission enabled? If not is there any kind of extra logging that we could enable for Marionette?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

bfcache handling is different on Android.
https://searchfox.org/mozilla-central/rev/fbeff4caf4fc319d973d6c1d006f0f480a3aba95/modules/libpref/init/StaticPrefList.yaml#2034-2038
That could affect the testcase here, assuming I'm interpreting the bug summary correctly.
Is it possible that the issue is that checked state is kept even after navigating, not that it is not kept?

Comment 4

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #3)

bfcache handling is different on Android.
https://searchfox.org/mozilla-central/rev/fbeff4caf4fc319d973d6c1d006f0f480a3aba95/modules/libpref/init/StaticPrefList.yaml#2034-2038

Setting this preference to false for GeckoView with Fission enabled the test indeed unexpectedly passes.

That could affect the testcase here, assuming I'm interpreting the bug summary correctly.
Is it possible that the issue is that checked state is kept even after navigating, not that it is not kept?

Its rather the other way around. In the test we expect that the clicked state is kept when navigation to a different page and then going back. But that's not happening with the default value of the preference. So why should non-Fission and Fission show different behavior?

Comment 5

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

I just read bug 1682394. So it would become a new feature and for now is only enabled on Android. Does it mean that we may have to change the test so it's not getting put into bfcache? That probably would fix it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

"unexpectedly passes" ? I'm now a bit lost. Is the test marked as failing on Android?

I don't think unload handler even runs on mobile (when unload listener doesn't block bfcache), so I'd expect checked state be false.

Comment 7

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #6)

"unexpectedly passes" ? I'm now a bit lost. Is the test marked as failing on Android?

Yes, because right now the test is marked as failing for Android Fission. And with the pref value changed the test passes.

I don't think unload handler even runs on mobile (when unload listener doesn't block bfcache), so I'd expect checked state be false.

As mentioned above it works fine with Fission disabled.

So here the states:

• Fission disabled: pass
• Fission enabled: fail
• Fission enabled and preference set to false: pass

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And what do we expect? Does "Fission disabled: pass" mean the test fails, because we expect it to fail?

Comment 9

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

Well, the question is if this is an expected difference between desktop and mobile, and if it will stay that way. We cannot simply change the test because it fails on Android with Fission enabled. If the test is not valid (based on the changes from bug 1682394) we could only remove it from the test suite. Does the HTML spec handle that case?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

But what does "fails on Android with Fission enabled" mean if the test has been marked as failing in general on Android?

On mobile unload event listener don't block bfcache. That is these days the case on all the mobile browsers.
So the testcase will likely work differently on mobile vs desktop.

There seems to be difference in behavior on Fission vs non-fission and unload event listeners.
non-fission doesn't block bfcache only if we're doing cross-origin load
https://searchfox.org/mozilla-central/rev/e94c6cb9649bfe4e6a3888460f41bcd4fe30a6ca/dom/base/Document.cpp#11090-11092
but that is a case we'd like to get rid of and I think other browsers already did.

Comment 11

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #10)

But what does "fails on Android with Fission enabled" mean if the test has been marked as failing in general on Android?

It was marked as failing when the tests got enabled for GeckoView with Fission enabled so that the tests can be run without having a perma failure. For me it's unclear if that is an expected fail, if the test is wrong and we might have to change it to let it pass for both desktop and mobile, or something else.

There seems to be difference in behavior on Fission vs non-fission and unload event listeners.
non-fission doesn't block bfcache only if we're doing cross-origin load
https://searchfox.org/mozilla-central/rev/e94c6cb9649bfe4e6a3888460f41bcd4fe30a6ca/dom/base/Document.cpp#11090-11092
but that is a case we'd like to get rid of and I think other browsers already did.

Sorry but I don't understand what "blocking bfcache" actually means. So how should we change the test to make it future-proof and compatible to a feature that we and other vendors have / want to implement? In case of webdriver we want to make sure that clicking a link will wait long enough for the unload handler to be handled.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

It is not really defined anywhere whether unload event listener prevents use of bfcache or not. In practice on mobile web browsers it doesn't prevent the page to enter bfcache and on desktop Chrome and Firefox it does. I believe desktop Safari behaves similarly to the mobile version.

Can you use pagehide event listener and check its .persisted property? That would tell whether the page is about to enter bfcache. Or use .persisted of pageshow event, that tells whether the just loaded page came out from bfcache.

Comment 13

[BMO Automation]

Moving bug to Remote Protocol::Marionette component per bug 1815831.

Comment 14

[Intermittent Failures Robot]

3 failures in 3761 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 3

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 3
  • debug: 1
  • opt: 1
  • debug-isolated-process: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2023-03-13&endday=2023-03-19&tree=all

Comment 15

[Intermittent Failures Robot]

3 failures in 3206 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 3

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 3
  • debug-isolated-process: 1
  • debug: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2023-04-10&endday=2023-04-16&tree=all

Comment 16

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #12)

It is not really defined anywhere whether unload event listener prevents use of bfcache or not. In practice on mobile web browsers it doesn't prevent the page to enter bfcache and on desktop Chrome and Firefox it does. I believe desktop Safari behaves similarly to the mobile version.

So I just had another look at this test given that it perma fails for me now locally. So the problem here is that we want to store a flag once we noticed that the unload handler has been run. Right now we set the checked state of the input element to true. Given that the click on the element navigated to a different page we traverse back the history by 1 item to reach the former page. Here we do a check now if the input element is checked.

Can you use pagehide event listener and check its .persisted property? That would tell whether the page is about to enter bfcache. Or use .persisted of pageshow event, that tells whether the just loaded page came out from bfcache.

This might be a workaround to run this above check or not. But maybe there is another option to set some state that would survive? Maybe we have to run this test inside a frame and post a message to the top-level browsing context? That way we would know as well that the page navigated and we do not have to run this specific check. Olli, what do you think? Is that fine or maybe something simpler exist?

Comment 17

[Intermittent Failures Robot]

1 failures in 3880 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2023-06-19&endday=2023-06-25&tree=all

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

But using iframes would test different thing. Do we want to test bfcache here or not?
(Navigations in iframes won't enter bfcache)

Comment 19

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #18)

But using iframes would test different thing. Do we want to test bfcache here or not?
(Navigations in iframes won't enter bfcache)

Given the name of the test I would say that we do not test bfcache here, but that a navigation takes place when the anchor is clicked and that a previously registered event handler by WebDriver for unload is handled. Given that WebDriver classic cannot send an event to the client we need to store a flag somewhere that can be read out afterward. With the postMessage approach we could actually send it up to the top-level window, and read it out afterward.

Comment 20

[Intermittent Failures Robot]

1 failures in 3076 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2023-08-14&endday=2023-08-20&tree=all

Comment 21

[Intermittent Failures Robot]

1 failures in 3217 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2024-01-08&endday=2024-01-14&tree=all

Comment 22

[Intermittent Failures Robot]

1 failures in 3235 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform and build breakdown:

• android-em-7-0-x86_64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1786639&startday=2024-04-08&endday=2024-04-14&tree=all

Comment 23

[[:owlish] 🦉 PST]

Hi Henrik, can we bump the priority of this? It blocks Android Fission

Comment 24

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #19)

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #18)

But using iframes would test different thing. Do we want to test bfcache here or not?
(Navigations in iframes won't enter bfcache)

Given the name of the test I would say that we do not test bfcache here, but that a navigation takes place when the anchor is clicked and that a previously registered event handler by WebDriver for unload is handled. Given that WebDriver classic cannot send an event to the client we need to store a flag somewhere that can be read out afterward. With the postMessage approach we could actually send it up to the top-level window, and read it out afterward.

If we just want to check that unload event handlers are triggered when navigating via a link in webdriver (which honestly feels more like a generic webplatform test rather than webdriver?), could we just write to localStorage and check this after navigating?

Comment 25

[Julian Descottes [:jdescottes]]

After trying, localStorage on unload doesn't seem to work on geckoview either, while it works fine on desktop. Same for document.cookie.

If part of the issue is that unload isn't reliably fired on mobile (seems to be the case according to MDN?), maybe as suggested we should switch to pagehide? That seems to work fine, either with the current test checking a checkbox, or with something using cookies or localstorage.

Comment 26

[Julian Descottes [:jdescottes]]

On a sidenote, the metadata for this test seems incorrect at the moment: https://searchfox.org/mozilla-central/rev/f6e3b81aac49e602f06c204f9278da30993cdc8a/testing/web-platform/meta/webdriver/tests/classic/element_click/navigate.py.ini

[navigate.py]

  [test_link_unload_event]
    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1786639

The intent was probably to have

[navigate.py]

  [test_link_unload_event]
    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1786639
    expected:
      if os == "android" and fission: FAIL

as it used to be previously. Alternatively we could also restore the metadata.

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: Bug 1786639 - [wdspec] Use pagehide instead of unload for webdriver navigate test

Depends on D208638

Comment 28

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

Lets take the approach with pagehide which Julian attached 5 days ago.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed2f92b696b8
[wdspec] Use pagehide instead of unload for webdriver navigate test r=webdriver-reviewers,whimboo

Comment 30

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

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

Comment 31

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ed2f92b696b8

Comment 32

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

Upstream PR merged by moz-wptsync-bot