fix: emit load/domcontentloaded events as reported by the browser (#1…

…6861)

Instead of requiring all frames in the subtree to receive a particular
event, we rely on the browser's definition of load and DOMContentLoaded.

This changes logic in a few edge cases:
- Some browsers do not emit load event upon window.stop() at all.
- DOMContentLoaded does not wait for subframes, so they might not be
  ready when passing `{ waitUntil: 'domcontentloaded' }`.

`networkidle` preserves the old logic.

packages/playwright-core/src/server/chromium/crPage.ts

@@ -435,7 +435,6 @@ class FrameSession {
       eventsHelper.addEventListener(this._client, 'Page.frameDetached', event => this._onFrameDetached(event.frameId, event.reason)),
       eventsHelper.addEventListener(this._client, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)),
       eventsHelper.addEventListener(this._client, 'Page.frameRequestedNavigation', event => this._onFrameRequestedNavigation(event)),
-      eventsHelper.addEventListener(this._client, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
       eventsHelper.addEventListener(this._client, 'Page.javascriptDialogOpening', event => this._onDialog(event)),
       eventsHelper.addEventListener(this._client, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)),
       eventsHelper.addEventListener(this._client, 'Runtime.bindingCalled', event => this._onBindingCalled(event)),
@@ -601,12 +600,6 @@ class FrameSession {
       this._page._frameManager.frameLifecycleEvent(event.frameId, 'domcontentloaded');
   }
 
-  _onFrameStoppedLoading(frameId: string) {
-    if (this._eventBelongsToStaleFrame(frameId))
-      return;
-    this._page._frameManager.frameStoppedLoading(frameId);
-  }
-
   _handleFrameTree(frameTree: Protocol.Page.FrameTree) {
     this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null);
     this._onFrameNavigated(frameTree.frame, true);

packages/playwright-core/src/server/dispatchers/frameDispatcher.ts

@@ -48,7 +48,7 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Pa
       url: frame.url(),
       name: frame.name(),
       parentFrame: FrameDispatcher.fromNullable(scope, frame.parentFrame()),
