Suddenly added beforeunload handler during window closing leads to trashing "Recently Closed"
Reported by
michael....@gmail.com,
Feb 16 2017
|
||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.68 YaBrowser/17.3.0.1195 (beta) Yowser/2.5 Safari/537.36 Steps to reproduce the problem: 1. Open these websites in a new (second) window: yandex.ru, opennet.ru, vk.com, market.yandex.ru, realty.yandex.ru, mail.yandex.ru, www.youtube.com, https://yandex.ru/maps/213/moscow/.; 2. Close the window using window (not keyboard) "X" button; 3. Right after window has just been closed, Press Cmd-Shift-T (or Ctrl-Shift-T on PC) to restore the closed window; 4. When the window opens and starts to load its tabs - Go to step 2 (do not wait for loading all the tabs). Do it until you see subset (not all) of the closed tabs. What is the expected behavior? What went wrong? Go to "History". There will be many entries of closed windows and/or tabs. Did this work before? N/A Chrome version: 56.0.2924.68 Channel: n/a OS Version: OS X 10.12.3 Flash Version: Shockwave Flash 24.0 r0 This bug happens on Windows, Linux and Mac OSX. I can reproduce it even on chromium 55. And on the lastest. I have a fix and will upload CL in a couple of hours. It happens when UnloadContoller has already started to do window closing procedure and after a moment one (or several) tab(s) will send a message that they also have beforeunload handler.
,
Feb 16 2017
Thanks for the fix, we can take the patch once its baked and verified in canary.Please request a merge once everything is set.
,
Feb 16 2017
,
Feb 17 2017
Sounds similar to an older bug fixed in issue 365052 . Thanks for tracking it down!
,
Feb 20 2017
mkolom@yandex-team.ru: Could you please confirm if the fix is landed so as to verify from TE end.
,
Feb 21 2017
Hasn't landed yet. I'm just putting https://codereview.chromium.org/2695233003/ in the CQ now.
,
Feb 22 2017
creis@, will cl listed at #6 be a safe merge to M57 once landed/baked/verified in Canary? Otherwise it can wait until M58 goes to Stable as this bug already exists on current M56 Stable.
,
Feb 22 2017
sky@: Can you answer comment 7 about whether this is safe to merge? I don't understand that code well enough to say how risky it is (though I know it would be a high value fix for M57 if it's safe).
,
Feb 22 2017
URGENT - PTAL ASAP. We're getting VERY close to M57 Stable promotion. And this issue is marked as M57 stable release blocker. Pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M58. Thank you.
,
Feb 22 2017
My understanding is this is a long standing issue, and not something new. I'm removing release block.
,
Mar 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65a0b3cdd8b7605020143b3a5f089dc0878bdbb3 commit 65a0b3cdd8b7605020143b3a5f089dc0878bdbb3 Author: mkolom <mkolom@yandex-team.ru> Date: Thu Mar 02 06:11:40 2017 Fix unload controller. There is a race when suddenly added beforeunload handler during window closing can lead to trashing TabRestoreService. (Browser::OnWindowClosing can be called several times with subsets of previously opened tabs) BUG= 692879 Review-Url: https://codereview.chromium.org/2695233003 Cr-Commit-Position: refs/heads/master@{#454192} [modify] https://crrev.com/65a0b3cdd8b7605020143b3a5f089dc0878bdbb3/chrome/browser/devtools/devtools_window.cc [modify] https://crrev.com/65a0b3cdd8b7605020143b3a5f089dc0878bdbb3/chrome/browser/lifetime/browser_close_manager_browsertest.cc [modify] https://crrev.com/65a0b3cdd8b7605020143b3a5f089dc0878bdbb3/chrome/browser/ui/unload_controller.cc
,
Mar 2 2017
,
Mar 2 2017
mkolom@: Is there more work to do here, or is this fixed? Thanks for the CL!
,
Mar 4 2017
That's all :)
,
Mar 7 2017
Thanks! |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mko...@yandex-team.ru
, Feb 16 2017