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

Issue 235113002: Oilpan: Remove guardRef and guardDeref from TreeScope. (Closed)

Created:
6 years, 8 months ago by Mads Ager (chromium)
Modified:
6 years, 8 months ago
CC:
blink-reviews, shans, webcomponents-bugzilla_chromium.org, eae+blinkwatch, fs, apavlov+blink_chromium.org, adamk+blink_chromium.org, aandrey+blink_chromium.org, dino_apple.com, rwlbuis, jamesr, krit, arv+blink, aboxhall, dsinclair, Timothy Loh, danakj, dstockwell, dglazkov+blink, Rik, pdr., rune+blink, Eric Willigers, Steve Block, rjwright, sof, jbroman, dmazzoni, gyuyoung.kim_webkit.org, darktears, gavinp+prerender_chromium.org, alancutter (OOO until 2018), kouhei+svg_chromium.org, Mike Lawther (Google), ed+blinkwatch_opera.com, f(malita), Inactive, Stephen Chennney, watchdog-blink-watchlist_google.com
Visibility:
Public.

Description

Oilpan: Remove guardRef and guardDeref from TreeScope. This leads to a situation where Nodes and their TreeScope can die at the same time and Nodes can no longer touch their document in their destructors. Instead we now have the following property: A Node is either explicitly removed from its container and |removedFrom| is called at that point, or the Node and the TreeScope die at the same time. This allows us to remove code from destructors that is only there to clean up the TreeScope if the object was not explicitly removed from the TreeScope. Other code in destructors that go through the TreeScope to carry something out is replaced by weak processing instead. BUG=357163 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172746

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rebase, fix leak via stylesheet lists, fix delay of load event. #

Patch Set 3 : Rebase again. #

Total comments: 48

Patch Set 4 : Address review comments. #

Total comments: 27

Patch Set 5 : Address comments. #

Total comments: 53

Patch Set 6 : Address review comments. #

Patch Set 7 : Only perform weak processing of the event handler registry if the document is active. #

Total comments: 34