-      loadStates: Array.from(frame._subtreeLifecycleEvents),
+      loadStates: Array.from(frame._firedLifecycleEvents),
     });
     this._frame = frame;
     this.addObjectListener(Frame.Events.AddLifecycle, lifecycleEvent => {

packages/playwright-core/src/server/frames.ts

@@ -62,6 +62,8 @@ export type GotoResult = {
 
 type ConsoleTagHandler = () => void;
 
+type RegularLifecycleEvent = Exclude<types.LifecycleEvent, 'networkidle'>;
+
 export type FunctionWithSource = (source: { context: BrowserContext, page: Page, frame: Frame}, ...args: any) => any;
 
 export type NavigationEvent = {
@@ -279,17 +281,11 @@ export class FrameManager {
     const frame = this._frames.get(frameId);
     if (frame) {
       this._removeFramesRecursively(frame);
-      // Recalculate subtree lifecycle for the whole tree - it should not be that big.
-      this._page.mainFrame()._recalculateLifecycle();
+      this._page.mainFrame()._recalculateNetworkIdle();
     }
   }
 
-  frameStoppedLoading(frameId: string) {
-    this.frameLifecycleEvent(frameId, 'domcontentloaded');
-    this.frameLifecycleEvent(frameId, 'load');
-  }
-
-  frameLifecycleEvent(frameId: string, event: types.LifecycleEvent) {
+  frameLifecycleEvent(frameId: string, event: RegularLifecycleEvent) {
     const frame = this._frames.get(frameId);
     if (frame)
       frame._onLifecycleEvent(event);
@@ -478,8 +474,8 @@ export class Frame extends SdkObject {
   };
 
   _id: string;
-  private _firedLifecycleEvents = new Set<types.LifecycleEvent>();
-  _subtreeLifecycleEvents = new Set<types.LifecycleEvent>();
+  _firedLifecycleEvents = new Set<types.LifecycleEvent>();
+  private _firedNetworkIdleSelf = false;
   _currentDocument: DocumentInfo;
   private _pendingDocument: DocumentInfo | undefined;
   readonly _page: Page;
@@ -516,7 +512,6 @@ export class Frame extends SdkObject {
       this._parentFrame._childFrames.add(this);
 
     this._firedLifecycleEvents.add('commit');
-    this._subtreeLifecycleEvents.add('commit');
     if (id !== kDummyFrameId)
       this._startNetworkIdleTimer();
   }
@@ -525,23 +520,26 @@ export class Frame extends SdkObject {
     return this._detached;
   }
 
-  _onLifecycleEvent(event: types.LifecycleEvent) {
+  _onLifecycleEvent(event: RegularLifecycleEvent) {
     if (this._firedLifecycleEvents.has(event))
       return;
     this._firedLifecycleEvents.add(event);
-    // Recalculate subtree lifecycle for the whole tree - it should not be that big.
-    this._page.mainFrame()._recalculateLifecycle();
+    this.emit(Frame.Events.AddLifecycle, event);
+    if (this === this._page.mainFrame() && this._url !== 'about:blank')
+      debugLogger.log('api', `  "${event}" event fired`);
+    this._page.mainFrame()._recalculateNetworkIdle();
   }
 
   _onClearLifecycle() {
+    for (const event of this._firedLifecycleEvents)
+      this.emit(Frame.Events.RemoveLifecycle, event);
     this._firedLifecycleEvents.clear();
-    // Recalculate subtree lifecycle for the whole tree - it should not be that big.
-    this._page.mainFrame()._recalculateLifecycle(this);
     // Keep the current navigation request if any.
     this._inflightRequests = new Set(Array.from(this._inflightRequests).filter(request => request === this._currentDocument.request));
     this._stopNetworkIdleTimer();
     if (this._inflightRequests.size === 0)
       this._startNetworkIdleTimer();
+    this._page.mainFrame()._recalculateNetworkIdle(this);
     this._onLifecycleEvent('commit');
   }
 
@@ -599,37 +597,26 @@ export class Frame extends SdkObject {
     });
   }
 
-  _recalculateLifecycle(frameThatAllowsRemovingLifecycleEvents?: Frame) {
-    const events = new Set<types.LifecycleEvent>(this._firedLifecycleEvents);
+  _recalculateNetworkIdle(frameThatAllowsRemovingNetworkIdle?: Frame) {
+    let isNetworkIdle = this._firedNetworkIdleSelf;
     for (const child of this._childFrames) {
-      child._recalculateLifecycle(frameThatAllowsRemovingLifecycleEvents);
-      // We require a particular lifecycle event to be fired in the whole
-      // frame subtree, and then consider it done.
-      for (const event of events) {
-        if (!child._subtreeLifecycleEvents.has(event))
-          events.delete(event);
-      }
+      child._recalculateNetworkIdle(frameThatAllowsRemovingNetworkIdle);
+      // We require networkidle event to be fired in the whole frame subtree, and then consider it done.
+      if (!child._firedLifecycleEvents.has('networkidle'))
+        isNetworkIdle = false;
     }
-    if (frameThatAllowsRemovingLifecycleEvents !== this) {
-      // Usually, lifecycle events are fired once and not removed after that, so we keep existing ones.
-      // However, when we clear them right before a new commit, this is allowed for a particular frame.
-      for (const event of this._subtreeLifecycleEvents)
-        events.add(event);
-    }
-    const mainFrame = this._page.mainFrame();
-    for (const event of events) {
-      // Checking whether we have already notified about this event.
-      if (!this._subtreeLifecycleEvents.has(event)) {
-        this.emit(Frame.Events.AddLifecycle, event);
-        if (this === mainFrame && this._url !== 'about:blank')
-          debugLogger.log('api', `  "${event}" event fired`);
-      }
+    if (isNetworkIdle && !this._firedLifecycleEvents.has('networkidle')) {
+      this._firedLifecycleEvents.add('networkidle');
+      this.emit(Frame.Events.AddLifecycle, 'networkidle');
+      if (this === this._page.mainFrame() && this._url !== 'about:blank')
+        debugLogger.log('api', `  "networkidle" event fired`);
     }
-    for (const event of this._subtreeLifecycleEvents) {
-      if (!events.has(event))
-        this.emit(Frame.Events.RemoveLifecycle, event);
+    if (frameThatAllowsRemovingNetworkIdle !== this && this._firedLifecycleEvents.has('networkidle') && !isNetworkIdle) {
+      // Usually, networkidle is fired once and not removed after that.
+      // However, when we clear them right before a new commit, this is allowed for a particular frame.
+      this._firedLifecycleEvents.delete('networkidle');
+      this.emit(Frame.Events.RemoveLifecycle, 'networkidle');
     }
-    this._subtreeLifecycleEvents = events;
   }
 
   async raceNavigationAction(progress: Progress, options: types.GotoOptions, action: () => Promise<network.Response | null>): Promise<network.Response | null> {
@@ -706,7 +693,7 @@ export class Frame extends SdkObject {
       event = await sameDocument.promise;
     }
 
-    if (!this._subtreeLifecycleEvents.has(waitUntil))
+    if (!this._firedLifecycleEvents.has(waitUntil))
       await helper.waitForEvent(progress, this, Frame.Events.AddLifecycle, (e: types.LifecycleEvent) => e === waitUntil).promise;
 
     const request = event.newDocument ? event.newDocument.request : undefined;
@@ -731,7 +718,7 @@ export class Frame extends SdkObject {
     if (navigationEvent.error)
       throw navigationEvent.error;
 
-    if (!this._subtreeLifecycleEvents.has(waitUntil))
+    if (!this._firedLifecycleEvents.has(waitUntil))
       await helper.waitForEvent(progress, this, Frame.Events.AddLifecycle, (e: types.LifecycleEvent) => e === waitUntil).promise;
 
     const request = navigationEvent.newDocument ? navigationEvent.newDocument.request : undefined;
@@ -740,7 +727,7 @@ export class Frame extends SdkObject {
 
   async _waitForLoadState(progress: Progress, state: types.LifecycleEvent): Promise<void> {
     const waitUntil = verifyLifecycle('state', state);
-    if (!this._subtreeLifecycleEvents.has(waitUntil))
+    if (!this._firedLifecycleEvents.has(waitUntil))
       await helper.waitForEvent(progress, this, Frame.Events.AddLifecycle, (e: types.LifecycleEvent) => e === waitUntil).promise;
   }
 
@@ -1629,7 +1616,10 @@ export class Frame extends SdkObject {
     // after the frame was detached - probably a race in the Firefox itself.
     if (this._firedLifecycleEvents.has('networkidle') || this._detached)
       return;
-    this._networkIdleTimer = setTimeout(() => this._onLifecycleEvent('networkidle'), 500);
+    this._networkIdleTimer = setTimeout(() => {
+      this._firedNetworkIdleSelf = true;
+      this._page.mainFrame()._recalculateNetworkIdle();
+    }, 500);
   }
 
   _stopNetworkIdleTimer() {

packages/playwright-core/src/server/webkit/wkPage.ts

@@ -380,9 +380,8 @@ export class WKPage implements PageDelegate {
       eventsHelper.addEventListener(this._session, 'Page.willCheckNavigationPolicy', event => this._onWillCheckNavigationPolicy(event.frameId)),
       eventsHelper.addEventListener(this._session, 'Page.didCheckNavigationPolicy', event => this._onDidCheckNavigationPolicy(event.frameId, event.cancel)),
       eventsHelper.addEventListener(this._session, 'Page.frameScheduledNavigation', event => this._onFrameScheduledNavigation(event.frameId)),
-      eventsHelper.addEventListener(this._session, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
-      eventsHelper.addEventListener(this._session, 'Page.loadEventFired', event => this._onLifecycleEvent(event.frameId, 'load')),
-      eventsHelper.addEventListener(this._session, 'Page.domContentEventFired', event => this._onLifecycleEvent(event.frameId, 'domcontentloaded')),
+      eventsHelper.addEventListener(this._session, 'Page.loadEventFired', event => this._page._frameManager.frameLifecycleEvent(event.frameId, 'load')),
+      eventsHelper.addEventListener(this._session, 'Page.domContentEventFired', event => this._page._frameManager.frameLifecycleEvent(event.frameId, 'domcontentloaded')),
       eventsHelper.addEventListener(this._session, 'Runtime.executionContextCreated', event => this._onExecutionContextCreated(event.context)),
       eventsHelper.addEventListener(this._session, 'Runtime.bindingCalled', event => this._onBindingCalled(event.contextId, event.argument)),
       eventsHelper.addEventListener(this._session, 'Console.messageAdded', event => this._onConsoleMessage(event)),
@@ -450,14 +449,6 @@ export class WKPage implements PageDelegate {
     this._page._frameManager.frameRequestedNavigation(frameId);
   }
 
-  private _onFrameStoppedLoading(frameId: string) {
-    this._page._frameManager.frameStoppedLoading(frameId);
-  }
-
-  private _onLifecycleEvent(frameId: string, event: types.LifecycleEvent) {
-    this._page._frameManager.frameLifecycleEvent(frameId, event);
-  }
-
   private _handleFrameTree(frameTree: Protocol.Page.FrameResourceTree) {
     this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null);
     this._onFrameNavigated(frameTree.frame, true);

tests/library/ignorehttpserrors.spec.ts

@@ -53,7 +53,7 @@ it('should work with mixed content', async ({ browser, server, httpsServer }) =>
   });
   const context = await browser.newContext({ ignoreHTTPSErrors: true });
   const page = await context.newPage();
-  await page.goto(httpsServer.PREFIX + '/mixedcontent.html', { waitUntil: 'domcontentloaded' });
+  await page.goto(httpsServer.PREFIX + '/mixedcontent.html', { waitUntil: 'load' });
   expect(page.frames().length).toBe(2);
   // Make sure blocked iframe has functional execution context
   // @see https://github.com/GoogleChrome/puppeteer/issues/2709

tests/page/page-goto.spec.ts

@@ -629,9 +629,20 @@ it('should properly wait for load', async ({ page, server, browserName }) => {
   ]);
 });
 
-it('should properly report window.stop()', async ({ page, server }) => {
-  server.setRoute('/module.js', async (req, res) => void 0);
-  await page.goto(server.PREFIX + '/window-stop.html');
+it('should not resolve goto upon window.stop()', async ({ browserName, page, server }) => {
+  let response;
+  server.setRoute('/module.js', (req, res) => {
+    res.writeHead(200, { 'Content-Type': 'text/javascript' });
+    response = res;
+  });
+  let done = false;
+  page.goto(server.PREFIX + '/window-stop.html').then(() => done = true).catch(() => {});
+  await server.waitForRequest('/module.js');
+  expect(done).toBe(false);
+  await page.waitForTimeout(1000);  // give it some time to erroneously resolve
+  response.end('');
+  await page.waitForTimeout(1000);  // give it more time to erroneously resolve
+  expect(done).toBe(browserName === 'firefox'); // Firefox fires DOMContentLoaded and load events in this case.
 });
 
 it('should return from goto if new navigation is started', async ({ page, server, browserName, isAndroid }) => {

tests/page/page-wait-for-navigation.spec.ts

@@ -155,18 +155,18 @@ it('should work with DOM history.back()/history.forward()', async ({ page, serve
   expect(page.url()).toBe(server.PREFIX + '/second.html');
 });
 
-it('should work when subframe issues window.stop()', async ({ page, server }) => {
+it('should work when subframe issues window.stop()', async ({ browserName, page, server }) => {
   server.setRoute('/frames/style.css', (req, res) => {});
-  const navigationPromise = page.goto(server.PREFIX + '/frames/one-frame.html');
+  let done = false;
+  page.goto(server.PREFIX + '/frames/one-frame.html').then(() => done = true).catch(() => {});
   const frame = await new Promise<Frame>(f => page.once('frameattached', f));
   await new Promise<void>(fulfill => page.on('framenavigated', f => {
     if (f === frame)
       fulfill();
   }));
-  await Promise.all([
-    frame.evaluate(() => window.stop()),
-    navigationPromise
-  ]);
+  await frame.evaluate(() => window.stop());
+  await page.waitForTimeout(2000);  // give it some time to erroneously resolve
+  expect(done).toBe(browserName !== 'webkit');  // Chromium and Firefox issue load event in this case.
 });
 
 it('should work with url match', async ({ page, server }) => {