1879163 - browsingContext.navigate resolves too early if beforeunload does a same document navigation

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

If the current page has a beforeunload listener which does a samedocument navigation such as:

        window.addEventListener(
          'beforeunload',
          () => {
            return history.replaceState(null, 'initial', window.location.href);
          },
          false
        );

Using browsingContext.navigate with wait="complete" will resolve as soon as the same-document navigation is done (ie almost immediately) and will not wait for the actual navigation to be done.

This makes at least one puppeteer test fail: navigation Page.goto should work when page calls history API in beforeunload

We can reproduce the same thing with a wdspec test:

async def test_same_document_navigation_in_before_unload(bidi_session, new_tab, url):
    url_before = url(
        "/webdriver/tests/bidi/browsing_context/support/empty.html"
    )

    await navigate_and_assert(bidi_session, new_tab, url_before, "complete")

    await bidi_session.script.evaluate(
        expression="""window.addEventListener(
          'beforeunload',
          () => history.replaceState(null, 'initial', window.location.href),
          false
        );""",
        target=ContextTarget(new_tab["context"]),
        await_promise=False)


    url_after = url_before.replace("empty.html", "other.html")
    await navigate_and_assert(bidi_session, new_tab, url_after, "complete")

Comment 1

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

Alex also added a new test covering more events. It will be synced with bug 1880290.

Comment 2

[Julian Descottes [:jdescottes]]

Check with Alex if this is a priority or an edge case.

Comment 3

[Julian Descottes [:jdescottes]]

Alex: this issue corresponds to the test you added in https://github.com/web-platform-tests/wpt/pull/44587
Should this be considered a blocker for puppeteer support or is it rather an edge case?

Comment 4

[alexrudenko]

It's probably more of an edge case but it would break clients like Puppeteer. The usage numbers for beforeunload and history push are relativelz high (https://chromestatus.com/metrics/feature/timeline/popularity/200, https://chromestatus.com/metrics/feature/timeline/popularity/2617) but I am not sure how often they are used together. It is also possible that the issue can be triggered via other APIs.

Comment 5

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

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

[Julian Descottes [:jdescottes]]

Let's set 3 points, but if it proves to be harder we can adjust.

Comment 7

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

The problem here is that we currently have a bogus handling of location change updates with the LOCATION_CHANGE_SAME_DOCUMENT flag set. Here we fold everything into including the hash only changes. The latter can actually be handled with the LOCATION_CHANGE_HASHCHANGE flag and won't see a state change notification. Further when the URL including the hash are identical there is no state change notification as well and receiving the location change notification with LOCATION_CHANGE_SAME_DOCUMENT should stop waiting. Only for other navigations even with the same URL we cannot immediately stop waiting but have to wait for the start and stop state change notifications.

Comment 8

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

