Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2826393004: v8binding: Makes Location's wrapper objects alive, really. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by Yuki(ooo_til_may8)
Modified:
3 days, 9 hours ago
CC:
chromium-reviews, blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-bindings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

v8binding: Makes Location's wrapper objects alive, really. https://crbug.com/252482 demonstrates that a) V8HTMLDocument's locationAttributeGetter is NOT using a private property to keep the location's wrapper alive. b) Location.idl does NOT specify [DependentLifetime]. c) V8 minor GC can collect a wrapper object of document.location if author script has no reference to it. d) V8Window::locationAttributeGetterCustom is using a private property to keep it alive, but it may be too late. At c), V8 may have already collected the location's wrapper object, and expandos may have been gone. The direct cause is that 1) There are two paths to create a Location's wrapper object; window.location and document.location. 2) document.location doesn't use a private property (keep_alive) though window.location uses it. This CL makes the following changes. i) Uses the wrapper tracing in order to make Location's wrapper objects alive. ii) Makes Location [DependentLifetime] so that the wrapper tracing works. BUG=252482 Review-Url: https://codereview.chromium.org/2826393004 Cr-Commit-Position: refs/heads/master@{#467313} Committed: https://chromium.googlesource.com/chromium/src/+/8e12aef305793ef5b7689939faa027ce3be906da

Patch Set 1 #

Total comments: 2

Patch Set 2 : Addressed a review comment. #

Patch Set 3 : Added a layout test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -15 lines) Patch
A third_party/WebKit/LayoutTests/bindings/location-lifetime.html View 1 2 1 chunk +28 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp View 1 2 chunks +1 line, -13 lines 0 comments Download
M third_party/WebKit/Source/core/frame/DOMWindow.h View 3 chunks +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/DOMWindow.cpp View 1 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Location.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/Location.idl View 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 23 (13 generated)
Yuki(ooo_til_may8)
Could you review this CL? This is my first time to use the wrapper tracing, ...
1 week, 1 day ago (2017-04-21 11:22:53 UTC) #4
haraken
LGTM We're planning to replace all keep-alive-for-gc logics with the wrapper tracing, just like this ...
1 week, 1 day ago (2017-04-21 11:49:52 UTC) #6
Michael Lippautz
lgtm On 2017/04/21 11:49:52, haraken wrote: > LGTM > > We're planning to replace all ...
1 week, 1 day ago (2017-04-21 12:13:17 UTC) #7
haraken
Can we add tests? You could use window.internals.GCObservation.
1 week, 1 day ago (2017-04-21 14:48:16 UTC) #10
jbroman
On 2017/04/21 at 14:48:16, haraken wrote: > Can we add tests? You could use window.internals.GCObservation. ...
1 week, 1 day ago (2017-04-21 17:19:08 UTC) #11
haraken
On 2017/04/21 17:19:08, jbroman wrote: > On 2017/04/21 at 14:48:16, haraken wrote: > > Can ...
1 week, 1 day ago (2017-04-21 19:11:05 UTC) #12
Yuki(ooo_til_may8)
Added a layout test. Confirmed a failure in case of document.location on ToT (w/o the ...
3 days, 11 hours ago (2017-04-26 12:10:11 UTC) #15
haraken
On 2017/04/26 12:10:11, Yuki wrote: > Added a layout test. Confirmed a failure in case ...
3 days, 11 hours ago (2017-04-26 12:11:44 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2826393004/40001
3 days, 10 hours ago (2017-04-26 12:32:53 UTC) #20
commit-bot: I haz the power
3 days, 9 hours ago (2017-04-26 13:56:50 UTC) #23
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8e12aef305793ef5b7689939faa0...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46