New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 745771 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Event listener not attached to dynamic iframe content

Reported by exner.cu...@googlemail.com, Jul 18 2017

Issue description

UserAgent: 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:
 
Labels: Needs-Triage-M61 Needs-Bisect OS-Windows
I am able to reproduce this issue on Latest Canary#61.0.3160.0 for 'Win7'.
Cc: brajkumar@chromium.org
Components: Platform>Extensions
Labels: -Type-Bug -Pri-2 -Needs-Bisect hasbisect-per-revision ReleaseBlock-Stable M-60 OS-Mac Pri-1 Type-Bug-Regression
Owner: dcheng@chromium.org
Status: Assigned (was: Unconfirmed)
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!

Comment 3 by dcheng@chromium.org, Jul 19 2017

Cc: yukishiino@chromium.org
Components: -Blink Blink>Bindings
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...!!
Labels: -M-60 M-61
This is too late for M60 as we're very close to Stable and only taking critical fixes.  Moving to 61

Comment 6 by gov...@chromium.org, 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.

Cc: dcheng@chromium.org
Owner: yukishiino@chromium.org
Status: Started (was: Assigned)
I think that I've found the cause.  Will upload a fix shortly.

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Labels: Merge-Request-61
Status: Fixed (was: Started)
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?

Cc: pnangunoori@chromium.org
Labels: Needs-Feedback
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. 

745771.mov
7.4 MB Download
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).

content.js
500 bytes View Download
Project Member

Comment 13 by sheriffbot@chromium.org, Aug 2 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
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
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 2 2017

Labels: -merge-approved-61 merge-merged-3163
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

Labels: TE-Verified-M61 TE-Verified-61.0.3163.29
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
Aug 3 2017 12-31 PM.webm
3.9 MB View Download

Sign in to add a comment