Issue metadata
Sign in to add a comment
|
beforeunload event not fired when printing an iframe content
Reported by
eldo...@gmail.com,
Nov 8
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36 Steps to reproduce the problem: 1. run the fiddle 2. click the "print" button 3. close the tab What is the expected behavior? A message dialog should appear asking confirmation for closing the page What went wrong? The page closes with no warnings Did this work before? Yes It works on stable version 70.0.3538.77 at least Does this work in other browsers? Yes Chrome version: 72.0.3604.0 Channel: stable OS Version: 10.0 Flash Version: I use a trick to print an arbitrary html through a function, using an injected iframe. This cause the problem reported. If I just try to open the print dialog using CTRL + P everything works fine.
,
Nov 8
,
Nov 8
I cannot reproduce using Chrome 70 and Windows 7. The beforeunload alert shows up for me.
,
Nov 8
I cannot reproduce with Chrome 70, but can with Chrome 72 dev channel.
,
Nov 8
I'm sorry, I messed up a little bit with the versions cause I opened this issue from the stable channel, but I was referring to the Canary version.
,
Nov 9
Thanks for filing the issue! Able to reproduce the issue on reported chrome version 72.0.3604.0 using Windows 10, Ubuntu 14.04 and Mac 10.13.1 Bisect Information: ------------------- Good Build: 71.0.3571.0 Bad Build: 71.0.3572.0 Providing with chromium Bisect, as running per-revision bisect ended up with errors after few builds are invoked. You are probably looking for a change made after 597039 (known good), but no later than 597057 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/7e3da6dc3d7cd7eb8a55ef301026fe1ab737f814..56dd83c9005f68d55ddbf85c551759526b4e37cd Suspecting: https://chromium.googlesource.com/chromium/src/+/1b2f95835607dbb9c6021df8893fb18f4c7aee9e Review URL: https://chromium-review.googlesource.com/c/1215512 @Yuki Yamada: Please help in assigning it to the right owner if this isn't related to your change. Adding RBS as this seems to be recent regression, please change/remove if not required.
,
Nov 9
,
Nov 10
,
Nov 12
Friendly ping! Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Nov 12
,
Nov 13
Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you. Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.
,
Nov 14
The fix is under review at: https://chromium-review.googlesource.com/c/chromium/src/+/1333548
,
Nov 15
Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you. Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.
,
Nov 15
When do you expect cl listed at #12 to land in trunk/canary? +thestig@, is this truly M71 stable blocker and will be fully safe to merge in this late in release cycle?
,
Nov 15
I don't have a good answer re: comment 14. I have no idea what websites actually do this when printing and what kind of website breakages will happen as a result.
,
Nov 16
The phenomena of the issue are simple: Expected: When you're about to close a tab, a dialog like "Do you really want to leave this page?" is shown. Actual: Usually, the dialog is shown (and you have a chance to cancel) as expected, however, when another dialog (like print dialog) is shown, the dialog of close confirmation is not shown. I'm not pretty sure if this is stable release block or not. I thought that I would be able to land the fix this week, but it seems like I'm failing. Probably, I can land the fix early next week, I think.
,
Nov 16
Re #16: Pls land the change to trunk ASAP and request a merge to M71 only if it is truly critical and fully safe to merge. Thank you.
,
Nov 19
haraken@ and I discussed about the fix and merge, and we concluded that we land the fix and merge it into M71. Now submitting the fix patch. Will request a merge once it's landed.
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a602b7b87adb930495bd95a17ab2771be7a60fd commit 1a602b7b87adb930495bd95a17ab2771be7a60fd Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Nov 19 09:56:32 2018 v8binding: Invoke 'beforeunload' event listeners even when paused This patch fixes a regression at https://crrev.com/c/1215512 , which doesn't run 'beforeunload' event listeners when one of the related execution contexts is paused. This patch adds an exceptional handling for 'beforeunload' event so that 'beforeunload' event listeners run regardless of pause. Bug: 903297 Change-Id: I92bb24a8fd16ee9f81a5082f5c561c97c1dd5348 Reviewed-on: https://chromium-review.googlesource.com/c/1333548 Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#609217} [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/core/v8/generated_code_helper.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/core/v8/generated_code_helper.h [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/core/v8/js_event_listener.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/templates/callback_function.cc.tmpl [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/templates/callback_function.h.tmpl [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/templates/callback_interface.cc.tmpl [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/templates/callback_interface.h.tmpl [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/templates/callback_invoke.cc.tmpl [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_variadic_any_args.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_treat_non_object_as_null_boolean_function.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_treat_non_object_as_null_void_function.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.cc [modify] https://crrev.com/1a602b7b87adb930495bd95a17ab2771be7a60fd/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.cc
,
Nov 19
Request a merge of #c19 into M71. I confirmed the fix locally. Will confirm the fix in Canary in parallel to the merge request to M71.
,
Nov 19
Pls update bug with canary result tomorrow morning. Pls note change has to be super safe to merge as it will go to stable very soon on Dec 4th. Thank you.
,
Nov 19
Also this seems to be a big cl, will it be ok to merge this late in release cycle?
,
Nov 20
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 20
Re 21 & 22: peria@ kindly confirmed the fix with a Canary on Mac OS X. The patch might look a little big in terms of LOC (lines of code), however, the fix is pretty simple actually. We added a flag and enabled it only for beforeunload event. Nothing complex is there. haraken@ and I think that the merge should be safe.
,
Nov 20
The NextAction date has arrived: 2018-11-20
,
Nov 20
How is the change looking in canary?
,
Nov 21
I checked a list of crashes (magic signature), but I didn't find something that looks related to my patch. The fix seems safe to me. https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+STRPOS%28product.Version%2C+%2772.0.%27%29+%3E+0+AND+STRPOS%28product.Version%2C+%2772.0.3616%27%29+%3E+0+AND+product.Version%3D%2772.0.3616.0%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27renderer%27 As peria@ unloaded at #c27, the issue is fixed as expected.
,
Nov 21
Approving merge to M71 branch 3578 based on comment #28. Please merge ASAP.
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78901fb89db9d9fac22c90ad24a7abda44a6eda0 Commit: 78901fb89db9d9fac22c90ad24a7abda44a6eda0 Author: yukishiino@chromium.org Commiter: yukishiino@chromium.org Date: 2018-11-21 14:34:39 +0000 UTC v8binding: Invoke 'beforeunload' event listeners even when paused This patch fixes a regression at https://crrev.com/c/1215512 , which doesn't run 'beforeunload' event listeners when one of the related execution contexts is paused. This patch adds an exceptional handling for 'beforeunload' event so that 'beforeunload' event listeners run regardless of pause. TBR=yukishiino@chromium.org (cherry picked from commit 1a602b7b87adb930495bd95a17ab2771be7a60fd) Bug: 903297 Change-Id: I92bb24a8fd16ee9f81a5082f5c561c97c1dd5348 Reviewed-on: https://chromium-review.googlesource.com/c/1333548 Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#609217} Reviewed-on: https://chromium-review.googlesource.com/c/1346431 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#786} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78901fb89db9d9fac22c90ad24a7abda44a6eda0 commit 78901fb89db9d9fac22c90ad24a7abda44a6eda0 Author: Yuki Shiino <yukishiino@chromium.org> Date: Wed Nov 21 14:34:39 2018 v8binding: Invoke 'beforeunload' event listeners even when paused This patch fixes a regression at https://crrev.com/c/1215512 , which doesn't run 'beforeunload' event listeners when one of the related execution contexts is paused. This patch adds an exceptional handling for 'beforeunload' event so that 'beforeunload' event listeners run regardless of pause. TBR=yukishiino@chromium.org (cherry picked from commit 1a602b7b87adb930495bd95a17ab2771be7a60fd) Bug: 903297 Change-Id: I92bb24a8fd16ee9f81a5082f5c561c97c1dd5348 Reviewed-on: https://chromium-review.googlesource.com/c/1333548 Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#609217} Reviewed-on: https://chromium-review.googlesource.com/c/1346431 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#786} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/core/v8/generated_code_helper.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/core/v8/generated_code_helper.h [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/core/v8/js_event_listener.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/templates/callback_function.cpp.tmpl [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/templates/callback_function.h.tmpl [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/templates/callback_interface.cpp.tmpl [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/templates/callback_interface.h.tmpl [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/templates/callback_invoke.cc.tmpl [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_variadic_any_args.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_treat_non_object_as_null_boolean_function.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_treat_non_object_as_null_void_function.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.cc [modify] https://crrev.com/78901fb89db9d9fac22c90ad24a7abda44a6eda0/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.cc
,
Nov 21
Merged to M71 (branch/3578). Confirmed that it builds successfully and the fix is working very well (locally on branch/3578). |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by eldo...@gmail.com
, Nov 8