fix(navigations): remove LifecycleWatcher, fix flakes #882
Conversation
src/frames.ts
Outdated
| if (!initial) { | ||
| for (const watcher of this._lifecycleWatchers) | ||
| watcher._onCommittedNewDocumentNavigation(frame); | ||
| // TODO should the document watchers only fire if !initial? |
There was a problem hiding this comment.
This was important for webkit, otherwise we used to resolve waitForNavigation() with about:blank on cross-process navigation.
There was a problem hiding this comment.
That doesn't appear to be the case. I added a test.
JoelEinbinder
force-pushed
the
new_wait_forNavigation
branch
from
b06b851
to
6ed0a8b
Compare
| // For some reason, Firefox issues load event with one outstanding request. | ||
| const failed = page.goto(server.PREFIX + '/one-style.html', { waitUntil: FFOX ? 'networkidle0' : 'load' }).catch(e => e); | ||
| await server.waitForRequest('/one-style.css'); | ||
| it('should fail when canceled by another navigation', async({page, server}) => { |
There was a problem hiding this comment.
Previously, we would reject any navigation promises if the page navigated somewhere before the lifecycle events occurred. But that sometimes had false positives. Consider
<script>
window.location.href = 'https://www.microsoft.com';
</script>This page will redirect to microsoft.com before the 'load' event. But everything is working as intended. This patch changes the behavior to no longer reject the goto promise.
The case this test is testing for can now only be detected if the navigation is interrupted before the document commits. This has a different error message per browser, all variations on the document request being aborted.
| server.setRoute('/one-style.css', (req, res) => response = res); | ||
| const navigationPromise = page.goto(server.PREFIX + '/one-style.html'); | ||
| await server.waitForRequest('/one-style.css'); | ||
| await page.goto(server.PREFIX + '/one-style.html', {waitUntil: []}); | ||
| const error = await page.waitForLoadState({ timeout: 1 }).catch(e => e); |
There was a problem hiding this comment.
This test could fake if waitForLoadState ran before the document committed with page.goto.
| @@ -345,7 +341,10 @@ export class FrameManager { | |||
| export class Frame { | |||
| _id: string; | |||
| readonly _firedLifecycleEvents: Set<LifecycleEvent>; | |||
| _lastDocumentId: string; | |||
| _lastDocumentId = ''; | |||
| _requestWatchers = new Set<(request: network.Request) => void>(); | |||
There was a problem hiding this comment.
nit: these watcher sets can now be private
There was a problem hiding this comment.
Unfortunately they can't. Private means class private, and they are used by the frame manager.
| return {dispose, value: requestMap}; | ||
| } | ||
|
|
||
| _createFrameDestroyedPromise(): Promise<Error> { |
There was a problem hiding this comment.
I think we can create this once in constructor and call it this._detachedOrDisconnectedPromise.
There was a problem hiding this comment.
I started with that, but it made the error messages have bad stacks. I added a test for this as well :)
Chromium flaked some tests on the bots, and I ended up refactoring all of our navigation code. ptal