In addition to the previously mentioned behavior, I noticed that we are emitting a browsingContext.fragmentNavigated event for the previous page just before the navigation to the final page begins. This causes an early abort in Puppeteer, preventing it from waiting for the actual network events and resulting in response.status() not being retrievable since no network event has been received yet. This issue arises because the NavigationListener in the content process informs us about this unrelated location change before the actual navigation starts:

 0:04.66 pid:9152 1719901421211	RemoteAgent	TRACE	[13] ProgressListener Start: expectNavigation=true resolveWhenStarted=false unloadTimeout=200 waitForExplicitStart=true
 0:04.66 pid:9152 1719901421211	RemoteAgent	TRACE	[13] ProgressListener Skip setting the unload timer
 0:04.66 pid:9152 1719901421211	RemoteAgent	TRACE	[13] NavigationListener onLocationChange, location: http://localhost:59379/grid.html
 0:04.66 pid:9152 1719901421211	RemoteAgent	TRACE	[13] NavigationListener onStateChange, stateFlags: 983041, status: 0, isStart: true, isStop: false, isNetwork: true, isBindingAborted: false, targetURI: http://localhost:59379/empty.html
 0:04.66 pid:9152 1719901421211	RemoteAgent	DEBUG	WebDriverBiDiConnection 9e517fb0-d64f-41c4-a1e8-fc86358138ea <- {"type":"event","method":"browsingContext.fragmentNavigated","params":{"context":"6491bd28-c685-4eff-9e94-844a6a3960a1","navigation":"fd8067ae-0cbb-4834-83d4-491d13bb2a48","timestamp":1719901421211,"url":"http://localhost:59379/grid.html"}}
 0:04.66 pid:9152 [Parent 9318: Main Thread]: I/BCWebProgress OnLocationChange({isTopLevel:1, isLoadingDocument:0}, <null>, http://localhost:59379/grid.html, LOCATION_CHANGE_SAME_DOCUMENT) on {top:1, id:d, url:http://localhost:59379/grid.html}
 0:04.66 pid:9152 1719901421213	RemoteAgent	TRACE	[6491bd28-c685-4eff-9e94-844a6a3960a1] Navigation started for url: http://localhost:59379/empty.html (c18a1e9f-077d-46e0-99b6-6b6e2120b1fc)
 0:04.66 pid:9152 1719901421213	RemoteAgent	DEBUG	WebDriverBiDiConnection 9e517fb0-d64f-41c4-a1e8-fc86358138ea <- {"type":"event","method":"browsingContext.navigationStarted","params":{"context":"6491bd28-c685-4eff-9e94-844a6a3960a1","navigation":"c18a1e9f-077d-46e0-99b6-6b6e2120b1fc","timestamp":1719901421213,"url":"http://localhost:59379/empty.html"}}

Preventing this extra event allows the Puppeteer test to pass. I assume that we should not forward any event when there is no active navigation. Also I need to check why we need to use the listener in content at all and cannot just rely on the parent process. But maybe that's a different bug.

Comment 9

[Julian Descottes [:jdescottes]]

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

Also I need to check why we need to use the listener in content at all and cannot just rely on the parent process. But maybe that's a different bug.

We can try to revisit, but back when I implemented the navigation manager, the parent process progress listener was missing navigation updates in several edge cases.

(eg see https://bugzilla.mozilla.org/show_bug.cgi?id=1756595#c4)

So I had trouble deciding between a parent process approach and a content process one. But also this work happened when I had to take a break last year, so it's possible I didn't investigate this properly :(

And maybe the better option is to monitor things in the parent process and fix the missing notifications case by case.

Comment 10

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

Attachment: Bug 1879163 - [puppeteer] Disable navigation tests for history API usage.

Comment 11

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

Attachment: Bug 1879163 - [wdspec] Add test for navigation in beforeunload.

Comment 12

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

Attachment: Bug 1879163 - [wdspec] Fix broken reference to empty.html and naming of fragment navigated event.

Comment 13

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

Attachment: Bug 1879163 - [remote] Fix handling of navigations for hash changed notifications.

Comment 14

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

Attachment: Bug 1879163 - [remote] Unify trace() logging for NavigationListenerChild.

Comment 15

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

Attachment: Bug 1879163 - [remote] Split "location-change" notification into "hash-changed" and "same-document-changed.

Comment 16

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

As discussed with Julian we are going to bump the points due to amount of work necessary to fix this bug. Patches are upcoming soon.

Comment 17

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

(In reply to Julian Descottes [:jdescottes] from comment #9)

So I had trouble deciding between a parent process approach and a content process one. But also this work happened when I had to take a break last year, so it's possible I didn't investigate this properly :(

And maybe the better option is to monitor things in the parent process and fix the missing notifications case by case.

I would wait with such a refactoring until we have the history API behavior correctly spec'ed and implemented. By that time it would make it easier to do such a refactoring without missing critical pieces.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/493f78c83560
[puppeteer] Disable navigation tests for history API usage. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/5cb2f2b82fbf
[wdspec] Add test for navigation in beforeunload. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/9f17bdbae0a9
[wdspec] Fix broken reference to empty.html and naming of fragment navigated event. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/9649a4cbfb8d
[remote] Fix handling of navigations for hash changed notifications. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e2581a312e7b
[remote] Unify trace() logging for NavigationListenerChild. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/b163e0e62e23
[remote] Split "location-change" notification into "hash-changed" and "same-document-changed. r=webdriver-reviewers,jdescottes

Comment 19

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

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

Comment 20

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

The upstream PR for the test expectation updates is at https://github.com/puppeteer/puppeteer/pull/12704.

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/493f78c83560
https://hg.mozilla.org/mozilla-central/rev/5cb2f2b82fbf
https://hg.mozilla.org/mozilla-central/rev/9f17bdbae0a9
https://hg.mozilla.org/mozilla-central/rev/9649a4cbfb8d
https://hg.mozilla.org/mozilla-central/rev/e2581a312e7b
https://hg.mozilla.org/mozilla-central/rev/b163e0e62e23

Comment 22

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

Upstream PR merged by moz-wptsync-bot