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

Issue 292503006: Oilpan: add [WillBeGarbageCollected] for Node. (Closed)

Created:
5 years ago by sof
Modified:
5 years ago
CC:
blink-reviews, arv+blink, eae+blinkwatch, abarth-chromium, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, watchdog-blink-watchlist_google.com, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Oilpan: add [WillBeGarbageCollected] for Node. Also make Document hold onto its DOMWindow via a Member when Oilpan is enabled. R=haraken@chromium.org,ager@chromium.org,tkent@chromium.org BUG=357163 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174804

Patch Set 1 #

Patch Set 2 : Pull in ImageBitmapTest fix for easier verification of CL #

Patch Set 3 : Rebased + use transition types in XSLTProcessor #

Total comments: 2

Patch Set 4 : DOMWindow dtor fixes #

Total comments: 4

Patch Set 5 : Have Document keep a weak reference to its DOMWindow #

Patch Set 6 : Rebased #

Total comments: 5

Patch Set 7 : Clear document touch event handlers upon losing DOMWindow. #

Total comments: 3

Patch Set 8 : Tidy up removeAllEventListeners() usage from DOMWindow dtor #

Total comments: 10

Patch Set 9 : Add FIXME + tighten up asserts #

Patch Set 10 : Fix unit test detaching document from window #

Total comments: 1

Patch Set 11 : Rebased #

Total comments: 2

Patch Set 12 : Make Document strongly refer to its DOMWindow #

