Event listener not attached to dynamic iframe content
Reported by
exner.cu...@googlemail.com,
Jul 18 2017
|
||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/59.0.3071.109 Chrome/59.0.3071.109 Safari/537.36 Steps to reproduce the problem: 1. Load the minimal non-working extension: https://gist.github.com/anonymous/107fe229ca97edd8bf7d630643f06097 2. Open a new tab. 3. Try closing the iframe using the button. What is the expected behavior? The extension appends an iframe to the current document. The iframe's content is dynamically generated by a javascript snippet. It also attaches an event listener to a button.On click the listener should fire and removes the iframe. What went wrong? The event listener is not added in ~50% of the cases. Did this work before? N/A Chrome version: 59.0.3071.109 Channel: n/a OS Version: Ubuntu 16.04 Flash Version:
,
Jul 19 2017
Able to reproduce on Windows-10, Ubuntu 14.04 and Mac OS 10.11.5 using chrome latest stable M59-59.0.3071.109. Note: The iframe content in new tab page will get closed only for the first time. Bisect Information: -------------------- Good build: 59.0.3036.0 Bad Build : 59.0.3038.0 You are probably looking for a change made after 455974 (known good), but no later than 455975 (first known bad). Change Log URL: https://chromium.googlesource.com/chromium/src/+log/10184858622f92e96aa3aa550b72dab2dc2d2eef..128ef2d7248bd291e08ddfa30c8444dc26abc9b8 From the above change log suspecting below change Review URL: https://codereview.chromium.org/2740873005 dcheng@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. Adding RB-stable for M-60 please feel free to edit if this is not the case. Thanks!
,
Jul 19 2017
,
Jul 24 2017
Just to update the latest behaviour, Still able to reproduce the issue on Win-10 using latest beta #60.0.3112.72. dcheng@ - Could you please have a look into the issue as it has been marked as a stable blocker for M60. Thanks...!!
,
Jul 24 2017
This is too late for M60 as we're very close to Stable and only taking critical fixes. Moving to 61
,
Jul 26 2017
URGENT - PTAL. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the M61 branch #3163 ASAP to have enough baking time in Beta before Stable promotion. Thank you! Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label.
,
Jul 28 2017
I think that I've found the cause. Will upload a fix shortly.
,
Jul 28 2017
Uploaded at: https://chromium-review.googlesource.com/c/591728
,
Jul 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8218917eb25640ddf0d37c8b7d36102a79737d5 commit b8218917eb25640ddf0d37c8b7d36102a79737d5 Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Jul 31 10:47:38 2017 v8binding: Fixes wrapper tracing to follow: LocalDOMWindow => Document. The root cause of https://crbug.com/745771 is that a listener object, stored in V8AbstractEventListener::listener_, is wrongly collected by V8 GC. Listener objects are expected to be wrapper-traced at DEFINE_TRACE_WRAPPERS(EventTarget) through DEFINE_TRACE_WRAPPERS(Node) and DEFINE_TRACE_WRAPPERS(ContainerNode), however, there seems no root object which kicks this path of wrapper- tracing. This CL adds a new path of wrapper-tracing from LocalDOMWindow to Document. Since LocalDOMWindow is a root object and Document is a ContainerNode which wrapper-traces all child nodes, this CL makes sure that all event listeners registered at any part of the document tree alive. (If an EventTarget is not attached to the DOM, such a case is not covered by this CL.) The fix was confirmed manually with using a test Chrome extension. Bug: 745771 Change-Id: I7c0d1fb766b78fd4d32ce813662c947ce5f7e819 Reviewed-on: https://chromium-review.googlesource.com/591728 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#490742} [modify] https://crrev.com/b8218917eb25640ddf0d37c8b7d36102a79737d5/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp [modify] https://crrev.com/b8218917eb25640ddf0d37c8b7d36102a79737d5/third_party/WebKit/Source/core/frame/LocalDOMWindow.h
,
Aug 1 2017
Confirmed the fix locally, the CL is sticking, and ToT is working just fine with the CL. May I request a merge of #c9 to M61?
,
Aug 2 2017
Tested on Chrome #62.0.3174.0 on Windows 7, Mac 10.12.6 and Ubuntu 14.04 and below are the observations noticed upon following the steps below: 1. Download and install the extension provided in https://gist.github.com/anonymous/107fe229ca97edd8bf7d630643f06097 2. Navigate to the different websites. Observations - When navigated to www.google.co.in, user is unable to remove the iframe. However, upon navigating to other websites like www.facebook.com, www.youtube.com, www.cnn.com etc user is able to remove the iframe. Note: Screencast is attached for reference.
,
Aug 2 2017
Re: #11, thanks for the confirmation. It's expected. The extension attached is NOT great for testing. Depending on web pages, it's possible that the iframe is shadowed by other elements, i.e. it's not guaranteed that the iframe is always placed on the top (in z-axis) of the web content. Then, you cannot simply click the button because it's shadowed. If you tweak the extension a little, then you can click the button. See the attached file. I added two lines on line 2 to 3 as follows. ---- frame.style.position = "fixed"; frame.style.zIndex = 99999; ---- This makes the iframe on the top (in z-axis).
,
Aug 2 2017
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d63e8b5ab223f704bd66f6b6d7e031b222e2369 commit 6d63e8b5ab223f704bd66f6b6d7e031b222e2369 Author: Yuki Shiino <yukishiino@chromium.org> Date: Wed Aug 02 14:56:32 2017 v8binding: Fixes wrapper tracing to follow: LocalDOMWindow => Document. The root cause of https://crbug.com/745771 is that a listener object, stored in V8AbstractEventListener::listener_, is wrongly collected by V8 GC. Listener objects are expected to be wrapper-traced at DEFINE_TRACE_WRAPPERS(EventTarget) through DEFINE_TRACE_WRAPPERS(Node) and DEFINE_TRACE_WRAPPERS(ContainerNode), however, there seems no root object which kicks this path of wrapper- tracing. This CL adds a new path of wrapper-tracing from LocalDOMWindow to Document. Since LocalDOMWindow is a root object and Document is a ContainerNode which wrapper-traces all child nodes, this CL makes sure that all event listeners registered at any part of the document tree alive. (If an EventTarget is not attached to the DOM, such a case is not covered by this CL.) The fix was confirmed manually with using a test Chrome extension. TBR=yukishiino@chromium.org (cherry picked from commit b8218917eb25640ddf0d37c8b7d36102a79737d5) Bug: 745771 Change-Id: I7c0d1fb766b78fd4d32ce813662c947ce5f7e819 Reviewed-on: https://chromium-review.googlesource.com/591728 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#490742} Reviewed-on: https://chromium-review.googlesource.com/598287 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#238} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/6d63e8b5ab223f704bd66f6b6d7e031b222e2369/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp [modify] https://crrev.com/6d63e8b5ab223f704bd66f6b6d7e031b222e2369/third_party/WebKit/Source/core/frame/LocalDOMWindow.h
,
Aug 3 2017
Tested the issue using #61.0.3163.29 on Mac 10.12.5, Linux Ubuntu 14.04, Win 10. Observed the iframe content in new tab page is getting closed for the first time. Hence adding the Verified labels. Please find the screen cast. Thanks |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by manoranj...@chromium.org
, Jul 18 2017