Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(139)

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

Created:
3 years, 8 months ago by Yuki
Modified:
3 years, 8 months 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, ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-21 12:13:17 UTC) #7
haraken
Can we add tests? You could use window.internals.GCObservation.
3 years, 8 months 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. ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months 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 years, 8 months 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 years, 8 months ago (2017-04-26 12:32:53 UTC) #20
commit-bot: I haz the power
3 years, 8 months 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 408576698