Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
391373009:
[oilpan]: Fix webkit unit tests for oilpan. (Closed)
|
Created:
6 years, 5 months ago by wibling-chromium Modified:
6 years, 5 months ago Reviewers:
Vyacheslav Egorov (Chromium), jochen (gone - plz use gerrit), Sami, Mads Ager (chromium), tkent, zerny-chromium, Erik Corry, Rick Byers, haraken, oilpan-reviews CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[oilpan]: Fix webkit unit tests for oilpan.
We have to force a GC to ensure the registered event handler are removed. This happens during weak processing with oilpan.
R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, jochen@chromium.org, oilpan-reviews@chromium.org, rbyers@chromium.org, skyostil@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178378
Patch Set 1 #
Total comments: 4
Patch Set 2 : extend comment #Patch Set 3 : use collectAllGarbage #Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... Source/web/tests/WebViewTest.cpp:1812: RefPtrWillBePersistent<WebCore::Document> document = webViewImpl->mainFrameImpl()->frame()->document(); why this change?
https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... Source/web/tests/WebViewTest.cpp:1812: RefPtrWillBePersistent<WebCore::Document> document = webViewImpl->mainFrameImpl()->frame()->document(); On 2014/07/17 14:17:24, jochen wrote: > why this change? Because I am doing a GC with no pointers on stack which means any object on the stack will not be traced. By putting the Document into a Persistent I am guaranteed the registry is still around after my GC.
On 2014/07/17 at 14:18:51, wibling wrote: > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > File Source/web/tests/WebViewTest.cpp (right): > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > Source/web/tests/WebViewTest.cpp:1812: RefPtrWillBePersistent<WebCore::Document> document = webViewImpl->mainFrameImpl()->frame()->document(); > On 2014/07/17 14:17:24, jochen wrote: > > why this change? > > Because I am doing a GC with no pointers on stack which means any object on the stack will not be traced. By putting the Document into a Persistent I am guaranteed the registry is still around after my GC. and all other pointers on the stack happen to stay valid because of the Document staying alive? Why not do a GC with stack scan?
On 2014/07/17 14:22:38, jochen wrote: > On 2014/07/17 at 14:18:51, wibling wrote: > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > File Source/web/tests/WebViewTest.cpp (right): > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > Source/web/tests/WebViewTest.cpp:1812: > RefPtrWillBePersistent<WebCore::Document> document = > webViewImpl->mainFrameImpl()->frame()->document(); > > On 2014/07/17 14:17:24, jochen wrote: > > > why this change? > > > > Because I am doing a GC with no pointers on stack which means any object on > the stack will not be traced. By putting the Document into a Persistent I am > guaranteed the registry is still around after my GC. > > and all other pointers on the stack happen to stay valid because of the Document > staying alive? Yes, since they are retrieved from the document. > Why not do a GC with stack scan? If I do then the div element will also be kept alive since there is a raw pointer to it on the stack.
On 2014/07/17 14:25:10, wibling-chromium wrote: > On 2014/07/17 14:22:38, jochen wrote: > > On 2014/07/17 at 14:18:51, wibling wrote: > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > File Source/web/tests/WebViewTest.cpp (right): > > > > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > Source/web/tests/WebViewTest.cpp:1812: > > RefPtrWillBePersistent<WebCore::Document> document = > > webViewImpl->mainFrameImpl()->frame()->document(); > > > On 2014/07/17 14:17:24, jochen wrote: > > > > why this change? > > > > > > Because I am doing a GC with no pointers on stack which means any object on > > the stack will not be traced. By putting the Document into a Persistent I am > > guaranteed the registry is still around after my GC. > > > > and all other pointers on the stack happen to stay valid because of the > Document > > staying alive? > > Yes, since they are retrieved from the document. > > > Why not do a GC with stack scan? > > If I do then the div element will also be kept alive since there is a raw > pointer to it on the stack. I could clear the pointer before doing a conservative GC, but we tend to lean towards non-conservative GCs in tests since there could be a value on the stack that would incorrectly cause an element to be kept alive that we want to die.
On 2014/07/17 at 14:27:03, wibling wrote: > On 2014/07/17 14:25:10, wibling-chromium wrote: > > On 2014/07/17 14:22:38, jochen wrote: > > > On 2014/07/17 at 14:18:51, wibling wrote: > > > > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > > File Source/web/tests/WebViewTest.cpp (right): > > > > > > > > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > > Source/web/tests/WebViewTest.cpp:1812: > > > RefPtrWillBePersistent<WebCore::Document> document = > > > webViewImpl->mainFrameImpl()->frame()->document(); > > > > On 2014/07/17 14:17:24, jochen wrote: > > > > > why this change? > > > > > > > > Because I am doing a GC with no pointers on stack which means any object on > > > the stack will not be traced. By putting the Document into a Persistent I am > > > guaranteed the registry is still around after my GC. > > > > > > and all other pointers on the stack happen to stay valid because of the > > Document > > > staying alive? > > > > Yes, since they are retrieved from the document. > > > > > Why not do a GC with stack scan? > > > > If I do then the div element will also be kept alive since there is a raw > > pointer to it on the stack. > > I could clear the pointer before doing a conservative GC, but we tend to lean towards non-conservative GCs in tests since there could be a value on the stack that would incorrectly cause an element to be kept alive that we want to die. ok Could you extend the comment before the GC explaining that it's important that the div can die but the document stays alive or something? with that lgtm
On 2014/07/17 14:28:58, jochen wrote: > On 2014/07/17 at 14:27:03, wibling wrote: > > On 2014/07/17 14:25:10, wibling-chromium wrote: > > > On 2014/07/17 14:22:38, jochen wrote: > > > > On 2014/07/17 at 14:18:51, wibling wrote: > > > > > > > > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > > > File Source/web/tests/WebViewTest.cpp (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... > > > > > Source/web/tests/WebViewTest.cpp:1812: > > > > RefPtrWillBePersistent<WebCore::Document> document = > > > > webViewImpl->mainFrameImpl()->frame()->document(); > > > > > On 2014/07/17 14:17:24, jochen wrote: > > > > > > why this change? > > > > > > > > > > Because I am doing a GC with no pointers on stack which means any object > on > > > > the stack will not be traced. By putting the Document into a Persistent I > am > > > > guaranteed the registry is still around after my GC. > > > > > > > > and all other pointers on the stack happen to stay valid because of the > > > Document > > > > staying alive? > > > > > > Yes, since they are retrieved from the document. > > > > > > > Why not do a GC with stack scan? > > > > > > If I do then the div element will also be kept alive since there is a raw > > > pointer to it on the stack. > > > > I could clear the pointer before doing a conservative GC, but we tend to lean > towards non-conservative GCs in tests since there could be a value on the stack > that would incorrectly cause an element to be kept alive that we want to die. > > ok > > Could you extend the comment before the GC explaining that it's important that > the div can die but the document stays alive or something? > > with that lgtm Sure, will do.
LGTM https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... Source/web/tests/WebViewTest.cpp:1824: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); We prefer using Heap::collectAllGarbage() in tests.
Thanks for the reviews! https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/391373009/diff/1/Source/web/tests/WebViewTest... Source/web/tests/WebViewTest.cpp:1824: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); On 2014/07/17 14:32:28, haraken wrote: > > We prefer using Heap::collectAllGarbage() in tests. Done.
The CQ bit was checked by wibling@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/391373009/40001
Message was sent while issue was closed.
Change committed as 178378 |
