Regression : Gmail main page doesn't load after reloading the current page.
Reported by
avsha...@etouch.net,
Sep 28 2017
|
|||||||||||||||
Issue descriptionChrome version : 63.0.3226.0 (Official Build) 209b73b2d53c279870bd421b7c04a1427798e2ef-refs/heads/master@{#504841} 32/64 bit OS : Windows (7,8,10), Linux(14.04 LTS), Mac(10.12.6) Precondition : Log in to Gmail account using valid credentials. What steps will reproduce the problem? 1. In Gmail inbox, open any email thread which has attached PDF file in it. 2. Click on attached PDF file to open it (let PDF load completely) and hit F5 key to reload the current page. 3. Observe. Actual Result : Gmail main page doesn't load after reloading the current page. Expected Result : Gmail main page should load completely after reloading the current page. (Note : Needs to reload page twice in order to load the Gmail page completely). This is a regression issue broken in ‘M-63’ and using the per-revision bisect providing the bisect results, Good build : 63.0.3216.0 (Revision : 502109) Bad build : 63.0.3217.0 (Revision : 502454) You are probably looking for a change made after 502250 (known good), but no later than 502251 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/282ea833bef8dbf11acfa618d06568a910242c4d..0dbded05ad0ba1c3d8388fd649130509ab97f07a Suspect : https://chromium.googlesource.com/chromium/src/+/0dbded05ad0ba1c3d8388fd649130509ab97f07a @clamy : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner. Thank you.
,
Sep 29 2017
Reassigning because blamed author is out for a while. Also changing the component, although I'm not sure if it's the right one.
,
Sep 29 2017
,
Sep 29 2017
FWIW, I am not able to repro the problem on 63.0.3223.8 :-( avshaikh@, could you please: 1. Confirm whether the problem is indeed related to PlzNavigate. Does the problem repro after launching Chrome with --disable-browser-side-navigation (or if this is easier - after disabling chrome://flags/#browser-side-navigation flag and restarting the browser) 2. Does the problem *always* repro or does it only repro from time to time? 3. Can you please share the "variations" / experiments you were assigned to on the chrome://version page?
,
Oct 3 2017
With response to c#4, Retested above issue using latest 63.0.3231.0 build and got the following observations: 1. Issue doesn't reproduce after disabling the chrome://flags/#browser-side-navigation flag. 2. And issue always repro only if we enable the --browser-side-navigation flag. 3. Variations from chrome://version page: 3095aa95-3f4a17df 7c1bc906-f55a7974 47e5d3db-3d47f4f4 d43bf3e5-bd7cd813 ba3f87da-45bda656 9e201a2b-7e3ae057 68812885-4d2fac87 9773d3bd-f23d1dea 99144bc3-3cc2175e 9e5c75f1-176eff0f f79cb77b-3d47f4f4 4ea303a6-ecbb250e 447469ba-ca7d8d80 7aa46da5-54f5980d f47ae82a-746c2ad4 3ac60855-486e2a9c f296190c-9e248081 4442aae2-75cb33fc ed1d377-e1cc0f14 75f0f0a0-a5822863 e2b18481-4ad60575 e7e71889-4ad60575 644b8345-ca7d8d80 94e68624-f23d1dea 828a5926-ca7d8d80
,
Oct 9 2017
lukasza@/jam, Could you please check and update the thread as per C#5, as it is marked as stable blocker issue. Thanks..!
,
Oct 9 2017
jam@ is OOO (previous week + this week). Assigning the issue to myself. I was not able to reproduce the issue on Linux 63.0.3231.0 build.
,
Oct 9 2017
I can reproduce it now. revision 504625 -> can't reproduce consistently. revision 507359 -> can reproduce consistently. I'll see if a bisection is possible.
,
Oct 9 2017
I thought I could reproduce/not reproduce the issue consistently, but it is flaky instead. I think there is a race condition. This race condition is described here: https://chromium-review.googlesource.com/c/chromium/src/+/671351 I log every NavigationHandleImpl methods: #Non-PlzNavigate A::NavigationHandleImplurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::WillStartRequesturl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::RegisterNavigationThrottlesurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::CheckWillStartRequesturl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::RunCompleteCallbackurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::WillProcessResponseurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::CheckWillProcessResponseurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::MaybeTransferAndProceedurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::MaybeTransferAndProceedInternalurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::ReadyToCommitNavigationurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::RunCompleteCallbackurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::DidCommitNavigationurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 A::~NavigationHandleImplurl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector = 1 B::NavigationHandleImplurl | https://mail.google.com/mail/u/0/#inbox/removed_part B::DidCommitNavigationurl | https://mail.google.com/mail/u/0/#inbox/removed_part B::~NavigationHandleImplurl | https://mail.google.com/mail/u/0/#inbox/removed_part Interpretation: two navigation happens consecutively. ┌───────┐ ┌────────┐ │Browser│ │Renderer│ └───┬───┘ └───┬────┘ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_A │ │ ║ ╟──────────────────CommitNavigation(UrlA) │ ║ ║ │──────────────────────────────> ║ ║ │ │ ║ ║ │DidCommitProvisionalLoad(UrlA)│ ║ ║ │<────────────────────────────── ║ ╚═════════════╪══════════════════════════════╪══════════════╝ │ │ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_B │ │ ║ ╟──────────────────CommitNavigation(UrlB) │ ║ ║ │──────────────────────────────> ║ ║ │ │ ║ ║ │DidCommitProvisionalLoad(UrlB)│ ║ ║ │<────────────────────────────── ║ ╚═════════════╪══════════════════════════════╪══════════════╝ ┌───┴───┐ ┌───┴────┐ │Browser│ │Renderer│ └───────┘ └────────┘ #PlzNavigate: A::NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::WillStartRequest | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::RegisterNavigationThrottles | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::CheckWillStartRequest | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::RunCompleteCallback | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::WillProcessResponse | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::CheckWillProcessResponse | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::MaybeTransferAndProceed | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::MaybeTransferAndProceedInternal | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::ReadyToCommitNavigation | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 A::RunCompleteCallback | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 B::NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part B::ReadyToCommitNavigation | https://mail.google.com/mail/u/0/#inbox/removed_part A::~NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 B::~NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part C::NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 C::DidCommitNavigation | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 C::~NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part?projector=1 D::NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part D::DidCommitNavigation | https://mail.google.com/mail/u/0/#inbox/removed_part D::~NavigationHandleImpl | https://mail.google.com/mail/u/0/#inbox/removed_part Interpretation: The first navigation commits in the Browser and we are waiting for the Renderer to send DidCommitProvisionalLoad. In the meantime, a second navigation replace the first one and commit in the browser. Then we receive the DidCommitProvisionalLoad from the first navigation, but no pending NavigationHandleImpl is found in TakeNavigationHandleForNavigation(), so a third NavigationHandleImpl is created. The same thing for the fourth navigation to the second url. ┌───────┐ ┌────────┐ │Browser│ │Renderer│ └───┬───┘ └───┬────┘ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_A │ │ ║ ╟──────────────────CommitNavigation(UrlA) │ ║ ║ │──────────────────────────────> ║ ╚═════════════╪══════════════════════════════╪══════════════╝ │ │ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_B │ │ ║ ╟──────────────────CommitNavigation(UrlB) │ ║ ║ │──────────────────────────────> ║ ╚═════════════╪══════════════════════════════╪══════════════╝ │ │ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_C │ │ ║ ╟──────────────DidCommitProvisionalLoad(UrlA)│ ║ ║ │<────────────────────────────── ║ ╚═════════════╪══════════════════════════════╪══════════════╝ │ │ │ │ ╔═════════════╪═══════╤══════════════════════╪══════════════╗ ║ NAVIGATIONHANDLE_D │ │ ║ ╟──────────────DidCommitProvisionalLoad(UrlB)│ ║ ║ │<────────────────────────────── ║ ╚═════════════╪══════════════════════════════╪══════════════╝ ┌───┴───┐ ┌───┴────┐ │Browser│ │Renderer│ └───────┘ └────────┘
,
Oct 10 2017
I know more now: When the reload-navigation happens, I think a "onBeforeUnload" handler triggers a back navigation. So there are two navigations in parallel. 1) The reload-navigation reach the ReadyToCommit state and the renderer is loading the response's body. 2) In the meantime, it is canceled by the back navigation. The back navigation is an "history-same-document" navigation and the frame is stuck this half-loaded state. I have an one-line change that fixes this issue, but I am not really sure of it for the moment. I will work on a browser test + fix today. +CC creis@ FYI.
,
Oct 10 2017
That's where I am today: I tried to make a test, but it is still not working as I wanted right now. https://chromium-review.googlesource.com/c/chromium/src/+/709117 FYI: Here is the one-line change that fixes the issue that I mentioned before: https://chromium-review.googlesource.com/c/chromium/src/+/709057 This is not a solution, it hides the problem in this particular case. @creis or @nasko: What do you think is the intended behavior when the user is on the page: ``` <script> history.pushState({}, 'pdf projector', '?projector=1'); window.addEventListener('beforeunload', function() { window.setTimeout(()=>history.back(),0) }); </script> ``` and then press "Reload"? Without PlzNavigate, I think that the reload happens first, and then the back navigation. With PlzNavigate, the back navigation stops the reload in the middle. I think the history-same-page-navigation happens when the reload navigation is in the ReadyToCommit stage. While the renderer is loading the response's body, the second navigation calls RenderFrameHostImpl::CommitNavigation(), which deletes the StreamHandle of the reload-navigation. If you have a better understanding of this, let me know ;-)
,
Oct 10 2017
,
Oct 10 2017
Correction FWIW: history.back() is called from an "unload" listener (and not from a "beforeunload" one.)
,
Oct 10 2017
Comment 11-13: I would expect navigations in unload handlers to fail. We disallowed that via this Blink Intent from lfg@: https://groups.google.com/a/chromium.org/d/msg/blink-dev/VfItzNe3WO0/Zo95RMlTAwAJ. setTimeout happens to be one of the ways around the limitation, but I'm not sure that means it's supported behavior. In particular, it seems like a bad race and one that I wouldn't expect to work. I wonder if we can talk with Gmail folks about alternatives for this case? Consider the case that a navigation from A to B does this setTimeout(function () { history.back() }). There's no reason to think that the timeout will run before B commits. Indeed, if it's a cross-process navigation, B has already committed by the time we run the unload handler to begin with. It's not really well-defined anyway-- history.back() is outside the page's control. It might be taking the user to a different page entirely (e.g., if the user copy/pasted the URL into a different tab that already had a back history). Maybe we should try to block navigations in setTimeout within unload? My understanding (via dcheng@) is that this is non-trivial, though. Of course, it does seem like PlzNavigate's behavior here isn't great either, leaving the reloaded page in a half-loaded state. We may want to find a better way to handle the race (e.g., letting the reload win and not respecting the back navigation?), even if it doesn't make the back navigation work before the reload occurs.
,
Oct 10 2017
The ideal solution would be that all pending navigations scheduled by the previous Document would be cancelled on commit. In the past, NavigationScheduler more or less managed this. IIRC, clamy@ wrote up a doc about doing this for PlzNavigate. I'm hoping that maybe we can get this behavior naturally (or at least clean up whatever cancellation logic we'll need to add to fix this) once we have 1:1 LocalFrame:Document, but that's at least a quarter, if not more, away.
,
Oct 10 2017
As for blocking navigations, we can see if blocking it once we're preparing to commit helps. The difficulty with doing this generally is I was trying to figure out how to address abusing setTimeout from beforeunload to bypass the navigation block on beforeunload...
,
Oct 11 2017
I made a simple patch to more aggressively block navigations: however, it doesn't seem to actually help... Patch I was testing with locally: https://chromium-review.googlesource.com/#/c/chromium/src/+/711094 (I'll do some more debugging tonight to see if this is actually blocking navigations or if I'm overlooking something)
,
Oct 11 2017
,
Oct 11 2017
I have a test for this issue: https://chromium-review.googlesource.com/c/chromium/src/+/709117 I found a new bug that allow history.back() to be called in an "unload" event handler. I allows this bug to happens. This one also happens without PlzNavigate. See https://crbug.com/773683.
,
Oct 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f6d5ab230fe3023f821d380804bf5d9fb90a1ec9 commit f6d5ab230fe3023f821d380804bf5d9fb90a1ec9 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Thu Oct 12 21:07:04 2017 PlzNavigate: Prevent same-document navigation from canceling pending load. This patch prevent a same-document navigation to stop the current document from loading. The same-document navigation will not replace the current one. So if it is stopped, the user will end up with a half loaded document. See the others CLs in this series: [1/2] This patch [2/2] https://chromium-review.googlesource.com/c/709117 Bug: 769645 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I9bc09581f24f4a16abdc38e4e3ddac21d85ec4d3 Reviewed-on: https://chromium-review.googlesource.com/709057 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#508463} [modify] https://crrev.com/f6d5ab230fe3023f821d380804bf5d9fb90a1ec9/content/browser/frame_host/render_frame_host_impl.cc
,
Oct 13 2017
Update : Retested above issue in latest chrome canary #63.0.3239.0 build on Windows(7,8,10), Mac(10.12.6) & Linux (14.04 LTS) OS and issue is fixed. Now gmail home page loads completely after reloading the current page. Thank you!
,
Oct 16 2017
Thanks for the verification! The fix was released in 63.0.3239.0, but I verified, the bug affects M61(stable) and M62(beta) too. M62 has reached the stable cut and will be released tomorrow (17 Oct), so it is probably too late for a merge request. Let me know if you think otherwise.
,
Oct 16 2017
,
Oct 16 2017
This bug requires manual review: We are only 0 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73fabc9f445e5332fc499d81ccb5f0222fb78e43 commit 73fabc9f445e5332fc499d81ccb5f0222fb78e43 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon Oct 16 14:39:10 2017 PlzNavigate: Add test for issue 769645 . This patch reproduce two issues: 1) Issue 773683: It is possible to call history.back() in an "unload" event handler. 2) Issue 769645 : The history.back() navigation prevents a reload navigation to fully load with PlzNavigate. See the others CLs in this series: [1/2] https://chromium-review.googlesource.com/c/709057 [2/2] This patch. Bug: 769645 ,773683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I5475b3acf8f36bf630b2c94adf11dac2e20d43ef Reviewed-on: https://chromium-review.googlesource.com/709117 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#509042} [modify] https://crrev.com/73fabc9f445e5332fc499d81ccb5f0222fb78e43/content/browser/frame_host/navigation_controller_impl_browsertest.cc
,
Oct 16 2017
confirmed with clamy@, this cl is quite small and well tested, and this is a user facing regression. Approving merge to M62 as this has already been tested and verified in M63 and has fix baked in for a few days.
,
Oct 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/052b122b42e79fc5c73312271a981e94125c08e7 commit 052b122b42e79fc5c73312271a981e94125c08e7 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon Oct 16 16:39:02 2017 PlzNavigate: Prevent same-document navigation from canceling pending load. This patch prevent a same-document navigation to stop the current document from loading. The same-document navigation will not replace the current one. So if it is stopped, the user will end up with a half loaded document. See the others CLs in this series: [1/2] This patch [2/2] https://chromium-review.googlesource.com/c/709117 TBR=nasko@chromium.org Bug: 769645 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I9bc09581f24f4a16abdc38e4e3ddac21d85ec4d3 Reviewed-on: https://chromium-review.googlesource.com/709057 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#508463}(cherry picked from commit f6d5ab230fe3023f821d380804bf5d9fb90a1ec9) Reviewed-on: https://chromium-review.googlesource.com/721121 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#692} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/052b122b42e79fc5c73312271a981e94125c08e7/content/browser/frame_host/render_frame_host_impl.cc
,
Oct 17 2017
Update : Retested above issue in latest chrome Stable build #62.0.3202.62 on Windows(7,8,10), Mac(10.12.6) & Linux (14.04 LTS) OS and issue is fixed. Now fix is working as intended on latest chrome Stable build. Thank you!
,
Oct 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43b022790cb8f701affe5dc2b1689203847ef2b4 commit 43b022790cb8f701affe5dc2b1689203847ef2b4 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Wed Oct 18 08:07:40 2017 Test: same-document navigation while loading. This CL adds a test for issue 769645 that was fixed by: https://chromium-review.googlesource.com/c/709057 It checks that when a browser-initiated same-document navigation happens while the current document is loading, the load isn't interrupted. Bug: 769645 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I2e2ae42d970dacc7217143488b5506759d61f22f Reviewed-on: https://chromium-review.googlesource.com/716756 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#509715} [modify] https://crrev.com/43b022790cb8f701affe5dc2b1689203847ef2b4/content/browser/frame_host/render_frame_host_impl_browsertest.cc |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by msrchandra@chromium.org
, Sep 28 2017