Fix issue #604

[manuelpuyol]

Closes #604

When quickly clicking between frames, even though the fetch request is canceled, the page's HTML is mutated, setting a new src to the frame element. This change will cause the pageSnapshot to be incorrect, so when we navigate back, the turbo-frame will have an incorrect src causing a new fetch and the bug shown in the issue.

This PR updates frames to know what was their previousSrc so they can revert back in case the request is canceled.

[dhh]

Cc @seanpdoyle

[seanpdoyle]

@manuelpuyol I've cherry-picked the test coverage you've introduced in this PR to #430, and the test suite passes. That pull request introduces the concept of a FrameVisit so that we can preserve and carry forward state like this without capturing that state in piecemeal by setting and deleting many FrameController instance variables.

Having said that, I've applied the following diff to main, and the suite also passes, without any changes to the implementation:

diff --git a/src/tests/functional/frame_navigation_tests.ts b/src/tests/functional/frame_navigation_tests.ts
index 3a2fadd..ac6eb03 100644
--- a/src/tests/functional/frame_navigation_tests.ts
+++ b/src/tests/functional/frame_navigation_tests.ts
@@ -1,5 +1,12 @@
 import { test } from "@playwright/test"
-import { getFromLocalStorage, nextEventNamed, nextEventOnTarget, pathname, scrollToSelector } from "../helpers/page"
+import {
+  getFromLocalStorage,
+  nextBeat,
+  nextEventNamed,
+  nextEventOnTarget,
+  pathname,
+  scrollToSelector,
+} from "../helpers/page"
 import { assert } from "chai"
 
 test("test frame navigation with descendant link", async ({ page }) => {
@@ -97,3 +104,38 @@ test("test promoted frame navigations are cached", async ({ page }) => {
   assert.equal(await page.getAttribute("#tab-frame", "src"), null, "caches one.html without #tab-frame[src]")
   assert.equal(await page.getAttribute("#tab-frame", "complete"), null, "caches one.html without [complete]")
 })
+
+test("test canceling frame requests don't mutate the history", async ({ page }) => {
+  await page.goto("/src/tests/fixtures/tabs.html")
+
+  await page.click("#tab-2")
+
+  await nextEventOnTarget(page, "tab-frame", "turbo:frame-load")
+  await nextEventNamed(page, "turbo:load")
+
+  assert.equal(await page.textContent("#tab-content"), "Two")
+  assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+  assert.equal(await page.getAttribute("#tab-frame", "complete"), "", "sets [complete]")
+
+  // This request will be canceled
+  page.click("#tab-1")
+  await page.click("#tab-3")
+
+  await nextEventOnTarget(page, "tab-frame", "turbo:frame-load")
+  await nextEventNamed(page, "turbo:load")
+
+  assert.equal(await page.textContent("#tab-content"), "Three")
+  assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/three.html")
+
+  await page.goBack()
+  await nextEventNamed(page, "turbo:load")
+
+  assert.equal(await page.textContent("#tab-content"), "Two")
+  assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+
+  // Make sure the frame is not mutated after some time.
+  await nextBeat()
+
+  assert.equal(await page.textContent("#tab-content"), "Two")
+  assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+})

Were you able to force the tests to fail?

[manuelpuyol]

@seanpdoyle hmm maybe the test is not ideal, since I still can see the bug in main

Screen.Recording.2022-11-30.at.12.59.42.PM.mov