Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1472823002:
Couple V8AbstractEventListener's lifetime to the V8 listeners lifetime (Closed)
|
Created:
5 years, 1 month ago by jochen (gone - plz use gerrit) Modified:
5 years ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCouple V8AbstractEventListener's lifetime to the V8 listeners lifetime
That way, we don't have to mess with the hidden values in the dtor.
BUG=558975
R=haraken@chromium.org,sigbjornf@opera.com
Committed: https://crrev.com/b6edd389a3d322e70cb3cf7d0c176cec635e34b0
Cr-Commit-Position: refs/heads/master@{#361356}
Patch Set 1 #
Total comments: 8
Patch Set 2 : updates #Patch Set 3 : oilpan #Patch Set 4 : updates #Patch Set 5 : updates #Patch Set 6 : clean up workers #
Total comments: 1
Patch Set 7 : ignore self refs #
Total comments: 3
Patch Set 8 : manage listeners on workers via a list #Patch Set 9 : updates #
Total comments: 7
Patch Set 10 : updates #
Total comments: 7
Patch Set 11 : updates #Messages
Total messages: 61 (4 generated)
https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:58: ASSERT(m_listener.isEmpty()); we never have to clearWrapper, as the wrapper will now always die before the event listener. https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); this won't work in the ctor, that's why I had to move the call to setListenerObject() from the ctors of the derived classes to the static factory method. also: LOL! OILPAN! how does this even work?? I guess we can now also remove the eager finalization magic from the header?
Thanks for looking into the work-around! https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:103: // Balanced in secondWeakCallback xor clearListenerObject. Can we add some ASSERT to check that secondWeakCallback and clearListenerObject are not called at the same time? https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); On 2015/11/23 13:26:15, jochen wrote: > this won't work in the ctor, that's why I had to move the call to > setListenerObject() from the ctors of the derived classes to the static factory > method. > > also: LOL! OILPAN! how does this even work?? I guess we can now also remove the > eager finalization magic from the header? Does this really compile with enable_oilpan=1? If you want to use ref/deref in oilpan, you need to make EventListener a RefCountedWillBeRefCountedGarbageCollected. RefCountedGarbageCollected provides a ref/deref which internally creates a persistent handle to the object itself. Sigbjorn may have a better thought.
https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:103: // Balanced in secondWeakCallback xor clearListenerObject. On 2015/11/23 at 13:40:55, haraken wrote: > Can we add some ASSERT to check that secondWeakCallback and clearListenerObject are not called at the same time? done https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); On 2015/11/23 at 13:40:55, haraken wrote: > On 2015/11/23 13:26:15, jochen wrote: > > this won't work in the ctor, that's why I had to move the call to > > setListenerObject() from the ctors of the derived classes to the static factory > > method. > > > > also: LOL! OILPAN! how does this even work?? I guess we can now also remove the > > eager finalization magic from the header? > > Does this really compile with enable_oilpan=1? I don't think so > > If you want to use ref/deref in oilpan, you need to make EventListener a RefCountedWillBeRefCountedGarbageCollected. RefCountedGarbageCollected provides a ref/deref which internally creates a persistent handle to the object itself. > > Sigbjorn may have a better thought. sounds possible
https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); On 2015/11/23 13:40:55, haraken wrote: > On 2015/11/23 13:26:15, jochen wrote: > > this won't work in the ctor, that's why I had to move the call to > > setListenerObject() from the ctors of the derived classes to the static > factory > > method. > > > > also: LOL! OILPAN! how does this even work?? I guess we can now also remove > the > > eager finalization magic from the header? > > Does this really compile with enable_oilpan=1? > > If you want to use ref/deref in oilpan, you need to make EventListener a > RefCountedWillBeRefCountedGarbageCollected. RefCountedGarbageCollected provides > a ref/deref which internally creates a persistent handle to the object itself. > > Sigbjorn may have a better thought. That's one alternative, another is to add a "SelfKeepAlive<V8AbstractEventListener> m_keepAlive" field, which you then assign "this" to here. (See CSSImageGeneratorValue for one such use.) That saves having to make all derived EventListeners RefCountedGarbageCollected. But, it would be good to avoid having to resort to Persistent<>s here - I will have to think through if that is possible somehow. The eager finalization can be retired.
ptal
On 2015/11/23 14:04:00, jochen wrote: > ptal Seems like a bunch of tests are crashing in linux_oilpan_rel.
https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); On 2015/11/23 13:26:15, jochen wrote: > this won't work in the ctor, that's why I had to move the call to > setListenerObject() from the ctors of the derived classes to the static factory > method. > I'm missing something.. could you explain why ref() can't be done in V8AbstractEventListener() ?
On 2015/11/23 14:31:45, haraken wrote: > On 2015/11/23 14:04:00, jochen wrote: > > ptal > > Seems like a bunch of tests are crashing in linux_oilpan_rel. (Self)Persistent<>s are still held upon (worker) heap shutdown.
On 2015/11/23 14:41:18, sof wrote: > On 2015/11/23 14:31:45, haraken wrote: > > On 2015/11/23 14:04:00, jochen wrote: > > > ptal > > > > Seems like a bunch of tests are crashing in linux_oilpan_rel. > > (Self)Persistent<>s are still held upon (worker) heap shutdown. (Trying to repro leak !Oilpan, as I don't understand how SelfPersistent<> would behave worser than ref-counts.)
On 2015/11/23 at 14:38:05, sigbjornf wrote: > https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): > > https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: ref(); > On 2015/11/23 13:26:15, jochen wrote: > > this won't work in the ctor, that's why I had to move the call to > > setListenerObject() from the ctors of the derived classes to the static factory > > method. > > > > I'm missing something.. could you explain why ref() can't be done in V8AbstractEventListener() ? while the ctor is running, the RefCounted<> isn't yet "adopted", and so we hit the ASSERT(!m_adoptionRequired) in RefCountedBase::ref
On 2015/11/23 14:54:21, sof wrote: > On 2015/11/23 14:41:18, sof wrote: > > On 2015/11/23 14:31:45, haraken wrote: > > > On 2015/11/23 14:04:00, jochen wrote: > > > > ptal > > > > > > Seems like a bunch of tests are crashing in linux_oilpan_rel. > > > > (Self)Persistent<>s are still held upon (worker) heap shutdown. > > (Trying to repro leak !Oilpan, as I don't understand how SelfPersistent<> would > behave worser than ref-counts.) Doesn't reproduce there. The issue here is that the thread termination GCs done for a worker only operate over the Oilpan heap. Which means that we won't flush out v8-side objects that end up clearing out these SelfPersistent<>s when being swept. Should WebThreadSupportingGC also perform v8 GCs during shutdown?
On 2015/11/23 at 16:08:36, sigbjornf wrote: > On 2015/11/23 14:54:21, sof wrote: > > On 2015/11/23 14:41:18, sof wrote: > > > On 2015/11/23 14:31:45, haraken wrote: > > > > On 2015/11/23 14:04:00, jochen wrote: > > > > > ptal > > > > > > > > Seems like a bunch of tests are crashing in linux_oilpan_rel. > > > > > > (Self)Persistent<>s are still held upon (worker) heap shutdown. > > > > (Trying to repro leak !Oilpan, as I don't understand how SelfPersistent<> would > > behave worser than ref-counts.) > > Doesn't reproduce there. > > The issue here is that the thread termination GCs done for a worker only operate over the Oilpan heap. Which means that we won't flush out v8-side objects that end up clearing out these SelfPersistent<>s when being swept. > > Should WebThreadSupportingGC also perform v8 GCs during shutdown? I think WorkerGlobalScope::dispose is supposed to clean things up
On 2015/11/23 15:04:09, jochen wrote: > On 2015/11/23 at 14:38:05, sigbjornf wrote: > > > https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... > > File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp > (right): > > > > > https://codereview.chromium.org/1472823002/diff/1/third_party/WebKit/Source/b... > > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:104: > ref(); > > On 2015/11/23 13:26:15, jochen wrote: > > > this won't work in the ctor, that's why I had to move the call to > > > setListenerObject() from the ctors of the derived classes to the static > factory > > > method. > > > > > > > I'm missing something.. could you explain why ref() can't be done in > V8AbstractEventListener() ? > > while the ctor is running, the RefCounted<> isn't yet "adopted", and so we hit > the ASSERT(!m_adoptionRequired) in RefCountedBase::ref Thanks, yes. We can switch (back) to something ctor-based when Oilpan is default-enabled for event listeners. I don't see a tidy way to do this across ENABLE(OILPAN) settings right now. Perhaps you could add a TODO(Oilpan) somewhere so that we don't forget?
i tried adding some clean-up code, but it still doesn't work :-/ should I try to do a RefCountedGarbageCollected thing?
https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if (EventTargetData* eventTargetData = this->eventTargetData()) { The EventTargetData and its event listener map is for the WorkerGlobalScope global object only, right?
On 2015/11/23 at 20:31:18, sigbjornf wrote: > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if (EventTargetData* eventTargetData = this->eventTargetData()) { > The EventTargetData and its event listener map is for the WorkerGlobalScope global object only, right? right. I think the problem here is that if you set something as an event listener, and then remove it again, the function might still exist, and so will the C++ object, just that it's not in that map anymore. I guess what we need is either a global registry of all V8AbstractEventListeners, so we can clear it at thread shutdown, or we teach the check that persistent self-references are ok at shutdown.
On 2015/11/23 20:52:05, jochen wrote: > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > (EventTargetData* eventTargetData = this->eventTargetData()) { > > The EventTargetData and its event listener map is for the WorkerGlobalScope > global object only, right? > > right. > > I think the problem here is that if you set something as an event listener, and > then remove it again, the function might still exist, and so will the C++ > object, just that it's not in that map anymore. > > I guess what we need is either a global registry of all > V8AbstractEventListeners, so we can clear it at thread shutdown, or we teach the > check that persistent self-references are ok at shutdown. Are we certain that no v8 GCs will strike after the thread heap shutdown? If not, then the weak callbacks here would access freed Oilpan heap objects, or could those be cancelled from running.
On 2015/11/23 at 21:41:23, sigbjornf wrote: > On 2015/11/23 20:52:05, jochen wrote: > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > The EventTargetData and its event listener map is for the WorkerGlobalScope > > global object only, right? > > > > right. > > > > I think the problem here is that if you set something as an event listener, and > > then remove it again, the function might still exist, and so will the C++ > > object, just that it's not in that map anymore. > > > > I guess what we need is either a global registry of all > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we teach the > > check that persistent self-references are ok at shutdown. > > Are we certain that no v8 GCs will strike after the thread heap shutdown? If not, then the weak callbacks here would access freed Oilpan heap objects, or could those be cancelled from running. yes, we just delete the entire isolate which frees up all v8 managed memory without invoking any of the callbacks.
latest patchset: ignore self refs for the assert()
On 2015/11/23 21:44:09, jochen wrote: > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > On 2015/11/23 20:52:05, jochen wrote: > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > The EventTargetData and its event listener map is for the > WorkerGlobalScope > > > global object only, right? > > > > > > right. > > > > > > I think the problem here is that if you set something as an event listener, > and > > > then remove it again, the function might still exist, and so will the C++ > > > object, just that it's not in that map anymore. > > > > > > I guess what we need is either a global registry of all > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we teach > the > > > check that persistent self-references are ok at shutdown. > > > > Are we certain that no v8 GCs will strike after the thread heap shutdown? If > not, then the weak callbacks here would access freed Oilpan heap objects, or > could those be cancelled from running. > > yes, we just delete the entire isolate which frees up all v8 managed memory > without invoking any of the callbacks. If we don't invoke the weak callbacks at the thread shutdown, won't it cause a memory leak in the Blink side in worker threads (with or without oilpan)?
Could someone kick off (mac_blink_oilpan_{dbg,rel}) trybots? I've so far been
unable to reproduce the failures locally w/ Oilpan-release.
(The https://code.google.com/p/chromium/issues/detail?id=559149 regression
prevents me from doing this directly. Sigh.)
On 2015/11/24 06:21:55, sof wrote:
> Could someone kick off (mac_blink_oilpan_{dbg,rel}) trybots? I've so far been
> unable to reproduce the failures locally w/ Oilpan-release.
>
> (The https://code.google.com/p/chromium/issues/detail?id=559149 regression
> prevents me from doing this directly. Sigh.)
Kicked off the bots.
On 2015/11/24 at 01:25:11, haraken wrote: > On 2015/11/23 21:44:09, jochen wrote: > > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > > On 2015/11/23 20:52:05, jochen wrote: > > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > > The EventTargetData and its event listener map is for the > > WorkerGlobalScope > > > > global object only, right? > > > > > > > > right. > > > > > > > > I think the problem here is that if you set something as an event listener, > > and > > > > then remove it again, the function might still exist, and so will the C++ > > > > object, just that it's not in that map anymore. > > > > > > > > I guess what we need is either a global registry of all > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we teach > > the > > > > check that persistent self-references are ok at shutdown. > > > > > > Are we certain that no v8 GCs will strike after the thread heap shutdown? If > > not, then the weak callbacks here would access freed Oilpan heap objects, or > > could those be cancelled from running. > > > > yes, we just delete the entire isolate which frees up all v8 managed memory > > without invoking any of the callbacks. > > If we don't invoke the weak callbacks at the thread shutdown, won't it cause a memory leak in the Blink side in worker threads (with or without oilpan)? In an oilpan world, discarding the entire thread's heap should work... Without oilpan, we memory indeed :-/ I guess we need a registry of all listeners, so we can clear it thread thread shutdown. In the mean time, I'll disable the caching via hidden values to fix the crashers
On 2015/11/24 at 06:21:55, sigbjornf wrote:
> Could someone kick off (mac_blink_oilpan_{dbg,rel}) trybots? I've so far been
unable to reproduce the failures locally w/ Oilpan-release.
>
> (The https://code.google.com/p/chromium/issues/detail?id=559149 regression
prevents me from doing this directly. Sigh.)
Feel free to cc me on such bugs
On 2015/11/24 07:17:07, jochen wrote: > On 2015/11/24 at 01:25:11, haraken wrote: > > On 2015/11/23 21:44:09, jochen wrote: > > > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > > > On 2015/11/23 20:52:05, jochen wrote: > > > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp > (right): > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > > > The EventTargetData and its event listener map is for the > > > WorkerGlobalScope > > > > > global object only, right? > > > > > > > > > > right. > > > > > > > > > > I think the problem here is that if you set something as an event > listener, > > > and > > > > > then remove it again, the function might still exist, and so will the > C++ > > > > > object, just that it's not in that map anymore. > > > > > > > > > > I guess what we need is either a global registry of all > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we > teach > > > the > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap shutdown? > If > > > not, then the weak callbacks here would access freed Oilpan heap objects, or > > > could those be cancelled from running. > > > > > > yes, we just delete the entire isolate which frees up all v8 managed memory > > > without invoking any of the callbacks. > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it cause a > memory leak in the Blink side in worker threads (with or without oilpan)? > > In an oilpan world, discarding the entire thread's heap should work... Without > oilpan, we memory indeed :-/ Oilpan discards all the thread's heap but doesn't call destructors for the leaked objects (because it's just dangerous). This means that if the remaining oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will leak. I think that with or without oilpan, we should invoke the weak callbacks before the v8::Isolate shuts down.
On 2015/11/24 07:28:36, haraken wrote: > On 2015/11/24 07:17:07, jochen wrote: > > On 2015/11/24 at 01:25:11, haraken wrote: > > > On 2015/11/23 21:44:09, jochen wrote: > > > > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > > > > On 2015/11/23 20:52:05, jochen wrote: > > > > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > > > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > > > > The EventTargetData and its event listener map is for the > > > > WorkerGlobalScope > > > > > > global object only, right? > > > > > > > > > > > > right. > > > > > > > > > > > > I think the problem here is that if you set something as an event > > listener, > > > > and > > > > > > then remove it again, the function might still exist, and so will the > > C++ > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we > > teach > > > > the > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap > shutdown? > > If > > > > not, then the weak callbacks here would access freed Oilpan heap objects, > or > > > > could those be cancelled from running. > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed > memory > > > > without invoking any of the callbacks. > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it cause > a > > memory leak in the Blink side in worker threads (with or without oilpan)? > > > > In an oilpan world, discarding the entire thread's heap should work... Without > > oilpan, we memory indeed :-/ > > Oilpan discards all the thread's heap but doesn't call destructors for the > leaked objects (because it's just dangerous). This means that if the remaining > oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will > leak. > > I think that with or without oilpan, we should invoke the weak callbacks before > the v8::Isolate shuts down. => add an indirection and use a registry that can separately release its ref-ed event listeners? (as suggested above.)
On 2015/11/24 07:34:23, sof wrote: > On 2015/11/24 07:28:36, haraken wrote: > > On 2015/11/24 07:17:07, jochen wrote: > > > On 2015/11/24 at 01:25:11, haraken wrote: > > > > On 2015/11/23 21:44:09, jochen wrote: > > > > > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > > > > > On 2015/11/23 20:52:05, jochen wrote: > > > > > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: > if > > > > > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > > > > > The EventTargetData and its event listener map is for the > > > > > WorkerGlobalScope > > > > > > > global object only, right? > > > > > > > > > > > > > > right. > > > > > > > > > > > > > > I think the problem here is that if you set something as an event > > > listener, > > > > > and > > > > > > > then remove it again, the function might still exist, and so will > the > > > C++ > > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or > we > > > teach > > > > > the > > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap > > shutdown? > > > If > > > > > not, then the weak callbacks here would access freed Oilpan heap > objects, > > or > > > > > could those be cancelled from running. > > > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed > > memory > > > > > without invoking any of the callbacks. > > > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it > cause > > a > > > memory leak in the Blink side in worker threads (with or without oilpan)? > > > > > > In an oilpan world, discarding the entire thread's heap should work... > Without > > > oilpan, we memory indeed :-/ > > > > Oilpan discards all the thread's heap but doesn't call destructors for the > > leaked objects (because it's just dangerous). This means that if the remaining > > oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will > > leak. > > > > I think that with or without oilpan, we should invoke the weak callbacks > before > > the v8::Isolate shuts down. > > => add an indirection and use a registry that can separately release its ref-ed > event listeners? (as suggested above.) My point is that this would not be an issue specific to EventListeners (even though EventListeners would be the only objects that leak in practice). I think we should invoke weak callbacks for all DOM wrappers before V8 shuts down (except for the main thread).
On 2015/11/24 at 07:28:36, haraken wrote: > On 2015/11/24 07:17:07, jochen wrote: > > On 2015/11/24 at 01:25:11, haraken wrote: > > > On 2015/11/23 21:44:09, jochen wrote: > > > > On 2015/11/23 at 21:41:23, sigbjornf wrote: > > > > > On 2015/11/23 20:52:05, jochen wrote: > > > > > > On 2015/11/23 at 20:31:18, sigbjornf wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472823002/diff/100001/third_party/WebKit/Sou... > > > > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if > > > > > > (EventTargetData* eventTargetData = this->eventTargetData()) { > > > > > > > The EventTargetData and its event listener map is for the > > > > WorkerGlobalScope > > > > > > global object only, right? > > > > > > > > > > > > right. > > > > > > > > > > > > I think the problem here is that if you set something as an event > > listener, > > > > and > > > > > > then remove it again, the function might still exist, and so will the > > C++ > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or we > > teach > > > > the > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap shutdown? > > If > > > > not, then the weak callbacks here would access freed Oilpan heap objects, or > > > > could those be cancelled from running. > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed memory > > > > without invoking any of the callbacks. > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it cause a > > memory leak in the Blink side in worker threads (with or without oilpan)? > > > > In an oilpan world, discarding the entire thread's heap should work... Without > > oilpan, we memory indeed :-/ > > Oilpan discards all the thread's heap but doesn't call destructors for the leaked objects (because it's just dangerous). This means that if the remaining oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will leak. > > I think that with or without oilpan, we should invoke the weak callbacks before the v8::Isolate shuts down. It's not safe to do that - the weak callbacks might create new persistent handles handles, the C++ objects might not expect to have their wrappers disappear, etc.. The way workers are shut down is to just delete all c++ objects and stop calling into v8
> > > > > > > I think the problem here is that if you set something as an event > > > listener, > > > > > and > > > > > > > then remove it again, the function might still exist, and so will > the > > > C++ > > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or > we > > > teach > > > > > the > > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap > shutdown? > > > If > > > > > not, then the weak callbacks here would access freed Oilpan heap > objects, or > > > > > could those be cancelled from running. > > > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed > memory > > > > > without invoking any of the callbacks. > > > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it > cause a > > > memory leak in the Blink side in worker threads (with or without oilpan)? > > > > > > In an oilpan world, discarding the entire thread's heap should work... > Without > > > oilpan, we memory indeed :-/ > > > > Oilpan discards all the thread's heap but doesn't call destructors for the > leaked objects (because it's just dangerous). This means that if the remaining > oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will > leak. > > > > I think that with or without oilpan, we should invoke the weak callbacks > before the v8::Isolate shuts down. > > It's not safe to do that - the weak callbacks might create new persistent > handles handles, the C++ objects might not expect to have their wrappers > disappear, etc.. > > The way workers are shut down is to just delete all c++ objects and stop calling > into v8 However, unless V8 releases the reference to the C++ objects, we cannot safely delete the C++ objects. Maybe would it be an option that we visit all wrappers and explicitly drop the references before the worker shuts down (instead of asking V8 to invoke the weak callbacks)?
On 2015/11/24 at 07:45:47, haraken wrote: > > > > > > > > I think the problem here is that if you set something as an event > > > > listener, > > > > > > and > > > > > > > > then remove it again, the function might still exist, and so will > > the > > > > C++ > > > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, or > > we > > > > teach > > > > > > the > > > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap > > shutdown? > > > > If > > > > > > not, then the weak callbacks here would access freed Oilpan heap > > objects, or > > > > > > could those be cancelled from running. > > > > > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed > > memory > > > > > > without invoking any of the callbacks. > > > > > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it > > cause a > > > > memory leak in the Blink side in worker threads (with or without oilpan)? > > > > > > > > In an oilpan world, discarding the entire thread's heap should work... > > Without > > > > oilpan, we memory indeed :-/ > > > > > > Oilpan discards all the thread's heap but doesn't call destructors for the > > leaked objects (because it's just dangerous). This means that if the remaining > > oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object will > > leak. > > > > > > I think that with or without oilpan, we should invoke the weak callbacks > > before the v8::Isolate shuts down. > > > > It's not safe to do that - the weak callbacks might create new persistent > > handles handles, the C++ objects might not expect to have their wrappers > > disappear, etc.. > > > > The way workers are shut down is to just delete all c++ objects and stop calling > > into v8 > > However, unless V8 releases the reference to the C++ objects, we cannot safely delete the C++ objects. > why? > Maybe would it be an option that we visit all wrappers and explicitly drop the references before the worker shuts down (instead of asking V8 to invoke the weak callbacks)?
On 2015/11/24 07:46:41, jochen wrote: > On 2015/11/24 at 07:45:47, haraken wrote: > > > > > > > > > I think the problem here is that if you set something as an > event > > > > > listener, > > > > > > > and > > > > > > > > > then remove it again, the function might still exist, and so > will > > > the > > > > > C++ > > > > > > > > > object, just that it's not in that map anymore. > > > > > > > > > > > > > > > > > > I guess what we need is either a global registry of all > > > > > > > > > V8AbstractEventListeners, so we can clear it at thread shutdown, > or > > > we > > > > > teach > > > > > > > the > > > > > > > > > check that persistent self-references are ok at shutdown. > > > > > > > > > > > > > > > > Are we certain that no v8 GCs will strike after the thread heap > > > shutdown? > > > > > If > > > > > > > not, then the weak callbacks here would access freed Oilpan heap > > > objects, or > > > > > > > could those be cancelled from running. > > > > > > > > > > > > > > yes, we just delete the entire isolate which frees up all v8 managed > > > memory > > > > > > > without invoking any of the callbacks. > > > > > > > > > > > > If we don't invoke the weak callbacks at the thread shutdown, won't it > > > cause a > > > > > memory leak in the Blink side in worker threads (with or without > oilpan)? > > > > > > > > > > In an oilpan world, discarding the entire thread's heap should work... > > > Without > > > > > oilpan, we memory indeed :-/ > > > > > > > > Oilpan discards all the thread's heap but doesn't call destructors for the > > > leaked objects (because it's just dangerous). This means that if the > remaining > > > oilpan object has a RefPtr to an non-oilpan object, the non-oilpan object > will > > > leak. > > > > > > > > I think that with or without oilpan, we should invoke the weak callbacks > > > before the v8::Isolate shuts down. > > > > > > It's not safe to do that - the weak callbacks might create new persistent > > > handles handles, the C++ objects might not expect to have their wrappers > > > disappear, etc.. > > > > > > The way workers are shut down is to just delete all c++ objects and stop > calling > > > into v8 > > > > However, unless V8 releases the reference to the C++ objects, we cannot safely > delete the C++ objects. > > > > why? Because there are a lot of dependencies in destruction ordering. If Oilpan just scans all remaining objects in the thread's heap and calls the destructors in that order, it will just crash. Due to the destruction dependency, Oilpan runs a thread-termination GCs multiple times to break the chain of Persistent => RefPtr/OwnPtr => Persistent => RefPtr/OwnPtr etc one by one and destruct all objects in a safe order. > > Maybe would it be an option that we visit all wrappers and explicitly drop the > references before the worker shuts down (instead of asking V8 to invoke the weak > callbacks)?
On 2015/11/24 07:17:37, jochen wrote:
> On 2015/11/24 at 06:21:55, sigbjornf wrote:
> > Could someone kick off (mac_blink_oilpan_{dbg,rel}) trybots? I've so far
been
> unable to reproduce the failures locally w/ Oilpan-release.
> >
> > (The https://code.google.com/p/chromium/issues/detail?id=559149 regression
> prevents me from doing this directly. Sigh.)
>
> Feel free to cc me on such bugs
Thanks; the folks working in this area are responsive & helpful, I wonder what
it turns out to be.
On 2015/11/24 06:25:51, haraken wrote:
> On 2015/11/24 06:21:55, sof wrote:
> > Could someone kick off (mac_blink_oilpan_{dbg,rel}) trybots? I've so far
been
> > unable to reproduce the failures locally w/ Oilpan-release.
> >
> > (The https://code.google.com/p/chromium/issues/detail?id=559149 regression
> > prevents me from doing this directly. Sigh.)
>
> Kicked off the bots.
Thanks,
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_oilpan...
is what strikes.
https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (left): https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:58: if (!m_listener.isEmpty()) { Zooming out here a bit.. simple question: what goes wrong if you don't clear the wrapper on an empty context() ?
https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (left): https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:58: if (!m_listener.isEmpty()) { On 2015/11/24 at 08:19:04, sof wrote: > Zooming out here a bit.. simple question: what goes wrong if you don't clear the wrapper on an empty context() ? the problem is that the m_scriptStateForListener turns out to be a random script state, and not the one the listener was created in.
https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1472823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:293: ASSERT(currentCount == m_numberOfSelfRefs); If this holds, tell the persistentRegion() to clear them all out and run another GC (or several, the live object set should be really quite small by now..) ?
next attempt: on the main thread, use self-refs. On worker threads, use a central per thread registry which we can clear at shutdown.
On 2015/11/24 at 10:05:02, jochen wrote: > next attempt: > > on the main thread, use self-refs. On worker threads, use a central per thread registry which we can clear at shutdown. Green bots, ptal
just some "surface" issues, i think we should go with this approach. https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h (right): https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:154: WorkerGlobalScope* m_workerGlobalScope; RawPtrWillBeMember<WorkerGlobalScope> along with adding visitor->trace(m_workerGlobalScope) to the trace method (let's define it out-of-line now.) https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8LazyEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8LazyEventListener.cpp:131: if (hasExistingListenerObject()) I think it would be tidier to have clearListenerObject() handle this directly and allow being called in a listener-less state. https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for (auto it = m_eventListeners.begin(); it != m_eventListeners.end(); ) { Wouldn't for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { } work? https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: if (static_cast<V8AbstractEventListener*>(listener.get())->hasExistingListenerObject()) V8AbstractEventListener* v8Listener = V8AbstractEventListener::cast(listener.get()); ASSERT(v8Listener); v8Listener->clearListenerObject(); https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:336: if (eventListener->type() != EventListener::JSEventListenerType) If giving 'eventListener' a type of V8AbstractEventListener* isn't acceptable, then V8AbstractEventListener* v8Listener = V8AbstractEventListener::cast(eventListener); if (!v8Listener) return; RELEASE_ASSERT(...); https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:343: if (eventListener->type() != EventListener::JSEventListenerType) Ditto re: use of V8AbstractEventListener::cast().
https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for (auto it = m_eventListeners.begin(); it != m_eventListeners.end(); ) { On 2015/11/24 at 12:10:24, sof wrote: > Wouldn't > > for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { > > } > > work? can that cope with m_eventListeners being modified during iteration (i.e. the current iterator being deleted)?
On 2015/11/24 12:14:13, jochen wrote: > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for (auto it = > m_eventListeners.begin(); it != m_eventListeners.end(); ) { > On 2015/11/24 at 12:10:24, sof wrote: > > Wouldn't > > > > for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { > > > > } > > > > work? > > can that cope with m_eventListeners being modified during iteration (i.e. the > current iterator being deleted)? I think so, see the translation at http://en.cppreference.com/w/cpp/language/range-for
On 2015/11/24 at 12:18:27, sigbjornf wrote: > On 2015/11/24 12:14:13, jochen wrote: > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for (auto it = > > m_eventListeners.begin(); it != m_eventListeners.end(); ) { > > On 2015/11/24 at 12:10:24, sof wrote: > > > Wouldn't > > > > > > for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { > > > > > > } > > > > > > work? > > > > can that cope with m_eventListeners being modified during iteration (i.e. the > > current iterator being deleted)? > > I think so, see the translation at http://en.cppreference.com/w/cpp/language/range-for that translation makes it look like it won't work, as __begin will no longer be valid at the end of the body when ++__begin is executed
On 2015/11/24 at 12:21:04, jochen wrote: > On 2015/11/24 at 12:18:27, sigbjornf wrote: > > On 2015/11/24 12:14:13, jochen wrote: > > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for (auto it = > > > m_eventListeners.begin(); it != m_eventListeners.end(); ) { > > > On 2015/11/24 at 12:10:24, sof wrote: > > > > Wouldn't > > > > > > > > for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { > > > > > > > > } > > > > > > > > work? > > > > > > can that cope with m_eventListeners being modified during iteration (i.e. the > > > current iterator being deleted)? > > > > I think so, see the translation at http://en.cppreference.com/w/cpp/language/range-for > > that translation makes it look like it won't work, as __begin will no longer be valid at the end of the body when ++__begin is executed (note that clearListenerObject will deregister the listener which in turn deletes the entry from the collection)
addressed all remaining comments
On 2015/11/24 12:28:11, jochen wrote: > On 2015/11/24 at 12:21:04, jochen wrote: > > On 2015/11/24 at 12:18:27, sigbjornf wrote: > > > On 2015/11/24 12:14:13, jochen wrote: > > > > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > > > > > > > > https://codereview.chromium.org/1472823002/diff/90010/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:215: for > (auto it = > > > > m_eventListeners.begin(); it != m_eventListeners.end(); ) { > > > > On 2015/11/24 at 12:10:24, sof wrote: > > > > > Wouldn't > > > > > > > > > > for (RefPtrWillBeRawPtr<EventListener> listener : m_eventListeners) { > > > > > > > > > > } > > > > > > > > > > work? > > > > > > > > can that cope with m_eventListeners being modified during iteration (i.e. > the > > > > current iterator being deleted)? > > > > > > I think so, see the translation at > http://en.cppreference.com/w/cpp/language/range-for > > > > that translation makes it look like it won't work, as __begin will no longer > be valid at the end of the body when ++__begin is executed > > (note that clearListenerObject will deregister the listener which in turn > deletes the entry from the collection) Ah, which runs straight into what ListHashSet advertises as not being supported. Thanks for the clarification.
lgtm. haraken@: if we run into other instances of weak callbacks not being run upon thread heap shutdowns, then let's revisit?
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472823002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472823002/160001
LGTM with comments. https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:107: if (m_workerGlobalScope) { I'd prefer using isMainThread(). See the following comment. https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:201: m_workerGlobalScope->deregisterEventListener(this); We should null out the m_workerGlobalScope. Otherwise, it has a risk of becoming a stale pointer. Once it's nulled out, we cannot distinguish a nulled-out m_workerGlobalScope from the main thread's m_workerGlobalScope. So I prefer using isMainThread() for the if predicate. https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: listener->clearListenerObject(); Worth having a comment that the clearListenerObject removes the entry from the ListHashSet. - I wonder why you cannot simply use a HashSet. - It would be more straightforward to use copyToVector and iterate the vector. https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:335: RELEASE_ASSERT(m_eventListeners.add(eventListener).isNewEntry); It's not nice to do a meaningful work inside ASSERT even if it's a RELEASE_ASSERT. RELEASE_ASSERT(!m_eventListeners.contains(eventListener)); m_eventListeners.add(eventListener);
The CQ bit was unchecked by haraken@chromium.org
https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:201: m_workerGlobalScope->deregisterEventListener(this); On 2015/11/24 at 13:16:00, haraken wrote: > We should null out the m_workerGlobalScope. Otherwise, it has a risk of becoming a stale pointer. in an oilpan world, that's not possible. in a pre-oilpan world that would mean that we're already in really really deep trouble > > Once it's nulled out, we cannot distinguish a nulled-out m_workerGlobalScope from the main thread's m_workerGlobalScope. So I prefer using isMainThread() for the if predicate. https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: listener->clearListenerObject(); On 2015/11/24 at 13:16:00, haraken wrote: > Worth having a comment that the clearListenerObject removes the entry from the ListHashSet. > > - I wonder why you cannot simply use a HashSet. can't be modified during iteration. > > - It would be more straightforward to use copyToVector and iterate the vector. increases the memory requirements unnecessarily https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:335: RELEASE_ASSERT(m_eventListeners.add(eventListener).isNewEntry); On 2015/11/24 at 13:16:00, haraken wrote: > It's not nice to do a meaningful work inside ASSERT even if it's a RELEASE_ASSERT. > > RELEASE_ASSERT(!m_eventListeners.contains(eventListener)); > m_eventListeners.add(eventListener); that's twice as slow...
added the comment and moved the addition to the set outside of the release_assert
I tried isMainThread() but if we'd ever get in one of those places with m_workerGlobalScope == nullptr (or previously, a dangling pointer), we're in deep trouble anyway, there's nothing sensible to do .
On 2015/11/24 13:21:03, jochen wrote: > https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp > (right): > > https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:201: > m_workerGlobalScope->deregisterEventListener(this); > On 2015/11/24 at 13:16:00, haraken wrote: > > We should null out the m_workerGlobalScope. Otherwise, it has a risk of > becoming a stale pointer. > > in an oilpan world, that's not possible. > > in a pre-oilpan world that would mean that we're already in really really deep > trouble > > > > > Once it's nulled out, we cannot distinguish a nulled-out m_workerGlobalScope > from the main thread's m_workerGlobalScope. So I prefer using isMainThread() for > the if predicate. > > https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): > > https://codereview.chromium.org/1472823002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:218: > listener->clearListenerObject(); > On 2015/11/24 at 13:16:00, haraken wrote: > > Worth having a comment that the clearListenerObject removes the entry from the > ListHashSet. > > > > - I wonder why you cannot simply use a HashSet. > > can't be modified during iteration. > > > > > - It would be more straightforward to use copyToVector and iterate the vector. > > increases the memory requirements unnecessarily Nit: ListHashSet would be slower in add/remove operations (than HashSet + copyToVector). Either way, that's not a big deal. LGTM.
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1472823002/#ps180001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472823002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472823002/180001
Message was sent while issue was closed.
Committed patchset #11 (id:180001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b6edd389a3d322e70cb3cf7d0c176cec635e34b0 Cr-Commit-Position: refs/heads/master@{#361356}
Message was sent while issue was closed.
The Oilpan leak and ASan bots are all green again with this one on board. |
