Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] When clicking on a link navigating away from the website, playwright throws a removeFrameSession failure #1762

Closed
cnishina opened this issue Apr 13, 2020 · 7 comments · Fixed by #1767
Assignees

Comments

@cnishina
Copy link
Contributor

Context:

  • Playwright Version: 0.13.0
  • Operating System: Mac

Code Snippet

it('should work', async () => {
  console.log('goto');
  await page.goto(
    'https://journal.cnishina.dev/2020/04/create-mp4-from-screencast.html');
  await new Promise((resolve) => {
    setTimeout(resolve, 500);
  });
  console.log('click journal');
  await page.click('a[href="https://journal.cnishina.dev/"]');
  console.log('youtube');
  await page.click('a[href="https://youtu.be/dQw4w9WgXcQ"]');
});

Describe the bug

As a person writing a test, if I click on a link that navigates me away from the page, then it should not throw an error. I noticed this when I was clicking the a[href="https://youtu.be/dQw4w9WgXcQ"]. Clicking on this throws an error. I am either looking for documentation explaining targetId and sessionId and why this does not work when this happens (what the workflow should be if we decide to click on a link navigating us away from the main page) or to fix it with:

removeFrameSession(targetId) {
    const frameSession = this._sessions.get(targetId);
    if (!frameSession)
        return;
    // Frame id equals target id.
    const frame = this._page._frameManager.frame(targetId);
    if (frame) {
        this._page._frameManager.removeChildFramesRecursively(frame);
        frameSession.dispose();
        this._sessions.delete(targetId);
    }
}

This just changes the assert(frame); and puts it into a conditional. I've tried out this change and it appears to work but might not be the right fix. Let me know and I can put in a PR this week. Thanks!

@cnishina
Copy link
Contributor Author

I have more info here: https://journal.cnishina.dev/2020/04/playwright-removeframesession.html on what I've tried, etc.

@arjunattam
Copy link
Contributor

Thanks for reporting this @cnishina. Can you share some more context on how you were setting up the browser and page? Were you using jest-playwright, or something similar? I tried doing this on plain Playwright in Node (v13.12 – code snippet below), and did not run into any errors. Trying to find out what I'm missing.

Code snippet
const { chromium } = require('playwright');

(async () => {
  const browser = await chromium.launch();
  const page = await browser.newPage();

  await page.goto(
    'https://journal.cnishina.dev/2020/04/create-mp4-from-screencast.html');
  await new Promise((resolve) => {
    setTimeout(resolve, 500);
  });
  console.log('click journal');
  await page.click('a[href="https://journal.cnishina.dev/"]');
  console.log('youtube');
  await page.click('a[href="https://youtu.be/dQw4w9WgXcQ"]');

  await browser.close();
})();

@pavelfeldman
Copy link
Member

Looks like a OOPIF error, @dgozman, PTAL!

@cnishina
Copy link
Contributor Author

Here's my sample code. This runs with npm test.

https:/cnishina/protractor-experiments/tree/master/playwright-issue-1762

cnishina added a commit to cnishina/playwright that referenced this issue Apr 13, 2020
When navigating away from the page loaded in `goto`, the frame no longer
exists. Page.removeFrameSession is called and throws an error. Instead
of calling the helper.assert method, wrapping the
removeChildFramesRecursively, dispose, and delete methods in a
conditional appears to resolve this issue.

closes microsoft#1762
@cnishina
Copy link
Contributor Author

I have the following: cnishina@778f4ba

Let me know if this is a reasonable PR or if there's something else going on.

@dgozman
Copy link
Contributor

dgozman commented Apr 13, 2020

I can reproduce this locally, but cannot write a test that reproduces this issue in our environment. Most likely, there is a race between renderer-issued frameDetached and browser-issued detatchFromTarget.

@cnishina Your commit is indeed a good solution. The only problem is that the following lines should be executed even when frame is undefined:

frameSession.dispose();
this._sessions.delete(targetId);

If you could update your change and send a PR - that would be wonderful!

@cnishina
Copy link
Contributor Author

Sounds good. I'll have something in by the end of the day...

cnishina added a commit to cnishina/playwright that referenced this issue Apr 13, 2020
When navigating away from the page loaded in `goto`, the frame no longer
exists. Page.removeFrameSession is called and throws an error. Instead
of calling the helper.assert method, moving removeChildFramesRecursively
into a conditional.

closes microsoft#1762
cnishina added a commit to cnishina/playwright that referenced this issue Apr 13, 2020
When navigating away from the page loaded in `goto`, the frame no longer
exists. Page.removeFrameSession is called and throws an error. Instead
of calling the helper.assert method, moved removeChildFramesRecursively
into a conditional.

closes microsoft#1762
cnishina added a commit to cnishina/playwright that referenced this issue Apr 13, 2020
When navigating away from the page loaded in `goto`, the frame no longer
exists. Page.removeFrameSession is called and throws an error. Instead
of calling the helper.assert method, moved removeChildFramesRecursively
into a conditional.

closes microsoft#1762
dgozman pushed a commit that referenced this issue Apr 15, 2020
…g from oopif (#1767)

When navigating away from the page loaded in `goto`, the frame no longer
exists. Page.removeFrameSession is called and throws an error. Instead
of calling the helper.assert method, moved removeChildFramesRecursively
into a conditional.

closes #1762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants