[Bug]: BFCache behavior breaks exposeFunction

DerGernTod commented 3 months ago

Version

1.45.0

Steps to reproduce

take a look at this puppet issue: https://github.com/puppeteer/puppeteer/issues/12662 playwright has the exact same issue

Expected behavior

execution of exposed function works properly after bfcache restore

Actual behavior

execution of exposed function throws an exception after bfcache restore

Additional context

No response

Environment

System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 11th Gen Intel(R) Core(TM) i9-11950H @ 2.60GHz
    Memory: 37.20 GB / 63.20 GB
  Binaries:
    Node: 20.11.0 - c:\workspaces\devtools\nodejs\node.EXE
    npm: 10.2.4 - c:\workspaces\devtools\nodejs\npm.CMD
    pnpm: 8.14.2 - c:\workspaces\devtools\nodejs\pnpm.CMD
  npmPackages:
    @playwright/test: 1.45.0 => 1.45.0

Smrtnyk commented 3 months ago

Puppeteer marked it as an upstream chromium issue, but was able to workaround it on the puppeteer side. I wonder if playwright is able to do the same, since we are using playwright as a testing software and this blocks us from testing some important scenarios.

mxschmitt commented 3 months ago

Would it be possible to provide a repro? I tried it with the following, and was not able to reproduce:

diff --git a/tests/assets/cached/bfcache/index.html b/tests/assets/cached/bfcache/index.html
new file mode 100644
index 000000000..abb1337c7
--- /dev/null
+++ b/tests/assets/cached/bfcache/index.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<body>BFCached<a href="target.html">next</a></body>
\ No newline at end of file
diff --git a/tests/assets/cached/bfcache/target.html b/tests/assets/cached/bfcache/target.html
new file mode 100644
index 000000000..9586111c3
--- /dev/null
+++ b/tests/assets/cached/bfcache/target.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<body>target</body>
\ No newline at end of file
diff --git a/tests/page/page-expose-function.spec.ts b/tests/page/page-expose-function.spec.ts
index ece4a602d..dbc2ce828 100644
--- a/tests/page/page-expose-function.spec.ts
+++ b/tests/page/page-expose-function.spec.ts
@@ -43,6 +43,18 @@ it('should work', async ({ page, server }) => {
   expect(result).toBe(36);
 });

+it('should work over BFCache', async ({ page, server }) => {
+  await page.goto(server.PREFIX + '/cached/bfcache/index.html');
+  await page.exposeFunction('compute', (a: number, b: number) => a * b);
+  expect(await page.evaluate(() => window['compute'](9, 4))).toBe(36);
+  await page.getByRole('link').click();
+  await expect(page).toHaveURL(server.PREFIX + '/cached/bfcache/target.html');
+  expect(await page.evaluate(() => window['compute'](9, 4))).toBe(36);
+  await page.goBack();
+  await expect(page).toHaveURL(server.PREFIX + '/cached/bfcache/index.html');
+  expect(await page.evaluate(() => window['compute'](9, 4))).toBe(36);
+});
+
 it('should work with handles and complex objects', async ({ page, server }) => {
   const fooHandle = await page.evaluateHandle(() => {
     window['fooValue'] = { bar: 2 };

Smrtnyk commented 3 months ago

are you sure bf cache restore was even triggered in your test case? you can verify it by registering an event listener for pageshow and checking persisted property on the event if it is true

For us, bf cache doesn't work with the old chromium binary, just with the new one, but exposed bindings are not restored properly

mxschmitt commented 3 months ago

Playwright has BFCache disabled as of today, so I guess this issue does not apply here?

Smrtnyk commented 3 months ago

Playwright has BFCache disabled as of today, so I guess this issue does not apply here?

well, we are enabling it via

launchOptions: {
                    args: [
                        "--auto-open-devtools-for-tabs",
                        "--window-position=1424,0",
                        "--enable-privacy-sandbox-ads-apis",
                    ],
                    ignoreDefaultArgs: [
                        "--disable-back-forward-cache",
                    ]

So it definitely applies We have some bf cache scenarios without exposed bindings, but we are not able to create more complex ones due to exposed bindings not working

pavelfeldman commented 3 months ago

Could you give us an idea on the scenario that you are testing? We think of bfcache as an optimization that stands in the way of the tester, so we disable it. It breaks a lot of the flows in the navigation pipeline, so even though it may look like it is working for you with the exception of the exposed functions, many other executioncontext-related features are broken internally.

Smrtnyk commented 3 months ago

We have lots of test pages served via tomcat in jsp pages. Those tests are executed in an async pipeline where we get results next day due to lots of tests. However we also use playwright to visit those jsp pages served via tomcat running a selection of tests (the ones we find crucial) also in our local PRs for fast feedback. Playwright is used just to visit those pages and to wait test end signal via exposed binding, all the test exec and assertion is done in the jsp pages themselves. This works just fine for a long time now, but we had now a need to write some bf cache tests for our product and this is where we came into an obstacle of getting the exception. For now we wrote bf cache tests locally in playwright.

At first we thought playwright uses puppeteer as a dependency so we filed an issue there which they fixed, but then we realised playwright is completely separate of it, thus deciding to open an issue here as well, hoping playwright could do the similar workaround puppeteer did.

microsoft / playwright