Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306083008:
Fix ScriptState leak in ScriptPromiseProperty unittests. (Closed)
|
Created:
5 years, 3 months ago by yhirano Modified:
5 years, 3 months ago CC:
blink-reviews, vivekg, blink-reviews-bindings_chromium.org, vivekg_samsung Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix ScriptState leak in ScriptPromiseProperty unittests.
BUG=526608
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201468
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Messages
Total messages: 22 (3 generated)
yhirano@chromium.org changed reviewers: + haraken@chromium.org
LGTM
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306083008/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306083008/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201468
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); Incidental to this CL, but having the name of this Heap entry point indicate that it doesn't perform conservative GCs would be helpful, arguably.
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 06:44:40, sof wrote: > Incidental to this CL, but having the name of this Heap entry point indicate > that it doesn't perform conservative GCs would be helpful, arguably. Yeah, a better idea would be to call V8GCController::collectGarbage multiple times in gc(). And don't call Heap::collectAllGarbage() here.
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 06:46:42, haraken wrote: > On 2015/08/31 06:44:40, sof wrote: > > Incidental to this CL, but having the name of this Heap entry point indicate > > that it doesn't perform conservative GCs would be helpful, arguably. > > Yeah, a better idea would be to call V8GCController::collectGarbage multiple > times in gc(). And don't call Heap::collectAllGarbage() here. You have to supply the "force" flag in V8GCController::collectGarbage() somehow though, which isn't done currently.
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 06:50:22, sof wrote: > On 2015/08/31 06:46:42, haraken wrote: > > On 2015/08/31 06:44:40, sof wrote: > > > Incidental to this CL, but having the name of this Heap entry point indicate > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > Yeah, a better idea would be to call V8GCController::collectGarbage multiple > > times in gc(). And don't call Heap::collectAllGarbage() here. > > You have to supply the "force" flag in V8GCController::collectGarbage() somehow > though, which isn't done currently. V8GCController::collectGarbage calls window.gc() and I guess the window.gc() passes the forced flag somehow... (I can't find the call path at the moment though.)
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 06:55:52, haraken wrote: > On 2015/08/31 06:50:22, sof wrote: > > On 2015/08/31 06:46:42, haraken wrote: > > > On 2015/08/31 06:44:40, sof wrote: > > > > Incidental to this CL, but having the name of this Heap entry point > indicate > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage multiple > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > somehow > > though, which isn't done currently. > > V8GCController::collectGarbage calls window.gc() and I guess the window.gc() > passes the forced flag somehow... (I can't find the call path at the moment > though.) > This is a unit test -- isn't "gc()" mapping to what v8 provides via --expose-gc ?
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 06:58:34, sof wrote: > On 2015/08/31 06:55:52, haraken wrote: > > On 2015/08/31 06:50:22, sof wrote: > > > On 2015/08/31 06:46:42, haraken wrote: > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > Incidental to this CL, but having the name of this Heap entry point > > indicate > > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > multiple > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > > somehow > > > though, which isn't done currently. > > > > V8GCController::collectGarbage calls window.gc() and I guess the window.gc() > > passes the forced flag somehow... (I can't find the call path at the moment > > though.) > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via --expose-gc > ? Which maps to v8/src/extensions/gc-extension.* i.e., it will force conservative Oilpan GCs as part of running. Which means that the use of collectAllGarbage() here is redundant + the addition of the Persistent<>s in two tests aren't needed. Right?
Message was sent while issue was closed.
On 2015/08/31 07:03:46, sof wrote: > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: > Heap::collectAllGarbage(); > On 2015/08/31 06:58:34, sof wrote: > > On 2015/08/31 06:55:52, haraken wrote: > > > On 2015/08/31 06:50:22, sof wrote: > > > > On 2015/08/31 06:46:42, haraken wrote: > > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > > Incidental to this CL, but having the name of this Heap entry point > > > indicate > > > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > > multiple > > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > > > somehow > > > > though, which isn't done currently. > > > > > > V8GCController::collectGarbage calls window.gc() and I guess the window.gc() > > > passes the forced flag somehow... (I can't find the call path at the moment > > > though.) > > > > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via > --expose-gc > > ? > > Which maps to v8/src/extensions/gc-extension.* > > i.e., it will force conservative Oilpan GCs as part of running. Which means that > the use of collectAllGarbage() here is redundant + the addition of the > Persistent<>s in two tests aren't needed. Right? Yes, I've just confirmed it :)
Message was sent while issue was closed.
On 2015/08/31 07:04:38, haraken wrote: > On 2015/08/31 07:03:46, sof wrote: > > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > > File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): > > > > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > > Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: > > Heap::collectAllGarbage(); > > On 2015/08/31 06:58:34, sof wrote: > > > On 2015/08/31 06:55:52, haraken wrote: > > > > On 2015/08/31 06:50:22, sof wrote: > > > > > On 2015/08/31 06:46:42, haraken wrote: > > > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > > > Incidental to this CL, but having the name of this Heap entry point > > > > indicate > > > > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > > > multiple > > > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > > > > somehow > > > > > though, which isn't done currently. > > > > > > > > V8GCController::collectGarbage calls window.gc() and I guess the > window.gc() > > > > passes the forced flag somehow... (I can't find the call path at the > moment > > > > though.) > > > > > > > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via > > --expose-gc > > > ? > > > > Which maps to v8/src/extensions/gc-extension.* > > > > i.e., it will force conservative Oilpan GCs as part of running. Which means > that > > the use of collectAllGarbage() here is redundant + the addition of the > > Persistent<>s in two tests aren't needed. Right? > > Yes, I've just confirmed it :) But gc() runs only one RequestGarbageCollectionForTesting(). So it's safer to call gc() multiple times.
Message was sent while issue was closed.
https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: Heap::collectAllGarbage(); On 2015/08/31 07:03:46, sof wrote: > On 2015/08/31 06:58:34, sof wrote: > > On 2015/08/31 06:55:52, haraken wrote: > > > On 2015/08/31 06:50:22, sof wrote: > > > > On 2015/08/31 06:46:42, haraken wrote: > > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > > Incidental to this CL, but having the name of this Heap entry point > > > indicate > > > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > > multiple > > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > > > somehow > > > > though, which isn't done currently. > > > > > > V8GCController::collectGarbage calls window.gc() and I guess the window.gc() > > > passes the forced flag somehow... (I can't find the call path at the moment > > > though.) > > > > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via > --expose-gc > > ? > > Which maps to v8/src/extensions/gc-extension.* > > i.e., it will force conservative Oilpan GCs as part of running. Which means that > the use of collectAllGarbage() here is redundant + the addition of the > Persistent<>s in two tests aren't needed. Right? Once I was told that relying on the conservative GC might make tests flaky. I want to ensure the holder wrapper which was stored in ScriptPromisePropertyGarbageCollectedTest::m_holder is collected.
Message was sent while issue was closed.
On 2015/08/31 07:05:34, haraken wrote: > ... > But gc() runs only one RequestGarbageCollectionForTesting(). So it's safer to > call gc() multiple times. which suggests (also) having V8GCController::collectAllGarbage(Isolate*), so as to have correspondence with GCController offerings?
Message was sent while issue was closed.
On 2015/08/31 07:11:25, sof wrote: > On 2015/08/31 07:05:34, haraken wrote: > > > ... > > But gc() runs only one RequestGarbageCollectionForTesting(). So it's safer to > > call gc() multiple times. > > which suggests (also) having V8GCController::collectAllGarbage(Isolate*), so as > to have correspondence with GCController offerings? Sounds nice.
Message was sent while issue was closed.
On 2015/08/31 07:10:44, yhirano wrote: > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: > Heap::collectAllGarbage(); > On 2015/08/31 07:03:46, sof wrote: > > On 2015/08/31 06:58:34, sof wrote: > > > On 2015/08/31 06:55:52, haraken wrote: > > > > On 2015/08/31 06:50:22, sof wrote: > > > > > On 2015/08/31 06:46:42, haraken wrote: > > > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > > > Incidental to this CL, but having the name of this Heap entry point > > > > indicate > > > > > > > that it doesn't perform conservative GCs would be helpful, arguably. > > > > > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > > > multiple > > > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > > > > > You have to supply the "force" flag in V8GCController::collectGarbage() > > > > somehow > > > > > though, which isn't done currently. > > > > > > > > V8GCController::collectGarbage calls window.gc() and I guess the > window.gc() > > > > passes the forced flag somehow... (I can't find the call path at the > moment > > > > though.) > > > > > > > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via > > --expose-gc > > > ? > > > > Which maps to v8/src/extensions/gc-extension.* > > > > i.e., it will force conservative Oilpan GCs as part of running. Which means > that > > the use of collectAllGarbage() here is redundant + the addition of the > > Persistent<>s in two tests aren't needed. Right? > > Once I was told that relying on the conservative GC might make tests flaky. I > want to ensure the holder wrapper which was stored in > ScriptPromisePropertyGarbageCollectedTest::m_holder is collected. Ah, that makes sense...
Message was sent while issue was closed.
On 2015/08/31 07:16:44, haraken wrote: > On 2015/08/31 07:10:44, yhirano wrote: > > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > > File Source/bindings/core/v8/ScriptPromisePropertyTest.cpp (right): > > > > > https://codereview.chromium.org/1306083008/diff/60001/Source/bindings/core/v8... > > Source/bindings/core/v8/ScriptPromisePropertyTest.cpp:153: > > Heap::collectAllGarbage(); > > On 2015/08/31 07:03:46, sof wrote: > > > On 2015/08/31 06:58:34, sof wrote: > > > > On 2015/08/31 06:55:52, haraken wrote: > > > > > On 2015/08/31 06:50:22, sof wrote: > > > > > > On 2015/08/31 06:46:42, haraken wrote: > > > > > > > On 2015/08/31 06:44:40, sof wrote: > > > > > > > > Incidental to this CL, but having the name of this Heap entry > point > > > > > indicate > > > > > > > > that it doesn't perform conservative GCs would be helpful, > arguably. > > > > > > > > > > > > > > Yeah, a better idea would be to call V8GCController::collectGarbage > > > > multiple > > > > > > > times in gc(). And don't call Heap::collectAllGarbage() here. > > > > > > > > > > > > You have to supply the "force" flag in > V8GCController::collectGarbage() > > > > > somehow > > > > > > though, which isn't done currently. > > > > > > > > > > V8GCController::collectGarbage calls window.gc() and I guess the > > window.gc() > > > > > passes the forced flag somehow... (I can't find the call path at the > > moment > > > > > though.) > > > > > > > > > > > > > This is a unit test -- isn't "gc()" mapping to what v8 provides via > > > --expose-gc > > > > ? > > > > > > Which maps to v8/src/extensions/gc-extension.* > > > > > > i.e., it will force conservative Oilpan GCs as part of running. Which means > > that > > > the use of collectAllGarbage() here is redundant + the addition of the > > > Persistent<>s in two tests aren't needed. Right? > > > > Once I was told that relying on the conservative GC might make tests flaky. I > > want to ensure the holder wrapper which was stored in > > ScriptPromisePropertyGarbageCollectedTest::m_holder is collected. > > Ah, that makes sense... But the tests here adds Persistent<>s to retain objects, so how are m_holders verifiedly collected across uses of destroyContext() ?
Message was sent while issue was closed.
On 2015/08/31 07:33:47, sof wrote: > On 2015/08/31 07:16:44, haraken wrote: > > On 2015/08/31 07:10:44, yhirano wrote: > > > > > > (snip) > > > Once I was told that relying on the conservative GC might make tests flaky. > I > > > want to ensure the holder wrapper which was stored in > > > ScriptPromisePropertyGarbageCollectedTest::m_holder is collected. > > > > Ah, that makes sense... > > But the tests here adds Persistent<>s to retain objects, so how are m_holders > verifiedly collected across uses of destroyContext() ? Sorry my memory was confused. I thought m_holder's aliveness affects the test results, but it seems not. I now think no garbage collection is needed in destroyContext(). https://codereview.chromium.org/1322673003/
Message was sent while issue was closed.
On 2015/08/31 07:43:48, yhirano wrote: > On 2015/08/31 07:33:47, sof wrote: > > On 2015/08/31 07:16:44, haraken wrote: > > > On 2015/08/31 07:10:44, yhirano wrote: > > > > > > > > (snip) > > > > Once I was told that relying on the conservative GC might make tests > flaky. > > I > > > > want to ensure the holder wrapper which was stored in > > > > ScriptPromisePropertyGarbageCollectedTest::m_holder is collected. > > > > > > Ah, that makes sense... > > > > But the tests here adds Persistent<>s to retain objects, so how are m_holders > > verifiedly collected across uses of destroyContext() ? > > Sorry my memory was confused. I thought m_holder's aliveness affects the test > results, but it seems not. > I now think no garbage collection is needed in destroyContext(). > https://codereview.chromium.org/1322673003/ ok, i started an Oilpan trybot for that one also. |
