Strange behavior regarding a scope of an anchor tag that has an img tag as a child
Reported by
arfar...@gmail.com,
Dec 10
|
|||||||||||||||||||
Issue descriptionChrome Version : 71.0.3578.80 URLs (if applicable) : https://codepen.io/anon/pen/vQoqVa Other browsers tested: Add OK or FAIL, along with the version, after other browsers where you have tested this issue: Safari: 12.0.2 OK Firefox: 62.0.3 OK Edge: What steps will reproduce the problem? (1) Click the image in the CodePen URL provided. (2) (3) What is the expected result? An alert should be displayed. What happens instead? No alert, and `Uncaught TypeError: complete is not a function at HTMLAnchorElement.onclick (index.html:6)` is logged out in console. Please provide any additional information below. Attach a screenshot if possible. Bisect resulted in below changelog commithttps://chromium.googlesource.com/chromium/src/+log/7e3da6dc3d7cd7eb8a55ef301026fe1ab737f814..56dd83c9005f68d55ddbf85c551759526b4e37c I've attached the screenshot showing the scope chain of <img> tag referenced from the scope of <a> tag's onclick event handler.
,
Dec 10
Able to reproduce the issue on reported chrome version #71.0.3578.80 and latest chrome #73.0.3636.0 using Windows 10, Ubuntu 17.10 and Mac 10.13.6 by following steps as per comment#0. Bisect information: =================== Good Build: 71.0.3571.0 Bad Build: 71.0.3572.0 As per comment#1 assigning to Hitoshi Yoshida and below is the Changelog URL. Note: Yuki Yamada is not available from past 30days, hence assigning Hitoshi Yoshida(reviewer). Change Log: https://chromium.googlesource.com/chromium/src/+/1b2f95835607dbb9c6021df8893fb18f4c7aee9e Reviewed-on: https://chromium-review.googlesource.com/c/1215512 Adding Release block stable as it is a recent regression, please feel free to remove if not applicable. @Hitoshi Yoshida: Please help us in reassigning the issue if it is not related to your change. Thanks.!
,
Dec 10
,
Dec 10
,
Dec 10
I'm now preparing a fix at: https://chromium-review.googlesource.com/c/chromium/src/+/1370144 The cause is that Blink is looking up the name |complete| in the scope of the HTMLImageElement (clicked element), although Blink should look up the name in the scope of the HTMLAnchorElement (element where the listener is registered). |complete| in HTMLImageElement: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/html_image_element.idl?rcl=163cedad0195617a5d10625469f687e3a24d4837&l=40&q=%5Cbcomplete%5Cb+case:yes For ReleaseBlock-Stable, I'm not really sure about the impact in either way whether this has a huge impact or it's unlikely to hit such a collision. If the function name was not "complete", the issue didn't happen. If it was not a HTMLImageElement, the issue didn't happen. But it's true that the reporter hit the case. I don't have a good sense about the real world usage.
,
Dec 10
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7bda23a6a30fbc032054da4426cd93070907b290 commit 7bda23a6a30fbc032054da4426cd93070907b290 Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Dec 10 15:03:28 2018 v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#615116} [modify] https://crrev.com/7bda23a6a30fbc032054da4426cd93070907b290/third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc [modify] https://crrev.com/7bda23a6a30fbc032054da4426cd93070907b290/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e68f6014371c9b1672cd3b063ad0dfb5a8ccf01 commit 9e68f6014371c9b1672cd3b063ad0dfb5a8ccf01 Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Dec 10 15:19:59 2018 v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615116}(cherry picked from commit 7bda23a6a30fbc032054da4426cd93070907b290) Reviewed-on: https://chromium-review.googlesource.com/c/1370146 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/branch-heads/3636@{#3} Cr-Branched-From: 6b4d81b2d610bba6e691389b2ef771db17c40498-refs/heads/master@{#615028} [modify] https://crrev.com/9e68f6014371c9b1672cd3b063ad0dfb5a8ccf01/third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc [modify] https://crrev.com/9e68f6014371c9b1672cd3b063ad0dfb5a8ccf01/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc
,
Dec 10
,
Dec 10
For merge safety, I'm pretty sure that the fix wouldn't cause a crash issue. I'd say that the fix merge to M71 must be pretty safe. What I'm not sure about is the criticalness of the issue. I'm inclined to support the stable merge, but open to others' opinions.
,
Dec 10
Thank you Yuki. haraken@, could you pls comment whether it is critical to merge to M71 this late in release cycle? Pls note M71 is out for Android and Desktop since last Tuesday, 12/04. We're planning next M71 respin with critical fixes.
,
Dec 10
This merge will be pretty safe. Regarding criticalness, I also support the stable merge.
,
Dec 10
,
Dec 10
Canary #73.0.3636.2 currently building for Android and Desktop which includes this merge.
,
Dec 10
Verified the fix on latest Chrome Canary i.e., 73.0.3636.2 on Windows 10 and Mac.
,
Dec 10
+prashanthpola to check on Android.
,
Dec 10
Thank you pbommana@ for canary verification. Thank you candrada@. I'm trying to merge the change to M71 to unblock Chrome OS M71 stable RC cut.
,
Dec 10
Approving merge to M71 branch 3578 and M72 branch 3626 based on comments #10, #12, #15 and #17.
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa commit 9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Dec 10 21:20:12 2018 v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615116}(cherry picked from commit 7bda23a6a30fbc032054da4426cd93070907b290) Reviewed-on: https://chromium-review.googlesource.com/c/1370489 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#236} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa/third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc [modify] https://crrev.com/9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dcfa1803d4408370e3b1a187b84d4a6b620f491a commit dcfa1803d4408370e3b1a187b84d4a6b620f491a Author: Yuki Shiino <yukishiino@chromium.org> Date: Mon Dec 10 21:21:32 2018 v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615116}(cherry picked from commit 7bda23a6a30fbc032054da4426cd93070907b290) Reviewed-on: https://chromium-review.googlesource.com/c/1370828 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#889} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/dcfa1803d4408370e3b1a187b84d4a6b620f491a/third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc [modify] https://crrev.com/dcfa1803d4408370e3b1a187b84d4a6b620f491a/third_party/blink/renderer/bindings/core/v8/js_event_handler.cc
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dcfa1803d4408370e3b1a187b84d4a6b620f491a Commit: dcfa1803d4408370e3b1a187b84d4a6b620f491a Author: yukishiino@chromium.org Commiter: govind@chromium.org Date: 2018-12-10 21:21:32 +0000 UTC v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615116}(cherry picked from commit 7bda23a6a30fbc032054da4426cd93070907b290) Reviewed-on: https://chromium-review.googlesource.com/c/1370828 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#889} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Dec 10
Verified on Android Chrome:73.0.3636.2 Device:Moto G4 Plus/7.0 with the steps mentioned in C#0.
,
Dec 10
Triggered M71 build #71.0.3578.94 for Android, Desktop and Chrome OS (currently building) with merge listed at #20.
,
Dec 11
Pls mark s fixed if nothing else is pending. Thank you.
,
Dec 11
,
Dec 11
Thank you Krishna for the hard work and support for fixing the issues.
,
Dec 11
Android: Works as per expected behavior, issue verified in 72.0.3626.14
,
Dec 11
Able to reproduce the issue on reported chrome version# 71.0.3578.80 using Windows 10, Ubuntu 17.10 and Mac 10.13.6 by following steps as per comment#0. Verified the fix on Mac 10.13.6, Windows 10 and Ubuntu 17.10 as per comment#0 on latest chrome version #72.0.3626.14 and #73.0.3637.0. Attaching screenshots for reference. Observed an alert being displayed. Hence, the fix is working as expected. Adding the verified labels. Thanks...!!
,
Dec 11
Tested the issue on Chrome version# 71.0.3578.95, fix is working as excepted, hence adding verified labels. Thanks!
,
Dec 11
verified on Webview shell browser with steps mentioned in comment #0 is no longer reproducible on Nexus 6P/O having 71.0.3578.95
,
Dec 12
,
Dec 12
What about adding a test (ideally, a web-platform-test) for this? Seems crucial to me.
,
Dec 12
Why not! Let me write a WPT.
,
Dec 12
Android: Works as per expected behavior, issue verified in 71.0.3578.98
,
Dec 12
Good news: 71.0.3578.98 is now available with the fix on Chrome Desktop Stable. You can force an update by visiting chrome://chrome. Thank you.
,
Dec 13
verified on Webview shell browser with steps mentioned in comment #0 is no longer reproducible on Pixel2/P having 73.0.3639.0
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa Commit: 9bea27c0383b8f83df4b90c9c59e81c7a62bf5fa Author: yukishiino@chromium.org Commiter: govind@chromium.org Date: 2018-12-10 21:20:12 +0000 UTC v8binding: Compile event handlers with currentTarget's scope https://crrev.com/c/1215512 mistakenly made content attribute's event handlers get compiled in the scope of event.target instead of the EventTarget where the event handler is registered. This patch fixes it so that event handlers correctly get compiled in the scope of the EventTarget where the event handler is registered, which is event->currentTarget() when dispatching an event. Bug: 913310 , 891635 Change-Id: I6f1e5d6f3a44a10837acec572d82ed788f00af2c Reviewed-on: https://chromium-review.googlesource.com/c/1370144 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615116}(cherry picked from commit 7bda23a6a30fbc032054da4426cd93070907b290) Reviewed-on: https://chromium-review.googlesource.com/c/1370489 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#236} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Dec 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eac2c7df93551c35ff63a5fae317d58d729e7823 commit eac2c7df93551c35ff63a5fae317d58d729e7823 Author: Yuki Shiino <yukishiino@chromium.org> Date: Tue Dec 25 07:44:19 2018 v8binding: Add lexical scope test of event handlers in WPT Adds a new web-platform test to check lexical scopes of the event handlers compiled from content attributes. Bug: 913310 Change-Id: I0037901622a03a8fff14dc33a3db73265de09ab0 Reviewed-on: https://chromium-review.googlesource.com/c/1388073 Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#618854} [add] https://crrev.com/eac2c7df93551c35ff63a5fae317d58d729e7823/third_party/blink/web_tests/external/wpt/html/webappapis/scripting/events/compile-event-handler-lexical-scopes.html
,
Dec 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79408d7bb0fc7035ae34bd18fa97bbdf3349236e commit 79408d7bb0fc7035ae34bd18fa97bbdf3349236e Author: Yuki Shiino <yukishiino@chromium.org> Date: Fri Dec 28 04:46:25 2018 v8binding: EventTarget must be Element or Window if content attributes After the fix of crbug.com/913310 , in case of content attributes, EventTarget must be either of Element or Window. This patch checks the condition accordingly. Bug: 913310 Change-Id: I1a70b60355dd11c1395c649ae1fe5a6a2b8a2781 Reviewed-on: https://chromium-review.googlesource.com/c/1390874 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#619116} [modify] https://crrev.com/79408d7bb0fc7035ae34bd18fa97bbdf3349236e/third_party/blink/renderer/bindings/core/v8/js_event_handler_for_content_attribute.cc [modify] https://crrev.com/79408d7bb0fc7035ae34bd18fa97bbdf3349236e/third_party/blink/renderer/bindings/core/v8/sanitize_script_errors.h |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by woxxom@gmail.com
, Dec 10