Patch Set 13 : Add now-required tracing of m_domWindow (oops) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -37 lines) Patch
M Source/core/dom/Document.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -4 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +13 lines, -2 lines 0 comments Download
M Source/core/dom/DocumentFragment.idl View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/dom/Element.idl View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/dom/NamedNodeMap.h View 1 chunk +7 lines, -7 lines 0 comments Download
M Source/core/dom/NamedNodeMap.cpp View 4 chunks +7 lines, -7 lines 0 comments Download
M Source/core/dom/Node.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/DOMWindow.h View 1 2 3 4 5 6 7 8 1 chunk +13 lines, -1 line 0 comments Download
M Source/core/frame/DOMWindow.cpp View 1 2 3 4 5 6 7 8 4 chunks +24 lines, -11 lines 0 comments Download
M Source/web/tests/ScrollingCoordinatorChromiumTest.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 48 (0 generated)
haraken
LGTM
5 years ago (2014-05-22 20:07:52 UTC) #1
tkent
+oilpan-reviews
5 years ago (2014-05-23 06:22:36 UTC) #2
Mads Ager (chromium)
https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWindow.cpp#newcode490 Source/core/frame/DOMWindow.cpp:490: clearDocument(); Why do we have this call to clearDocument ...
5 years ago (2014-05-23 06:27:29 UTC) #3
sof
https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWindow.cpp#newcode490 Source/core/frame/DOMWindow.cpp:490: clearDocument(); On 2014/05/23 06:27:29, Mads Ager (chromium) wrote: > ...
5 years ago (2014-05-23 06:32:58 UTC) #4
sof
Updated and ready for a closer look. (Will wait for https://codereview.chromium.org/288343017/ to land before taking ...
5 years ago (2014-05-23 07:31:00 UTC) #5
haraken
The destruction issue of DOMWindow looks hard. https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp#newcode368 Source/core/frame/DOMWindow.cpp:368: RefPtrWillBeRawPtr<Document> document; ...
5 years ago (2014-05-23 08:32:13 UTC) #6
sof
https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp#newcode498 Source/core/frame/DOMWindow.cpp:498: clearEventQueue(); On 2014/05/23 08:32:14, haraken wrote: > > I ...
5 years ago (2014-05-23 08:56:33 UTC) #7
sof
Rebased on top of http://crrev.com/288343017 https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWindow.cpp#newcode498 Source/core/frame/DOMWindow.cpp:498: clearEventQueue(); On 2014/05/23 08:32:14, ...
5 years ago (2014-05-23 10:53:03 UTC) #8
haraken
> > I also wonder why we don't need to call Document::detach() in a case ...
5 years ago (2014-05-23 12:23:26 UTC) #9
sof
haraken, thanks - awesome comments as always :) https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Document.cpp#newcode5724 Source/core/dom/Document.cpp:5724: m_domWindow.clear(); ...
5 years ago (2014-05-23 14:29:07 UTC) #10
sof
On 2014/05/23 12:23:26, haraken wrote: > > > I also wonder why we don't need ...
5 years ago (2014-05-23 14:30:32 UTC) #11
haraken
https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Document.cpp#newcode5724 Source/core/dom/Document.cpp:5724: m_domWindow.clear(); On 2014/05/23 14:29:07, sof wrote: > On 2014/05/23 ...
5 years ago (2014-05-23 15:02:44 UTC) #12
sof
https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWindow.cpp#newcode497 Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); On 2014/05/23 15:02:45, haraken wrote: > > Instead ...
5 years ago (2014-05-23 15:30:30 UTC) #13
haraken
On 2014/05/23 15:30:30, sof wrote: > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWindow.cpp > File Source/core/frame/DOMWindow.cpp (right): > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWindow.cpp#newcode497 > ...
5 years ago (2014-05-23 15:44:13 UTC) #14
haraken
> ~DOMWindow() { > #if ENABLE(OILPAN) > // FIXME: Oilpan: Should be removed once EventTarget ...
5 years ago (2014-05-23 15:50:26 UTC) #15
sof
On 2014/05/23 15:44:13, haraken wrote: > On 2014/05/23 15:30:30, sof wrote: > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWindow.cpp ...
5 years ago (2014-05-23 15:52:56 UTC) #16
haraken
On 2014/05/23 15:52:56, sof wrote: > On 2014/05/23 15:44:13, haraken wrote: > > On 2014/05/23 ...
5 years ago (2014-05-23 15:56:59 UTC) #17
sof
On 2014/05/23 15:56:59, haraken wrote: > On 2014/05/23 15:52:56, sof wrote: > > On 2014/05/23 ...
5 years ago (2014-05-23 20:20:27 UTC) #18
haraken
Thanks for the update! The last remaining question to me is m_document->detach(). https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp ...
5 years ago (2014-05-23 20:53:57 UTC) #19
sof
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp#newcode348 Source/core/frame/DOMWindow.cpp:348: m_document->detach(); On 2014/05/23 20:53:58, haraken wrote: > > I'm ...
5 years ago (2014-05-23 21:00:15 UTC) #20
haraken
> Well observed that host() and its event registry are also GCed objects; I wonder ...
5 years ago (2014-05-23 21:03:35 UTC) #21
sof
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp#oldcode495 Source/core/frame/DOMWindow.cpp:495: ASSERT(m_document->isDisposed()); On 2014/05/23 20:53:58, haraken wrote: > > Just ...
5 years ago (2014-05-24 05:50:59 UTC) #22
sof
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWindow.cpp#oldcode495 Source/core/frame/DOMWindow.cpp:495: ASSERT(m_document->isDisposed()); On 2014/05/24 05:51:00, sof wrote: > On 2014/05/23 ...
5 years ago (2014-05-24 06:53:10 UTC) #23
sof
https://codereview.chromium.org/292503006/diff/170001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/170001/Source/core/frame/DOMWindow.cpp#newcode499 Source/core/frame/DOMWindow.cpp:499: ASSERT(!m_eventQueue); This assert showed up a unit test not ...
5 years ago (2014-05-24 08:06:26 UTC) #24
haraken
Thanks for the detailed explanation. Now I'm convinced of your reasoning about DOMWindow shutdown. LGTM, ...
5 years ago (2014-05-26 00:58:19 UTC) #25
sof
On 2014/05/26 00:58:19, haraken wrote: > Thanks for the detailed explanation. Now I'm convinced of ...
5 years ago (2014-05-26 05:23:02 UTC) #26
Mads Ager (chromium)
https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.cpp#newcode5749 Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); This seems fishy to me. If the Document ...
5 years ago (2014-05-26 06:15:19 UTC) #27
haraken
On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.cpp > File Source/core/dom/Document.cpp (right): > > ...
5 years ago (2014-05-26 07:33:49 UTC) #28
Mads Ager (chromium)
On 2014/05/26 07:33:49, haraken wrote: > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 07:38:02 UTC) #29
sof
On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > On 2014/05/26 07:33:49, haraken wrote: > > ...
5 years ago (2014-05-26 07:44:39 UTC) #30
haraken
On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > On 2014/05/26 07:33:49, haraken wrote: > > ...
5 years ago (2014-05-26 07:48:59 UTC) #31
zerny-chromium
On 2014/05/26 07:48:59, haraken wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 07:53:47 UTC) #32
Mads Ager (chromium)
On 2014/05/26 07:48:59, haraken wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 07:57:48 UTC) #33
Mads Ager (chromium)
On 2014/05/26 07:44:39, sof wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 08:00:25 UTC) #34
sof
On 2014/05/26 08:00:25, Mads Ager (chromium) wrote: > On 2014/05/26 07:44:39, sof wrote: > > ...
5 years ago (2014-05-26 08:02:58 UTC) #35
Mads Ager (chromium)
On 2014/05/26 08:02:58, sof wrote: > On 2014/05/26 08:00:25, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 08:09:57 UTC) #36
Mads Ager (chromium)
On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: > On 2014/05/26 07:48:59, haraken wrote: > > ...
5 years ago (2014-05-26 08:23:06 UTC) #37
haraken
On 2014/05/26 08:23:06, Mads Ager (chromium) wrote: > On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: ...
5 years ago (2014-05-26 08:33:45 UTC) #38
sof
On 2014/05/26 08:09:57, Mads Ager (chromium) wrote: > On 2014/05/26 08:02:58, sof wrote: ... > ...
5 years ago (2014-05-26 08:34:32 UTC) #39
sof
On 2014/05/26 08:33:45, haraken wrote: > On 2014/05/26 08:23:06, Mads Ager (chromium) wrote: > > ...
5 years ago (2014-05-26 09:05:17 UTC) #40
haraken
On 2014/05/26 09:05:17, sof wrote: > On 2014/05/26 08:33:45, haraken wrote: > > On 2014/05/26 ...
5 years ago (2014-05-26 09:08:55 UTC) #41
sof
On 2014/05/26 09:08:55, haraken wrote: > On 2014/05/26 09:05:17, sof wrote: ...... > > > ...
5 years ago (2014-05-26 09:26:33 UTC) #42
Mads Ager (chromium)
LGTM Thanks Sigbjørn!
5 years ago (2014-05-26 09:42:38 UTC) #43
tkent
Source/web lgtm
5 years ago (2014-05-26 10:42:50 UTC) #44
sof
The CQ bit was checked by sigbjornf@opera.com
5 years ago (2014-05-26 10:53:08 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/292503006/230001
5 years ago (2014-05-26 10:53:26 UTC) #46
commit-bot: I haz the power
Change committed as 174804
5 years ago (2014-05-26 12:03:16 UTC) #47
Nils Barth (inactive)
5 years ago (2014-05-27 07:32:53 UTC) #48
Message was sent while issue was closed.
On 2014/05/26 12:03:16, I haz the power (commit-bot) wrote:
> Change committed as 174804

FYI (no action necessary):
This change broke run-bindings-tests, due to changing a real IDL file that is
used by test files.
This is ok (we're not going to run r-b-t as PRESUBMIT for changes in core/),
and yhirano@ will rebaseline test results,
but something to be aware of generally.

Powered by Google App Engine
This is Rietveld 408576698