Iframe and pushState() brokes history.back() on first load
Reported by
a.gonzal...@gmail.com,
Dec 20 2017
|
|||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36 Steps to reproduce the problem: 1. Open https://adrigzr.github.io/issue-iframe-pushstate/ on a new tab (important new tab!). Check "History length: 1". 2. Click on "Iframe & Push" button. 3. Click on "Back" button, it should do nothing at all. 4. Open new tab again. 5. Click on "Push & Iframe" button. 6. Click on "Back" button, it goes back on history as usually :) What is the expected behavior? It should go back on history. What went wrong? window.history.back() is not working. This only happens when history.length == 1 and you pushState() after an iframe insertion. Did this work before? Yes 62 Does this work in other browsers? Yes Chrome version: 63.0.3239.84 Channel: stable OS Version: Flash Version:
,
Dec 20 2017
,
Dec 20 2017
,
Dec 20 2017
Can you paste the contents of the chrome://version page?
,
Dec 21 2017
Google Chrome 63.0.3239.84 (Official Build) (64-bit) Revision 8f51ed0e633e109109762a3deb18a50e8c138819-refs/branch-heads/3239@{#643} OS Mac OS X JavaScript V8 6.3.292.46 Flash 28.0.0.126 /Users/XXX/Library/Application Support/Google/Chrome/PepperFlash/28.0.0.126/PepperFlashPlayer.plugin User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36 Command Line /Applications/Google Chrome.app/Contents/MacOS/Google Chrome --flag-switches-begin --enable-devtools-experiments --javascript-harmony --flag-switches-end Executable Path /Applications/Google Chrome.app/Contents/MacOS/Google Chrome Profile Path /Users/XXX/Library/Application Support/Google/Chrome/Default Variations c134752e-b8b72c88 3095aa95-3f4a17df 6c43306f-ca7d8d80 7c1bc906-f55a7974 47e5d3db-3d47f4f4 1210a805-ecd831c b1edbc38-cf4f6ead d43bf3e5-bd7cd813 ba3f87da-45bda656 776de70c-eadfd437 79616653-3f4a17df f9884634-f23d1dea 9e201a2b-6e3ce1c 68812885-4d2fac87 f347910c-3d47f4f4 4b61504a-d25ea691 9773d3bd-f23d1dea 8e3b2dc5-93702590 9e5c75f1-d2a7f95e f79cb77b-3d47f4f4 4ea303a6-ecbb250e d92562a9-ca7d8d80 25fc488a-4d2fac87 1c2f7bbf-3f4a17df 1bced4a3-59ba3fef b2f0086-93053e47 ef25c1eb-ca7d8d80 494d8760-6843eff2 f47ae82a-746c2ad4 3ac60855-486e2a9c f296190c-8282a32d 4442aae2-6bdfffe7 ed1d377-e1cc0f14 75f0f0a0-d7f6b13c e2b18481-75cb33fc e7e71889-e1cc0f14 94e68624-803f8fc4 e9ce63c1-3d47f4f4 da4aaa01-ca7d8d80
,
Dec 21 2017
Unable to reproduce the issue on reported chrome version 63.0.3239.84 and on the latest dev 65.0.3298.3 using Ubuntu 14.04 with the below mentioned steps. 1. Launched Chrome 2. Navigated to https://adrigzr.github.io/issue-iframe-pushstate/ (In a new tab) 3. Checked "History length: 1 4. Clicked on "Iframe & Push" button 5. Clicked on "Back" button and opened a new tab. 6. Clicked on "Push & Iframe" button. 7. Clicked on "Back" button. We observed a change on clicking the back button, i.e., it went back to history. Attaching the screen cast of the same. @Reporter: Could you please check the screen cast and let us know whether we have missed any steps in reproducing the issue. Please the the same on a clean profile and mention if the issue still persists. Any further inputs from your end helps us to triage the issue in a better way. Thanks!
,
Dec 21 2017
There you go, the bug is there. When you click the `back` button the first time, your navigation bar (back and forward buttons) doesn't do anything. In other words, executing `window.history.back()` after `iframe & push` doesn't work. The new tab step is bad reproduced. I meant rerunning steps 1, 2, and 3 again, and click `Push & Iframe` button instead. After that, when you click on `back` button, the navigation bar works as expected. Thank you for your time.
,
Dec 21 2017
Thank you for providing more feedback. Adding requester "vamshi.kommuri@techmahindra.com" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 21 2017
I think I can reproduce.
,
Dec 22 2017
Able to reproduce the issue on the reported chrome version 63.0.3239.84 and on the latest canary 65.0.3299.0 using Mac 10.13.1, Ubuntu 14.04 and Windows 10. Manual Bisect Info: ================= Good Build: 63.0.3216.0 Bad Build: 63.0.3217.0 You are probably looking for a change made after 502250 (known good), but no later than 502251 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/282ea833bef8dbf11acfa618d06568a910242c4d..0dbded05ad0ba1c3d8388fd649130509ab97f07a Reviewe URL: https://chromium-review.googlesource.com/581488 Suspecting the same. @clamy: Please confirm the issue and help in re-assigning if it is not related to your change. Note: Adding label RB-stable feel free to remove if not required. Thanks!
,
Dec 22 2017
Yes it would make sense it's PlzNavigate related.
,
Jan 8 2018
Still seeing the issue on Windows 7,Mac 10.12.6 & ubuntu 14.04 using latest Canary-65.0.3315.0 as per C#0. clamy@,Could you please take a look into this issue. Thanks..!
,
Jan 11 2018
Issue 800961 has been merged into this issue.
,
Jan 17 2018
clamy@ Ping! This issue is marked as RB-Stable for M65, could you please let us know is there any latest update available on this issue? Thanks!
,
Jan 17 2018
Sorry I'm already investigating several other bugs. I'm trying to get to this bug this week, but I'm not sure I'll be able to. We've been dealing with a lot of navigation-related bugs due to PlzNavigate, and I don't think we'll be able to fix all of them for 65.
,
Jan 17 2018
I can reproduce. If it helps, I'll take a look at it this end of the day.
,
Jan 17 2018
I took a very quick look.
_______________________________________________________________________
|
1 |bool LocalFrameClientImpl::NavigateBackForward(int offset) const {
2 | WebViewImpl* webview = web_frame_->ViewImpl();
3 | if (!webview->Client())
4 | return false;
5 |
6 | DCHECK(offset);
7 | if (offset > webview->Client()->HistoryForwardListCount())
8 | return false;
9 | if (offset < -webview->Client()->HistoryBackListCount())
* 10| return false;
11| webview->Client()->NavigateBackForwardSoon(offset);
12| return true;
13|}
____|___________________________________________________________________
It returns line 10.
RenderViewImpl::history_list_offset_ is 0 (instead of 1).
I wonder if we could remove lines [5..10].
For me, the browser is the source of truth about the history.
I will take a look at RenderViewImpl::history_list_offset_...
,
Jan 17 2018
No I don't think we can do that. While the browser process is the source of truth about the history, some of its history state needs to be replicated in the renderer processes since some JavaScript APIs might need synchronous access. It would seem we don't update RenderViewImpl::history_list_offset_ properly, and we should fix that.
,
Jan 18 2018
I wrote a regression test here: https://chromium-review.googlesource.com/873918 Assigning the issue to me.
,
Jan 19 2018
I understand the issue.
The history state is stored in RenderViewImpl
{
history_list_offset_;
history_list_length_;
}
It is updated in two places:
- RenderFrameImpl::CommitNavigation.
- RenderFrameImpl::DidCommitProvisionalLoad.
The issue is that:
- an "iframe insertion" calls BeginNavigation -> CommitNavigation -> DidCommitNavigation
- a "pushState calls DidCommitNavigation.
If these messages are interleaved, things go wrong.
In the repro steps:
1) An iframe is inserted, the current state is [offset = 0, length = 1]. BeginNavigation is sent.
2) "PushState" is called, state is updated in DidCommitProvisionalLoad [offset = 1, length = 2]
3) RenderFrameImpl::CommitNavigation is received for the iframe. state is updated [offset = 0, length = 1]
It looks like this:
```
┌────────┐ ┌───────┐
│Renderer│ │Browser│
└───┬────┘ └───┬───┘
│ BeginNavigation │
│ - url=iframe.hmtl │
│ - history_offset = 0 │
│ - history_length = 1 │
1) │ ────────────────────────────>
│ ┌──┤
│ DidCommitProvisionalLoad │ │
│ - "pushState" │ │
│ - history_offset = 1 │ │
│ - history_length = 2 │ │
2) │ ────────────────────────────> 2)
│ │ │
│ CommitNavigation │ │
│ - url=iframe.hmtl │ │
│ - history_offset = 0 │ │
│ - history_length = 1 │ │
┌──│ <────────────────────────┘ │
│ │ │
│ │ DidCommitProvisionalLoad │
│ │ - url=iframe.hmtl │
│ │ - history_offset = 0 │
│ │ - history_length = 1 │
└─>│ ────────────────────────────> 1)
┌───┴────┐ ┌───┴───┐
│Renderer│ │Browser│
└────────┘ └───────┘
```
The issue is that two different processes can update the state:
- the browser by sending CommitNavigation
- the renderer by calling DidCommitNavigation.
This is tricky. What are your though about this creis@ and alexmos@?
,
Jan 25 2018
FYI: Fix will probably be: https://chromium-review.googlesource.com/c/chromium/src/+/881171
,
Jan 30 2018
Friendly ping to get an update on this issue.
,
Jan 30 2018
I am working on this right now. I need to update my patch.
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/841b1d49851beaf63a447187d013948c10b901d4 commit 841b1d49851beaf63a447187d013948c10b901d4 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Wed Jan 31 08:42:29 2018 Fix wrong RenderView history. This fixes https://crbug.com/796561 and adds a regression test. The issue was a race between the browser and renderer processes while updating the renderer's history. The renderer's history is updated in * RenderFrameImpl::CommitNavigation and in * RenderFrameImpl::DidCommitProvisionalLoad. ┌────────┐ ┌───────┐ │Renderer│ │Browser│ └───┬────┘ └───┬───┘ │ BeginNavigation │ │ - url=iframe.html │ │ - history_offset = 0 │ │ - history_length = 1 │ 1) │ ──────────────────────────> ┐ │ │ │ │ DidCommitProvisionalLoad │ │ │ - "pushState" │ │ │ - history_offset = 1 │ │ │ - history_length = 2 │ │ 2) │ ──────────────────────────> │ │ │ │ │ CommitNavigation │ │ │ - url=iframe.html │ │ │ - history_offset = 0 │ │ │ - history_length = 1 │ │ ┌─│ <───────────────────────────┘ │ │ │ │ │ DidCommitProvisionalLoad │ │ │ - url=iframe.hmtl │ │ │ - history_offset = 0 │ │ │ - history_length = 1 │ └>│ ──────────────────────────> ┌───┴────┐ ┌───┴───┐ │Renderer│ │Browser│ └────────┘ └───────┘ What happens in the bug. 1) An iframe is created: [history.offset = 0, history.size = 1]. 2) history.pushState: history is updated [offset = 1, size = 2]. 3) CommitNavigation is received, history is updated using the old values transmitted in 1) with BeginNavigation. This CL update the values sent in CommitNavigation. This reduce considerably the duration when the race can happen. Before this CL, the race could happen if a history.pushState is sent in between BeginNavigation and CommitNavigation (~300ms <-> severals seconds). After this CL, the race happens if the history.pushState is sent between when the CommitNavigation message is sent and when it is received by the renderer process. Bug: 796561 Change-Id: I7bf72fadbf9f4946da28126a394adddf051f49a8 Reviewed-on: https://chromium-review.googlesource.com/881171 Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#533233} [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/browser/frame_host/navigation_request.h [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/renderer/render_frame_impl.cc [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/841b1d49851beaf63a447187d013948c10b901d4/content/renderer/render_view_impl.cc
,
Jan 31 2018
Last patch fixed the issue. I am waiting it reaches canary and will request a merge in M65.
,
Jan 31 2018
[Auto-generated comment by a script] We noticed that this issue is targeted for M-65; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-65 label, otherwise remove Merge-TBD label. Thanks.
,
Feb 2 2018
It reached Canary 66.0.3336.0. I tried it on Android and I can no more reproduce the bug.
I am requesting a merge in M65.
* The fix was simple.
* Bug Severity is low, the issue was there for a while. It can (rarely) cause:
- javascript("history.length") to be incorrect
- javascript("history.back()") not to work.
,
Feb 3 2018
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 4 2018
Pls merge your change to M65 branch 3325 before 2:00 PM PT tomorrow, Monday (02/25/18) so we can pick it up for last dev release on Tuesday. Thank you.
,
Feb 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ea0d71e3ed26313f2ca74d1297ccd99d20364db commit 0ea0d71e3ed26313f2ca74d1297ccd99d20364db Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon Feb 05 09:15:17 2018 Fix wrong RenderView history. This fixes https://crbug.com/796561 and adds a regression test. The issue was a race between the browser and renderer processes while updating the renderer's history. The renderer's history is updated in * RenderFrameImpl::CommitNavigation and in * RenderFrameImpl::DidCommitProvisionalLoad. ┌────────┐ ┌───────┐ │Renderer│ │Browser│ └───┬────┘ └───┬───┘ │ BeginNavigation │ │ - url=iframe.html │ │ - history_offset = 0 │ │ - history_length = 1 │ 1) │ ──────────────────────────> ┐ │ │ │ │ DidCommitProvisionalLoad │ │ │ - "pushState" │ │ │ - history_offset = 1 │ │ │ - history_length = 2 │ │ 2) │ ──────────────────────────> │ │ │ │ │ CommitNavigation │ │ │ - url=iframe.html │ │ │ - history_offset = 0 │ │ │ - history_length = 1 │ │ ┌─│ <───────────────────────────┘ │ │ │ │ │ DidCommitProvisionalLoad │ │ │ - url=iframe.hmtl │ │ │ - history_offset = 0 │ │ │ - history_length = 1 │ └>│ ──────────────────────────> ┌───┴────┐ ┌───┴───┐ │Renderer│ │Browser│ └────────┘ └───────┘ What happens in the bug. 1) An iframe is created: [history.offset = 0, history.size = 1]. 2) history.pushState: history is updated [offset = 1, size = 2]. 3) CommitNavigation is received, history is updated using the old values transmitted in 1) with BeginNavigation. This CL update the values sent in CommitNavigation. This reduce considerably the duration when the race can happen. Before this CL, the race could happen if a history.pushState is sent in between BeginNavigation and CommitNavigation (~300ms <-> severals seconds). After this CL, the race happens if the history.pushState is sent between when the CommitNavigation message is sent and when it is received by the renderer process. TBR=arthursonzogni@chromium.org (cherry picked from commit 841b1d49851beaf63a447187d013948c10b901d4) Bug: 796561 Change-Id: I7bf72fadbf9f4946da28126a394adddf051f49a8 Reviewed-on: https://chromium-review.googlesource.com/881171 Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#533233} Reviewed-on: https://chromium-review.googlesource.com/901262 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#293} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/browser/frame_host/navigation_request.h [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/renderer/render_frame_impl.cc [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/0ea0d71e3ed26313f2ca74d1297ccd99d20364db/content/renderer/render_view_impl.cc |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by peter@chromium.org
, Dec 20 2017