Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(246)

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

Created:
7 months, 3 weeks ago by Yuki
Modified:
7 months, 3 weeks 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

Messages

Total messages: 23 (13 generated)
Yuki
Could you review this CL? This is my first time to use the wrapper tracing, ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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 ...
7 months, 3 weeks ago (2017-04-21 12:13:17 UTC) #7
haraken
Can we add tests? You could use window.internals.GCObservation.
7 months, 3 weeks 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. ...
7 months, 3 weeks 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 ...
7 months, 3 weeks ago (2017-04-21 19:11:05 UTC) #12
Yuki
Added a layout test. Confirmed a failure in case of document.location on ToT (w/o the ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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
7 months, 3 weeks ago (2017-04-26 12:32:53 UTC) #20
commit-bot: I haz the power
7 months, 3 weeks 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...

Powered by Google App Engine
This is Rietveld 0eb02b776