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

Issue 569643002: Simplify and weaken DOMWindowProperty unregistration (Closed)

Created:
6 years, 3 months ago by sof
Modified:
6 years, 3 months ago
CC:
blink-reviews
Project:
blink
Visibility:
Public.

Description

Simplify and weaken DOMWindowProperty unregistration A DOMWindowProperty object should not be kept alive if it is only referenced from a LocalDOMWindow's set of registered DOMWindowProperty objects. Adjust LocalDOMWindow's DOMObjectProperty references to be weak for Oilpan also. In most cases, DOMWindowProperty objects will remain alive for at least as long as their associated LocalDOMWindow, but this cannot be assumed. The added test checks for that wrt DOMSelection objects created from shadow roots. Along with adjusting the weakness of these references, simplify how LocalDOMWindow unregisters its DOMWindowProperty objects. That is, no longer have the instances unregister themselves, but let the LocalDOMWindow remove them all after having notified them of impending destruction. R=haraken,ager BUG=413316 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181970

Patch Set 1 #

Patch Set 2 : Simplify DOMWindowProperty notification steps #

Patch Set 3 : Add test #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -23 lines) Patch
A LayoutTests/fast/dom/shadow/shadow-selection-detach-crash.html View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
A + LayoutTests/fast/dom/shadow/shadow-selection-detach-crash-expected.txt View 1 2 1 chunk +2 lines, -3 lines 0 comments Download
M Source/core/frame/DOMWindowProperty.cpp View 1 2 chunks +4 lines, -7 lines 0 comments Download
M Source/core/frame/LocalDOMWindow.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/frame/LocalDOMWindow.cpp View 1 1 chunk +4 lines, -12 lines 4 comments Download

Messages

Total messages: 11 (3 generated)
sof
Please take a look. Follow up and weaken what was reverted in https://codereview.chromium.org/558213002/ The assumption ...
6 years, 3 months ago (2014-09-14 16:02:27 UTC) #2
haraken
LGTM
6 years, 3 months ago (2014-09-15 05:08:52 UTC) #3
Mads Ager (chromium)
https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp File Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp#oldcode561 Source/core/frame/LocalDOMWindow.cpp:561: WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties; Do you still need the copy ...
6 years, 3 months ago (2014-09-15 07:58:22 UTC) #5
sof
https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp File Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp#oldcode561 Source/core/frame/LocalDOMWindow.cpp:561: WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties; On 2014/09/15 07:58:21, Mads Ager (chromium) ...
6 years, 3 months ago (2014-09-15 08:10:25 UTC) #6
Mads Ager (chromium)
LGTM https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp File Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/569643002/diff/40001/Source/core/frame/LocalDOMWindow.cpp#oldcode561 Source/core/frame/LocalDOMWindow.cpp:561: WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties; On 2014/09/15 08:10:25, sof wrote: ...
6 years, 3 months ago (2014-09-15 08:11:40 UTC) #7
sof
Thanks for the reviews. Should there be unexpected problems with the unregistration simplifications here, will ...
6 years, 3 months ago (2014-09-15 08:26:32 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/569643002/40001
6 years, 3 months ago (2014-09-15 08:27:45 UTC) #10
commit-bot: I haz the power
6 years, 3 months ago (2014-09-15 09:59:46 UTC) #11
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 181970

Powered by Google App Engine
This is Rietveld 408576698