Patch Set 8 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+501 lines, -146 lines) Patch
M Source/core/accessibility/AXObjectCache.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/accessibility/AXObjectCache.cpp View 1 1 chunk +12 lines, -0 lines 0 comments Download
M Source/core/css/StyleSheetList.h View 1 2 chunks +7 lines, -2 lines 0 comments Download
M Source/core/css/StyleSheetList.cpp View 1 2 3 3 chunks +8 lines, -7 lines 0 comments Download
M Source/core/dom/ContainerNode.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 3 4 5 6 7 6 chunks +9 lines, -5 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 9 chunks +71 lines, -4 lines 0 comments Download
M Source/core/dom/DocumentMarkerController.h View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/dom/DocumentMarkerController.cpp View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/dom/DocumentTest.cpp View 1 chunk +51 lines, -40 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/dom/EventHandlerRegistry.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/EventHandlerRegistry.cpp View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
M Source/core/dom/Node.h View 1 2 3 4 5 6 7 5 chunks +8 lines, -2 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 2 3 4 5 6 7 8 chunks +27 lines, -3 lines 0 comments Download
M Source/core/dom/ProcessingInstruction.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/dom/StyleEngine.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/dom/StyleEngine.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/dom/TreeScope.h View 1 2 3 6 chunks +12 lines, -38 lines 0 comments Download
M Source/core/dom/TreeScope.cpp View 1 2 3 4 5 chunks +19 lines, -1 line 0 comments Download
M Source/core/dom/TreeScopeAdopter.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/TreeScopeAdopter.cpp View 3 chunks +6 lines, -0 lines 0 comments Download
M Source/core/dom/shadow/ShadowRoot.h View 1 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/dom/shadow/ShadowRoot.cpp View 1 2 3 4 5 6 7 3 chunks +12 lines, -1 line 0 comments Download
M Source/core/dom/shadow/ShadowRootRareData.h View 1 2 chunks +4 lines, -2 lines 0 comments Download
M Source/core/html/HTMLCanvasElement.h View 1 2 3 chunks +4 lines, -1 line 0 comments Download
M Source/core/html/HTMLCanvasElement.cpp View 1 2 3 4 5 6 7 2 chunks +8 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFormElement.cpp View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M Source/core/html/HTMLInputElement.cpp View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLLinkElement.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 7 6 chunks +20 lines, -5 lines 0 comments Download
M Source/core/html/HTMLObjectElement.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLStyleElement.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext.h View 3 chunks +11 lines, -3 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.h View 1 2 3 4 5 3 chunks +7 lines, -7 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 3 4 5 2 chunks +11 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.idl View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/html/canvas/WebGLRenderingContext.h View 1 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContext.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContext.idl View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContextBase.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/html/canvas/WebGLRenderingContextBase.cpp View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/html/imports/HTMLImportChild.h View 1 2 3 4 5 2 chunks +5 lines, -0 lines 0 comments Download
M Source/core/html/imports/HTMLImportChild.cpp View 1 2 chunks +8 lines, -0 lines 0 comments Download
M Source/core/html/shadow/DateTimeEditElement.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/shadow/TextControlInnerElements.cpp View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/page/DOMSelection.h View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/page/DOMSelection.cpp View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/FocusController.h View 1 2 3 4 5 3 chunks +3 lines, -1 line 0 comments Download
M Source/core/page/Page.h View 1 2 3 4 5 6 4 chunks +4 lines, -3 lines 0 comments Download
M Source/core/page/Page.cpp View 1 2 3 4 5 3 chunks +12 lines, -2 lines 0 comments Download
M Source/core/speech/SpeechInput.h View 1 2 3 4 5 3 chunks +8 lines, -2 lines 0 comments Download
M Source/core/speech/SpeechInput.cpp View 3 chunks +17 lines, -2 lines 0 comments Download
M Source/core/svg/SVGElement.cpp View 1 1 chunk +4 lines, -1 line 0 comments Download
M Source/core/svg/SVGFEImageElement.cpp View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/svg/SVGMPathElement.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGSVGElement.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/svg/SVGStyleElement.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/svg/SVGTextPathElement.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGUseElement.cpp View 1 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/svg/animation/SVGSMILElement.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/platform/graphics/GraphicsContext.cpp View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Mads Ager (chromium)
I'm about to go on vacation and this one depends on the Page change so ...
6 years, 8 months ago (2014-04-11 12:37:55 UTC) #1
tkent
I checked HTMLFormElement, HTMLInputElement, BaseMultipleFieldsDateAndTimeInputType, DateTimeEditElement, TextControlInnerElements, and SpeechInput. The changes to them look reasonable.
6 years, 8 months ago (2014-04-21 00:07:52 UTC) #2
Mads Ager (chromium)
Found a couple of extra issues after rebasing. Fixed and ready for review.
6 years, 8 months ago (2014-04-23 13:33:19 UTC) #3
tkent
lgtm https://codereview.chromium.org/235113002/diff/40001/Source/core/dom/TreeScope.h File Source/core/dom/TreeScope.h (right): https://codereview.chromium.org/235113002/diff/40001/Source/core/dom/TreeScope.h#newcode114 Source/core/dom/TreeScope.h:114: #if ENABLE(OILPAN) Here is in !ENABLE(OILPAN) https://codereview.chromium.org/235113002/diff/40001/Source/core/dom/TreeScope.h#newcode128 Source/core/dom/TreeScope.h:128: ...
6 years, 8 months ago (2014-04-24 01:20:23 UTC) #4
haraken
This is a great and complicated CL! https://codereview.chromium.org/235113002/diff/40001/Source/core/css/StyleSheetList.cpp File Source/core/css/StyleSheetList.cpp (right): https://codereview.chromium.org/235113002/diff/40001/Source/core/css/StyleSheetList.cpp#newcode73 Source/core/css/StyleSheetList.cpp:73: if (!m_treeScope) ...
6 years, 8 months ago (2014-04-24 04:18:42 UTC) #5
Mads Ager (chromium)
Thanks for the careful reviews! Ready for another round. :) https://codereview.chromium.org/235113002/diff/40001/Source/core/css/StyleSheetList.cpp File Source/core/css/StyleSheetList.cpp (right): https://codereview.chromium.org/235113002/diff/40001/Source/core/css/StyleSheetList.cpp#newcode73 ...
6 years, 8 months ago (2014-04-24 10:57:35 UTC) #6
haraken
Thanks for the update! Let me take another close look tomorrow morning.
6 years, 8 months ago (2014-04-24 11:59:11 UTC) #7
Mads Ager (chromium)
On 2014/04/24 11:59:11, haraken wrote: > Thanks for the update! Let me take another close ...
6 years, 8 months ago (2014-04-24 12:18:26 UTC) #8
Erik Corry
I got to shadowRoot.h https://codereview.chromium.org/235113002/diff/60001/Source/core/accessibility/AXObjectCache.h File Source/core/accessibility/AXObjectCache.h (right): https://codereview.chromium.org/235113002/diff/60001/Source/core/accessibility/AXObjectCache.h#newcode219 Source/core/accessibility/AXObjectCache.h:219: HashSet<Node*> m_textMarkerNodes; I wonder who ...
6 years, 8 months ago (2014-04-24 13:53:15 UTC) #9
Mads Ager (chromium)
Thanks Erik! https://codereview.chromium.org/235113002/diff/60001/Source/core/accessibility/AXObjectCache.cpp File Source/core/accessibility/AXObjectCache.cpp (right): https://codereview.chromium.org/235113002/diff/60001/Source/core/accessibility/AXObjectCache.cpp#newcode541 Source/core/accessibility/AXObjectCache.cpp:541: removeNodeForUse(node); This is where it is cleared ...
6 years, 8 months ago (2014-04-24 14:12:13 UTC) #10
wibling-chromium
lgtm https://codereview.chromium.org/235113002/diff/60001/Source/core/html/HTMLFormElement.cpp File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/235113002/diff/60001/Source/core/html/HTMLFormElement.cpp#newcode89 Source/core/html/HTMLFormElement.cpp:89: document().formController().willDeleteForm(this); Perhaps augment the comment to say that ...
6 years, 8 months ago (2014-04-24 14:43:04 UTC) #11
haraken
This will be a final round of comments. https://codereview.chromium.org/235113002/diff/80001/Source/core/accessibility/AXObjectCache.h File Source/core/accessibility/AXObjectCache.h (right): https://codereview.chromium.org/235113002/diff/80001/Source/core/accessibility/AXObjectCache.h#newcode213 Source/core/accessibility/AXObjectCache.h:213: Document& ...
6 years, 8 months ago (2014-04-25 05:21:41 UTC) #12
haraken
https://codereview.chromium.org/235113002/diff/80001/Source/core/dom/ProcessingInstruction.cpp File Source/core/dom/ProcessingInstruction.cpp (right): https://codereview.chromium.org/235113002/diff/80001/Source/core/dom/ProcessingInstruction.cpp#newcode59 Source/core/dom/ProcessingInstruction.cpp:59: m_sheet->clearOwnerNode(); On 2014/04/25 05:21:42, haraken wrote: > > You ...
6 years, 8 months ago (2014-04-25 05:29:58 UTC) #13
Erik Corry
LGTM with nits https://codereview.chromium.org/235113002/diff/80001/Source/core/html/HTMLMediaElement.cpp File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/235113002/diff/80001/Source/core/html/HTMLMediaElement.cpp#newcode337 Source/core/html/HTMLMediaElement.cpp:337: // With Oilpan load events on ...
6 years, 8 months ago (2014-04-25 09:23:47 UTC) #14
Mads Ager (chromium)
Thanks again for the careful reviewing. https://codereview.chromium.org/235113002/diff/60001/Source/core/html/HTMLFormElement.cpp File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/235113002/diff/60001/Source/core/html/HTMLFormElement.cpp#newcode89 Source/core/html/HTMLFormElement.cpp:89: document().formController().willDeleteForm(this); On 2014/04/24 ...
6 years, 8 months ago (2014-04-25 10:58:24 UTC) #15
tkent
https://codereview.chromium.org/235113002/diff/80001/Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right): https://codereview.chromium.org/235113002/diff/80001/Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp#newcode304 Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:304: element->removePickerIndicatorOwner(); On 2014/04/25 10:58:25, Mads Ager (chromium) wrote: > ...
6 years, 8 months ago (2014-04-25 12:51:15 UTC) #16
Mads Ager (chromium)
https://codereview.chromium.org/235113002/diff/80001/Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right): https://codereview.chromium.org/235113002/diff/80001/Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp#newcode304 Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:304: element->removePickerIndicatorOwner(); On 2014/04/25 12:51:16, tkent wrote: > On 2014/04/25 ...
6 years, 8 months ago (2014-04-25 12:57:50 UTC) #17
haraken
Did a full review. LGTM with comments. https://codereview.chromium.org/235113002/diff/120001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/235113002/diff/120001/Source/core/dom/Document.cpp#newcode5639 Source/core/dom/Document.cpp:5639: // perform ...
6 years, 8 months ago (2014-04-25 14:30:31 UTC) #18
Mads Ager (chromium)
Thanks Haraken! I'm going to commit this and continue working on the visiting of the ...
6 years, 8 months ago (2014-04-28 09:45:20 UTC) #19
Mads Ager (chromium)
The CQ bit was checked by ager@chromium.org
6 years, 8 months ago (2014-04-28 09:46:14 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/235113002/140001
6 years, 8 months ago (2014-04-28 09:46:31 UTC) #21
haraken
Thanks for the detailed replies. All of them make sense. I believe that your upcoming ...
6 years, 8 months ago (2014-04-28 10:17:28 UTC) #22
commit-bot: I haz the power
6 years, 8 months ago (2014-04-28 11:03:06 UTC) #23
Message was sent while issue was closed.
Change committed as 172746

Powered by Google App Engine
This is Rietveld 408576698