Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
320253002:
Oilpan: Prepare to move ImageLoader and its subclasses to Oilpan heap. (Closed)
|
Created:
6 years, 6 months ago by tkent Modified:
6 years, 5 months ago CC:
blink-reviews, ed+blinkwatch_opera.com, philipj_slow, blink-reviews-html_chromium.org, kouhei+svg_chromium.org, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, krit, f(malita), gavinp+loader_chromium.org, gyuyoung.kim_webkit.org, Stephen Chennney, Nate Chapin, pdr., rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionOilpan: Prepare to move ImageLoader and its subclasses to Oilpan heap.
- We need to add Persistent<Element> in ImageLoader in order to
follow the current behavior by manual ref/deref.
Also, the manual ref/deref is replaced with RefPtr<Element>.
- ImageLoader was the last off-heap user of EventSender<T>. Now all of
EventSender<T> users are on-heap. We can trace EventSender::m_dispatchSoonList
and m_dispatchingList.
- HTMLImageElement and SVGImageElement had ImageLoader as part objects. This CL
makes them OwnPtr / Member because using on-heap objects as part objects are
dangerous.
- Add HTMLImageElement::create and SVGImageElement::create.
BUG=357163
R=haraken@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175876
Patch Set 1 : #
Total comments: 17
Patch Set 2 : Persistent<Element> #
Total comments: 5
Patch Set 3 : rebase #
Total comments: 17
Messages
Total messages: 37 (0 generated)
Please review this.
https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... File Source/core/html/HTMLImageElement.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... Source/core/html/HTMLImageElement.h:144: WeakPtr<HTMLFormElement> m_form; WeakPtrWillBeMember is available. https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... File Source/core/html/HTMLImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... Source/core/html/HTMLImageLoader.h:44: HTMLImageLoader(Element*); Add explicit. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:129: m_element->deref(); It looks strange that we don't have corresponding code to this. (See my comment below.) https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:427: m_keepAlive = this; I'm afraid this will cause memory leak. If timerFired() is not triggered, this ImageLoader will kept alive forever. I think a right fix would be: - Call 'm_keepAlive = m_element' here. - Call 'm_keepAlive.clear()' in ImageLoader's destructor. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:158: PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients; Why does this need to be a Persistent? It looks strange to trace a persistent handle. https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... File Source/core/svg/SVGImageElement.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... Source/core/svg/SVGImageElement.h:34: class SVGImageElement FINAL : public SVGGraphicsElement, Nit: ',' should come to the head of the next line. https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... File Source/core/svg/SVGImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... Source/core/svg/SVGImageLoader.h:37: SVGImageLoader(SVGImageElement*); Add explicit.
https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... File Source/core/html/HTMLImageElement.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... Source/core/html/HTMLImageElement.h:144: WeakPtr<HTMLFormElement> m_form; On 2014/06/09 11:12:33, haraken wrote: > > WeakPtrWillBeMember is available. It's unrelated to the purpose of this CL. https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... File Source/core/html/HTMLImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/html/HTMLIma... Source/core/html/HTMLImageLoader.h:44: HTMLImageLoader(Element*); On 2014/06/09 11:12:33, haraken wrote: > > Add explicit. Will do. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:129: m_element->deref(); On 2014/06/09 11:12:33, haraken wrote: > > It looks strange that we don't have corresponding code to this. (See my comment > below.) The corresponding code is the destructor of m_keepAlive. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:136: visitor->trace(m_clients); Will remove this. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:427: m_keepAlive = this; On 2014/06/09 11:12:34, haraken wrote: > > I'm afraid this will cause memory leak. If timerFired() is not triggered, this > ImageLoader will kept alive forever. > > I think a right fix would be: > > - Call 'm_keepAlive = m_element' here. > - Call 'm_keepAlive.clear()' in ImageLoader's destructor. I think timerFired() is always triggered. m_keepAlive points this ImageLoader until timerFired is called. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:158: PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients; On 2014/06/09 11:12:34, haraken wrote: > > Why does this need to be a Persistent? It looks strange to trace a persistent > handle. The comment above explains it. Tracing this is my bad. I'll remove it. https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... File Source/core/svg/SVGImageElement.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... Source/core/svg/SVGImageElement.h:34: class SVGImageElement FINAL : public SVGGraphicsElement, On 2014/06/09 11:12:34, haraken wrote: > > Nit: ',' should come to the head of the next line. It's unrelated to the purpose of this CL. https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... File Source/core/svg/SVGImageLoader.h (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/svg/SVGImage... Source/core/svg/SVGImageLoader.h:37: SVGImageLoader(SVGImageElement*); On 2014/06/09 11:12:34, haraken wrote: > > Add explicit. will do.
https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:427: m_keepAlive = this; On 2014/06/10 01:43:09, tkent wrote: > On 2014/06/09 11:12:34, haraken wrote: > > > > I'm afraid this will cause memory leak. If timerFired() is not triggered, this > > ImageLoader will kept alive forever. > > > > I think a right fix would be: > > > > - Call 'm_keepAlive = m_element' here. > > - Call 'm_keepAlive.clear()' in ImageLoader's destructor. > > I think timerFired() is always triggered. m_keepAlive points this ImageLoader > until timerFired is called. It's not clear to me if timerFired() is always triggered. If timerFired() were always triggered, we wouldn't need the following code: if (m_elementIsProtected) m_element->deref(); in ~ImageLoader() in non-oilpan builds. The comment at line 126 also implies that it's possible that timerFired() is not fired and thus there is a case where we need to manually deref the element. If we're not sure, I think it's safer to protect the element than the ImageLoader. Also, in terms of semantics, I think what we want to protect would be not the ImageLoader but the element.
Updated the patch. https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/320253002/diff/20001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:427: m_keepAlive = this; On 2014/06/10 01:59:05, haraken wrote: > On 2014/06/10 01:43:09, tkent wrote: > > On 2014/06/09 11:12:34, haraken wrote: > > > > > > I'm afraid this will cause memory leak. If timerFired() is not triggered, > this > > > ImageLoader will kept alive forever. > > > > > > I think a right fix would be: > > > > > > - Call 'm_keepAlive = m_element' here. > > > - Call 'm_keepAlive.clear()' in ImageLoader's destructor. > > > > I think timerFired() is always triggered. m_keepAlive points this ImageLoader > > until timerFired is called. > > It's not clear to me if timerFired() is always triggered. If timerFired() were > always triggered, we wouldn't need the following code: > > if (m_elementIsProtected) > m_element->deref(); > > in ~ImageLoader() in non-oilpan builds. The comment at line 126 also implies > that it's possible that timerFired() is not fired and thus there is a case where > we need to manually deref the element. > > If we're not sure, I think it's safer to protect the element than the > ImageLoader. Also, in terms of semantics, I think what we want to protect would > be not the ImageLoader but the element. If enable_oilpan=0 and the owner element releases the ownership of the ImageLoader before the element dies, it's possible to call ~ImageLoader without timerFired. However, we have no such code at this moment and the owner element and the ImageLoader always die together. We might have such code in the future. If so, Persistent<ImageLoader> actually has inconsistent semantics with enable_oilpan=0. I have change it to Persistent<Element> in Patch Set 2.
https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.cpp (left): https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.cpp:129: m_element->deref(); Shall we add an ASSERT like this? ASSERT(!m_elementIsProtected) #if ENABLE(OILPAN) ASSERT(!m_keepAlive); #endif https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:150: ImageLoader& m_loader; You might want to change this to a Member. It will clarify the relationship that ImageLoader must outlive its clients. https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:157: PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients; I'm curious why you need to make this map a persistent. Even if you don't make this map a persistent, it will be guaranteed that the ImageLoader is alive when ~ImageLoaderClientRemover touches the ImageLoader. (1) An ImageLoaderClient and an ImageLoader become unreachable. (2) A GC is triggered. (3) The ImageLoaderClient is removed from the map by oilpan's weak processing. ~ImageLoaderClientRemover is called. It can touche the ImageLoader, because the ImageLoader is not yet swept. (4) The ImageLoader is swept.
https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:157: PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients; On 2014/06/10 04:15:13, haraken wrote: > > I'm curious why you need to make this map a persistent. Even if you don't make > this map a persistent, it will be guaranteed that the ImageLoader is alive when > ~ImageLoaderClientRemover touches the ImageLoader. > > (1) An ImageLoaderClient and an ImageLoader become unreachable. > (2) A GC is triggered. > (3) The ImageLoaderClient is removed from the map by oilpan's weak processing. > ~ImageLoaderClientRemover is called. It can touche the ImageLoader, because the > ImageLoader is not yet swept. > (4) The ImageLoader is swept. Though I don't remember the precise mechanism, we need to make this Persistent-in-GarbageCollectedFinalized<> in this case. Making this Member<> causes crash in ImageBitmapTest.ImageBitmapLiveResourcePriority. Please see modules/webdatabse/DatabaseSync.h. modules/webdatabase/DatabseContext.h has non-persistent weak HashMap. It's ok because DatabaseCloser doesn't touch the DatabaseContext. I guess removing weak HashMap entries is done in the weak processing, but finalizing weak HashMap entries is done in the sweep stage?
https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:157: PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients; On 2014/06/10 05:32:46, tkent wrote: > On 2014/06/10 04:15:13, haraken wrote: > > > > I'm curious why you need to make this map a persistent. Even if you don't make > > this map a persistent, it will be guaranteed that the ImageLoader is alive > when > > ~ImageLoaderClientRemover touches the ImageLoader. > > > > (1) An ImageLoaderClient and an ImageLoader become unreachable. > > (2) A GC is triggered. > > (3) The ImageLoaderClient is removed from the map by oilpan's weak processing. > > ~ImageLoaderClientRemover is called. It can touche the ImageLoader, because > the > > ImageLoader is not yet swept. > > (4) The ImageLoader is swept. > > Though I don't remember the precise mechanism, we need to make this > Persistent-in-GarbageCollectedFinalized<> in this case. Making this Member<> > causes crash in ImageBitmapTest.ImageBitmapLiveResourcePriority. > Please see modules/webdatabse/DatabaseSync.h. > modules/webdatabase/DatabseContext.h has non-persistent weak HashMap. It's ok > because DatabaseCloser doesn't touch the DatabaseContext. > > I guess removing weak HashMap entries is done in the weak processing, but > finalizing weak HashMap entries is done in the sweep stage? Given that ImageLoaderClientRemover is off heap, ~ImageLoaderClientRemover should be called promptly when the OwnPtr is removed from the hash map in the weak processing phase (which happens before the sweeping phase). Help me understand: Does the ImageLoaderClient have a Member back to the ImageLoader? (It was not clear from a quick grep.) If the ImageLoaderClient has a Member back to the ImageLoader, this change makes sense because the Member guarantees that the ImageLoader is destructed in the next GC of the GC that destructed the ImageLoaderClient. Would it be possible to make this hash map a static hash map? Unlike DatabaseContext, I don't see any threading issue here.
On 2014/06/10 05:54:10, haraken wrote: > Given that ImageLoaderClientRemover is off heap, ~ImageLoaderClientRemover > should be called promptly when the OwnPtr is removed from the hash map in the > weak processing phase (which happens before the sweeping phase). This is right. ~ImageLoaderClientRemover is called in the weak processing. I think the weak processing for m_clients doesn't happen if both of ImageLoader and ImageLoader client are not reachable from GC roots.
On 2014/06/10 06:58:26, tkent wrote: > > Given that ImageLoaderClientRemover is off heap, ~ImageLoaderClientRemover > > should be called promptly when the OwnPtr is removed from the hash map in the > > weak processing phase (which happens before the sweeping phase). > > This is right. ~ImageLoaderClientRemover is called in the weak processing. > > I think the weak processing for m_clients doesn't happen if both of ImageLoader > and ImageLoader client are not reachable from GC roots. I confirmed this. If m_clients is not a Persistent and both of ImageLoader and ImageLoaderClient are not reachable from GC roots, they are swept together and weak processing doesn't happen.
On 2014/06/10 07:57:11, tkent wrote: > On 2014/06/10 06:58:26, tkent wrote: > > > Given that ImageLoaderClientRemover is off heap, ~ImageLoaderClientRemover > > > should be called promptly when the OwnPtr is removed from the hash map in > the > > > weak processing phase (which happens before the sweeping phase). > > > > This is right. ~ImageLoaderClientRemover is called in the weak processing. > > > > I think the weak processing for m_clients doesn't happen if both of > ImageLoader > > and ImageLoader client are not reachable from GC roots. > > I confirmed this. If m_clients is not a Persistent and both of ImageLoader and > ImageLoaderClient are not reachable from GC roots, they are swept together and > weak processing doesn't happen. Thanks, it looks like things are working relying on a fragile assumption. In order to guarantee that the ImageLoader dies in a next GC cycle following a GC that destructs the ImageLoaderClient, the ImageLoaderClient should have a Member back to the ImageLoader. Currently we have the back Member in the following path: - ImageBitmap is the only ImageLoaderClient. - The ImageBitmap has a Member to the HTMLImageElement. - The HTMLImageElement has a Member to the ImageLoader. To make things clearer, how about making the following changes? - Make ImageLoaderClient directly hold a Member back to the ImageLoader. - Move the persistent hash map to a static hash map and have a good comment above the map.
On 2014/06/10 08:07:13, haraken wrote: > - Make ImageLoaderClient directly hold a Member back to the ImageLoader. > - Move the persistent hash map to a static hash map and have a good comment > above the map. We can't do it. If the HashMap was static, ImageLoaderClient and ImageLoader die together. We need Persistent<> data member for incremental destruction.
On 2014/06/10 08:31:27, tkent wrote:
> On 2014/06/10 08:07:13, haraken wrote:
> > - Make ImageLoaderClient directly hold a Member back to the ImageLoader.
> > - Move the persistent hash map to a static hash map and have a good comment
> > above the map.
>
> We can't do it. If the HashMap was static, ImageLoaderClient and ImageLoader
> die together. We need Persistent<> data member for incremental destruction.
Isn't it because the ImageLoaderClientRemover does not have a Member back to the
ImageLoader?
I guess the following would work:
class ImageLoaderClientRemover {
Member<ImageLoader> m_loader;
ImageLoaderClient* m_client; // This is an intentional raw pointer.
};
static PersistentHeapHashMap<WeakMember<ImageLoaderClient>,
OwnPtr<ImageLoaderClientRemover> > m_clients;
Then things should happen in the following order:
(1) An ImageLoaderClient becomes unreachable.
(2) A GC is triggered.
(3) The ImageLoaderClient is removed from the weak map. The
ImageLoaderClientRemover is destructed. Then ImageLoader becomes unreachable.
(4) A next GC is triggered.
(5) The ImageLoader is destructed.
I think this is exactly the way DatabaseSync is working and we want to implement
the same thing for the ImageLoader. The way the current ImageLoader works looks
like a magic.
Note:
> > - Make ImageLoaderClient directly hold a Member back to the ImageLoader.
^^^ I found that this comment doesn't make sense. We need to make
ImageLoaderClientRemover (not ImageLoaderClient) hold a Member back to the
ImageLoader.
On 2014/06/10 08:40:34, haraken wrote: > > We can't do it. If the HashMap was static, ImageLoaderClient and ImageLoader > > die together. We need Persistent<> data member for incremental destruction. > > Isn't it because the ImageLoaderClientRemover does not have a Member back to the > ImageLoader? We can't make ImageLoaderClientRemover on-heap. If we do so, ~ImageLoaderClientRemover is not called during the weak processing. Also, blink_gc_plugin doesn't allow accessing Member<ImageLoader> in ~ImageLoaderClientRemover. Making ImageLoaderClientRemover::m_loader Persistent<> is not reasonable because the number of Persistent increases.
ah, understood what you meant. LGTM. Sorry about a lot of noise.
Message was sent while issue was closed.
Committed patchset #3 manually as r175876 (presubmit successful).
Message was sent while issue was closed.
Just a random idea (which might not work):
class ImageLoader {
// We no longer need ImageLoaderClientRemover.
HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
};
class ImageLoaderClient {
void trace(Visitor* visitor) {
visitor->registerWeakMembers(this, processWeakMembers);
visitor->trace(m_loader);
}
void processWeakMembers() {
m_loader->willRemoveClient(this);
}
Member<ImageLoader> m_loader;
};
Either way, I'd like to have Mads to take another look at the ImageLoader
change.
Message was sent while issue was closed.
I don't understand the use of persistents in ImageLoader. I added a couple of comments below. https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; I don't think this persistent is doing anything in the oilpan build? m_keepAlive is always either 0 or equal to m_element. Since m_element is already traced if this object is alive it doesn't add anything to have the persistent here. The persistent is holding something alive that is always alive anyway when this object is alive? https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:156: GC_PLUGIN_IGNORE("http://crbug.com/353083") Why does not map have to be a PersistentHeapHashMap? Weak processing only happens if this object is alive anyway, so it should make a difference whether or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is traced. In both situations, the weak processing will only happen if this object ImageLoader object is alive.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:156: GC_PLUGIN_IGNORE("http://crbug.com/353083") On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > Why does not map have to be a PersistentHeapHashMap? Weak processing only > happens if this object is alive anyway, so it should make a difference whether > or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is > traced. In both situations, the weak processing will only happen if this object > ImageLoader object is alive. Sorry, that should be: why does /the/ map have to be ...
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:156: GC_PLUGIN_IGNORE("http://crbug.com/353083") On 2014/06/10 12:37:45, Mads Ager (chromium) wrote: > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > Why does not map have to be a PersistentHeapHashMap? Weak processing only > > happens if this object is alive anyway, so it should make a difference whether > > or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is > > traced. In both situations, the weak processing will only happen if this > object > > ImageLoader object is alive. > > Sorry, that should be: why does /the/ map have to be ... Oh, is that the point here? This persistent makes sure that the map gets traced in the GC where this object dies which means that we get an extra round of weak processing for the map and can therefore do the cleanup during weak processing and not during destruction. Auch, that is complicated. :(
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > I don't think this persistent is doing anything in the oilpan build? m_keepAlive > is always either 0 or equal to m_element. Since m_element is already traced if > this object is alive it doesn't add anything to have the persistent here. The > persistent is holding something alive that is always alive anyway when this > object is alive? Good point. I agree that we can remove this Persistent. https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:156: GC_PLUGIN_IGNORE("http://crbug.com/353083") On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > Why does not map have to be a PersistentHeapHashMap? Weak processing only > happens if this object is alive anyway, so it should make a difference whether > or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is > traced. In both situations, the weak processing will only happen if this object > ImageLoader object is alive. The problem is that the weak processing does not happen if the ImageLoader and the ImageLoaderClient die together, but the weak processing must happen even in that case to keep the consistency of the memory cache. That's why we need to have ugly code like this. Another suggestion that doesn't use persistent maps is written in comment #17.
Message was sent while issue was closed.
On 2014/06/10 12:44:52, Mads Ager (chromium) wrote: > https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... > File Source/core/loader/ImageLoader.h (right): > > https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... > Source/core/loader/ImageLoader.h:156: > GC_PLUGIN_IGNORE("http://crbug.com/353083") > On 2014/06/10 12:37:45, Mads Ager (chromium) wrote: > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > Why does not map have to be a PersistentHeapHashMap? Weak processing only > > > happens if this object is alive anyway, so it should make a difference > whether > > > or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that > is > > > traced. In both situations, the weak processing will only happen if this > > object > > > ImageLoader object is alive. > > > > Sorry, that should be: why does /the/ map have to be ... > > Oh, is that the point here? This persistent makes sure that the map gets traced > in the GC where this object dies which means that we get an extra round of weak > processing for the map and can therefore do the cleanup during weak processing > and not during destruction. Auch, that is complicated. :( Exactly...
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:58: class ImageLoader : public NoBaseWillBeGarbageCollectedFinalized<ImageLoader>, public ImageResourceClient { Nit: FINAL https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/10 12:47:59, haraken wrote: > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > I don't think this persistent is doing anything in the oilpan build? > m_keepAlive > > is always either 0 or equal to m_element. Since m_element is already traced if > > this object is alive it doesn't add anything to have the persistent here. The > > persistent is holding something alive that is always alive anyway when this > > object is alive? > > Good point. I agree that we can remove this Persistent. Yes. But this does point at a behavior change. The behavior semi-preserving change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive is a RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies that the element must be allowed to die or if it is OK to strongly trace m_element regardless of m_elementIsProtected.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:156: GC_PLUGIN_IGNORE("http://crbug.com/353083") On 2014/06/10 12:47:59, haraken wrote: > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > Why does not map have to be a PersistentHeapHashMap? Weak processing only > > happens if this object is alive anyway, so it should make a difference whether > > or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is > > traced. In both situations, the weak processing will only happen if this > object > > ImageLoader object is alive. > > The problem is that the weak processing does not happen if the ImageLoader and > the ImageLoaderClient die together, but the weak processing must happen even in > that case to keep the consistency of the memory cache. > > That's why we need to have ugly code like this. > > Another suggestion that doesn't use persistent maps is written in comment #17. The weak processing doesn't happen if it is not persistent and we cannot let the destructor kill the ImageLoaderClientRemover because it touches the ImageLoader itself. OK, got it. We did this once before in database code; I think Kent wrote that earlier in this review as well and I just didn't get it. :) OK, I understand this persistent. Maybe we can extend the comment to say that this persistent is here to make sure that all ImageLoaderClientRemovers are destructed in weak processing even when the ImageLoader is unreachable? It would be great to get rid of it long term, and there is a bug report to track that.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/10 12:54:28, zerny-chromium wrote: > On 2014/06/10 12:47:59, haraken wrote: > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > I don't think this persistent is doing anything in the oilpan build? > > m_keepAlive > > > is always either 0 or equal to m_element. Since m_element is already traced > if > > > this object is alive it doesn't add anything to have the persistent here. > The > > > persistent is holding something alive that is always alive anyway when this > > > object is alive? > > > > Good point. I agree that we can remove this Persistent. > > Yes. But this does point at a behavior change. The behavior semi-preserving > change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive is a > RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies that > the element must be allowed to die or if it is OK to strongly trace m_element > regardless of m_elementIsProtected. You're right, but as far as I see, m_element is a strong, back pointer to the Element. (For example, we don't have any code that clears m_element in non-oilpan builds.) So I think it's safe to use a Member in oilpan and thus remove m_keepAlive.
Message was sent while issue was closed.
> > Another suggestion that doesn't use persistent maps is written in comment
#17.
>
> The weak processing doesn't happen if it is not persistent and we cannot let
the
> destructor kill the ImageLoaderClientRemover because it touches the
ImageLoader
> itself. OK, got it. We did this once before in database code; I think Kent
wrote
> that earlier in this review as well and I just didn't get it. :)
My proposal is to remove the ImageLoaderClientRemover and let the
ImageLoaderClient do the weak processing for the ImageLoaderClient itself.
> class ImageLoader {
> // We no longer need ImageLoaderClientRemover.
> HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
> };
>
> class ImageLoaderClient {
> void trace(Visitor* visitor) {
> visitor->registerWeakMembers(this, processWeakMembers);
> visitor->trace(m_loader);
> }
>
> void processWeakMembers() {
> m_loader->willRemoveClient(this);
> }
>
> Member<ImageLoader> m_loader;
> };
(I just want to make sure I'm understanding things correctly; I'm fine with the
persistent in a sense that we already have the same issue for DatabaseSync.)
Message was sent while issue was closed.
On 2014/06/10 13:03:39, haraken wrote:
> > > Another suggestion that doesn't use persistent maps is written in comment
> #17.
> >
> > The weak processing doesn't happen if it is not persistent and we cannot let
> the
> > destructor kill the ImageLoaderClientRemover because it touches the
> ImageLoader
> > itself. OK, got it. We did this once before in database code; I think Kent
> wrote
> > that earlier in this review as well and I just didn't get it. :)
>
> My proposal is to remove the ImageLoaderClientRemover and let the
> ImageLoaderClient do the weak processing for the ImageLoaderClient itself.
>
> > class ImageLoader {
> > // We no longer need ImageLoaderClientRemover.
> > HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
> > };
> >
> > class ImageLoaderClient {
> > void trace(Visitor* visitor) {
> > visitor->registerWeakMembers(this, processWeakMembers);
> > visitor->trace(m_loader);
> > }
> >
> > void processWeakMembers() {
> > m_loader->willRemoveClient(this);
> > }
> >
> > Member<ImageLoader> m_loader;
> > };
>
>
> (I just want to make sure I'm understanding things correctly; I'm fine with
the
> persistent in a sense that we already have the same issue for DatabaseSync.)
I'm not sure I follow this concrete proposal Haraken. The weak processing in
ImageLoaderClient will only happen when ImageLoaderClient is alive, not when it
is dead. So we would remove the client when it is alive and not when it is dead.
Message was sent while issue was closed.
On 2014/06/10 13:10:17, Mads Ager (chromium) wrote:
> On 2014/06/10 13:03:39, haraken wrote:
> > > > Another suggestion that doesn't use persistent maps is written in
comment
> > #17.
> > >
> > > The weak processing doesn't happen if it is not persistent and we cannot
let
> > the
> > > destructor kill the ImageLoaderClientRemover because it touches the
> > ImageLoader
> > > itself. OK, got it. We did this once before in database code; I think Kent
> > wrote
> > > that earlier in this review as well and I just didn't get it. :)
> >
> > My proposal is to remove the ImageLoaderClientRemover and let the
> > ImageLoaderClient do the weak processing for the ImageLoaderClient itself.
> >
> > > class ImageLoader {
> > > // We no longer need ImageLoaderClientRemover.
> > > HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
> > > };
> > >
> > > class ImageLoaderClient {
> > > void trace(Visitor* visitor) {
> > > visitor->registerWeakMembers(this, processWeakMembers);
> > > visitor->trace(m_loader);
> > > }
> > >
> > > void processWeakMembers() {
> > > m_loader->willRemoveClient(this);
> > > }
> > >
> > > Member<ImageLoader> m_loader;
> > > };
> >
> >
> > (I just want to make sure I'm understanding things correctly; I'm fine with
> the
> > persistent in a sense that we already have the same issue for DatabaseSync.)
>
> I'm not sure I follow this concrete proposal Haraken. The weak processing in
> ImageLoaderClient will only happen when ImageLoaderClient is alive, not when
it
> is dead. So we would remove the client when it is alive and not when it is
dead.
oh, sorry; I wanted to say:
> class ImageLoaderClient {
> void trace(Visitor* visitor) {
> visitor->registerWeakMembers(this, processWeakMembers);
> visitor->trace(m_loader);
> }
>
> void processWeakMembers() {
> if (!this->isAlive())
> m_loader->willRemoveClient(this);
> }
>
> Member<ImageLoader> m_loader;
> };
processWeakMembers is called at every GC, and this->isAlive becomes false in a
GC where the ImageLoaderClient dies, and thus we can dispatch willRemoveClient
in the "last" GC. Am I missing something?
Message was sent while issue was closed.
On 2014/06/10 13:15:32, haraken wrote:
> On 2014/06/10 13:10:17, Mads Ager (chromium) wrote:
> > On 2014/06/10 13:03:39, haraken wrote:
> > > > > Another suggestion that doesn't use persistent maps is written in
> comment
> > > #17.
> > > >
> > > > The weak processing doesn't happen if it is not persistent and we cannot
> let
> > > the
> > > > destructor kill the ImageLoaderClientRemover because it touches the
> > > ImageLoader
> > > > itself. OK, got it. We did this once before in database code; I think
Kent
> > > wrote
> > > > that earlier in this review as well and I just didn't get it. :)
> > >
> > > My proposal is to remove the ImageLoaderClientRemover and let the
> > > ImageLoaderClient do the weak processing for the ImageLoaderClient itself.
> > >
> > > > class ImageLoader {
> > > > // We no longer need ImageLoaderClientRemover.
> > > > HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
> > > > };
> > > >
> > > > class ImageLoaderClient {
> > > > void trace(Visitor* visitor) {
> > > > visitor->registerWeakMembers(this, processWeakMembers);
> > > > visitor->trace(m_loader);
> > > > }
> > > >
> > > > void processWeakMembers() {
> > > > m_loader->willRemoveClient(this);
> > > > }
> > > >
> > > > Member<ImageLoader> m_loader;
> > > > };
> > >
> > >
> > > (I just want to make sure I'm understanding things correctly; I'm fine
with
> > the
> > > persistent in a sense that we already have the same issue for
DatabaseSync.)
> >
> > I'm not sure I follow this concrete proposal Haraken. The weak processing in
> > ImageLoaderClient will only happen when ImageLoaderClient is alive, not when
> it
> > is dead. So we would remove the client when it is alive and not when it is
> dead.
>
> oh, sorry; I wanted to say:
>
> > class ImageLoaderClient {
> > void trace(Visitor* visitor) {
> > visitor->registerWeakMembers(this, processWeakMembers);
> > visitor->trace(m_loader);
> > }
> >
> > void processWeakMembers() {
> > if (!this->isAlive())
> > m_loader->willRemoveClient(this);
> > }
> >
> > Member<ImageLoader> m_loader;
> > };
>
> processWeakMembers is called at every GC, and this->isAlive becomes false in a
> GC where the ImageLoaderClient dies, and thus we can dispatch willRemoveClient
> in the "last" GC. Am I missing something?
processWeakMember is only called if the trace method is called which only
happens if the object is alive. Weak processing always only happens if the
object that wants to perform weak processing is alive. So, in processWeakMembers
this->isAlive is always true. :)
Message was sent while issue was closed.
On 2014/06/10 13:17:51, Mads Ager (chromium) wrote:
> On 2014/06/10 13:15:32, haraken wrote:
> > On 2014/06/10 13:10:17, Mads Ager (chromium) wrote:
> > > On 2014/06/10 13:03:39, haraken wrote:
> > > > > > Another suggestion that doesn't use persistent maps is written in
> > comment
> > > > #17.
> > > > >
> > > > > The weak processing doesn't happen if it is not persistent and we
cannot
> > let
> > > > the
> > > > > destructor kill the ImageLoaderClientRemover because it touches the
> > > > ImageLoader
> > > > > itself. OK, got it. We did this once before in database code; I think
> Kent
> > > > wrote
> > > > > that earlier in this review as well and I just didn't get it. :)
> > > >
> > > > My proposal is to remove the ImageLoaderClientRemover and let the
> > > > ImageLoaderClient do the weak processing for the ImageLoaderClient
itself.
> > > >
> > > > > class ImageLoader {
> > > > > // We no longer need ImageLoaderClientRemover.
> > > > > HeahHashSet<WeakMember<ImageLoaderClient>> m_clients;
> > > > > };
> > > > >
> > > > > class ImageLoaderClient {
> > > > > void trace(Visitor* visitor) {
> > > > > visitor->registerWeakMembers(this, processWeakMembers);
> > > > > visitor->trace(m_loader);
> > > > > }
> > > > >
> > > > > void processWeakMembers() {
> > > > > m_loader->willRemoveClient(this);
> > > > > }
> > > > >
> > > > > Member<ImageLoader> m_loader;
> > > > > };
> > > >
> > > >
> > > > (I just want to make sure I'm understanding things correctly; I'm fine
> with
> > > the
> > > > persistent in a sense that we already have the same issue for
> DatabaseSync.)
> > >
> > > I'm not sure I follow this concrete proposal Haraken. The weak processing
in
> > > ImageLoaderClient will only happen when ImageLoaderClient is alive, not
when
> > it
> > > is dead. So we would remove the client when it is alive and not when it is
> > dead.
> >
> > oh, sorry; I wanted to say:
> >
> > > class ImageLoaderClient {
> > > void trace(Visitor* visitor) {
> > > visitor->registerWeakMembers(this, processWeakMembers);
> > > visitor->trace(m_loader);
> > > }
> > >
> > > void processWeakMembers() {
> > > if (!this->isAlive())
> > > m_loader->willRemoveClient(this);
> > > }
> > >
> > > Member<ImageLoader> m_loader;
> > > };
> >
> > processWeakMembers is called at every GC, and this->isAlive becomes false in
a
> > GC where the ImageLoaderClient dies, and thus we can dispatch
willRemoveClient
> > in the "last" GC. Am I missing something?
>
> processWeakMember is only called if the trace method is called which only
> happens if the object is alive. Weak processing always only happens if the
> object that wants to perform weak processing is alive. So, in
processWeakMembers
> this->isAlive is always true. :)
ah, makes sense!
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/10 12:58:11, haraken wrote: > On 2014/06/10 12:54:28, zerny-chromium wrote: > > On 2014/06/10 12:47:59, haraken wrote: > > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > > I don't think this persistent is doing anything in the oilpan build? > > > m_keepAlive > > > > is always either 0 or equal to m_element. Since m_element is already > traced > > if > > > > this object is alive it doesn't add anything to have the persistent here. > > The > > > > persistent is holding something alive that is always alive anyway when > this > > > > object is alive? > > > > > > Good point. I agree that we can remove this Persistent. > > > > Yes. But this does point at a behavior change. The behavior semi-preserving > > change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive is a > > RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies > that > > the element must be allowed to die or if it is OK to strongly trace m_element > > regardless of m_elementIsProtected. > > You're right, but as far as I see, m_element is a strong, back pointer to the > Element. (For example, we don't have any code that clears m_element in > non-oilpan builds.) So I think it's safe to use a Member in oilpan and thus > remove m_keepAlive. Thank you for comments. If we don't have m_keepAlive and an ImageLoader and its owner Element are unreachable from GC roots, they are swept. m_keepAlive prevents it because it makes them reachable.
Message was sent while issue was closed.
Thanks Kent. I finally understand what is going on. :) https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/10 23:56:57, tkent wrote: > On 2014/06/10 12:58:11, haraken wrote: > > On 2014/06/10 12:54:28, zerny-chromium wrote: > > > On 2014/06/10 12:47:59, haraken wrote: > > > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > > > I don't think this persistent is doing anything in the oilpan build? > > > > m_keepAlive > > > > > is always either 0 or equal to m_element. Since m_element is already > > traced > > > if > > > > > this object is alive it doesn't add anything to have the persistent > here. > > > The > > > > > persistent is holding something alive that is always alive anyway when > > this > > > > > object is alive? > > > > > > > > Good point. I agree that we can remove this Persistent. > > > > > > Yes. But this does point at a behavior change. The behavior semi-preserving > > > change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive is > a > > > RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies > > that > > > the element must be allowed to die or if it is OK to strongly trace > m_element > > > regardless of m_elementIsProtected. > > > > You're right, but as far as I see, m_element is a strong, back pointer to the > > Element. (For example, we don't have any code that clears m_element in > > non-oilpan builds.) So I think it's safe to use a Member in oilpan and thus > > remove m_keepAlive. > > Thank you for comments. > > If we don't have m_keepAlive and an ImageLoader and its owner Element are > unreachable from GC roots, they are swept. m_keepAlive prevents it because it > makes them reachable. > OK, I see. This is needed because the ImageLoader is reachable from a scheduled timer but oilpan does not know about that pointer. Maybe we can update the comment or bug report with that? We might want to consider implementing support for HeapTimer which keeps its containing object alive when it is scheduled. That would make sure that the containing object could never disappear while there is a pending timer without relying on hacks like these.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:141: GC_PLUGIN_IGNORE("http://crbug.com/353083") I think this persistent should use a separate bug number and an explanation that it is here to keep the ImageLoader alive when a pending timer is the only reference to the imageloader and its element. This is the bug number for allowing the persistent to ensure that weak processing takes place for a collection also in the GC round where the containing object is unreachable.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/11 06:35:14, Mads Ager (chromium) wrote: > On 2014/06/10 23:56:57, tkent wrote: > > On 2014/06/10 12:58:11, haraken wrote: > > > On 2014/06/10 12:54:28, zerny-chromium wrote: > > > > On 2014/06/10 12:47:59, haraken wrote: > > > > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > > > > I don't think this persistent is doing anything in the oilpan build? > > > > > m_keepAlive > > > > > > is always either 0 or equal to m_element. Since m_element is already > > > traced > > > > if > > > > > > this object is alive it doesn't add anything to have the persistent > > here. > > > > The > > > > > > persistent is holding something alive that is always alive anyway when > > > this > > > > > > object is alive? > > > > > > > > > > Good point. I agree that we can remove this Persistent. > > > > > > > > Yes. But this does point at a behavior change. The behavior > semi-preserving > > > > change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive > is > > a > > > > RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies > > > that > > > > the element must be allowed to die or if it is OK to strongly trace > > m_element > > > > regardless of m_elementIsProtected. > > > > > > You're right, but as far as I see, m_element is a strong, back pointer to > the > > > Element. (For example, we don't have any code that clears m_element in > > > non-oilpan builds.) So I think it's safe to use a Member in oilpan and thus > > > remove m_keepAlive. > > > > Thank you for comments. > > > > If we don't have m_keepAlive and an ImageLoader and its owner Element are > > unreachable from GC roots, they are swept. m_keepAlive prevents it because it > > makes them reachable. > > > > OK, I see. This is needed because the ImageLoader is reachable from a scheduled > timer but oilpan does not know about that pointer. Maybe we can update the > comment or bug report with that? We might want to consider implementing support > for HeapTimer which keeps its containing object alive when it is scheduled. That > would make sure that the containing object could never disappear while there is > a pending timer without relying on hacks like these. Just a drive-by comment not related to the essence of this CL: I considered introducing HeapTimer, but I'm afraid that it won't solve our issue. class ImageLoader : public GarbageCollected<ImageLoader> { HeapTimer<ImageLoader> m_timer; // This won't keep the ImageLoader alive, so this won't solve our issue. }; class ImageLoader : public GarbageCollected<ImageLoader> { PersistentHeapTimer<ImageLoader> m_timer; // This will solve our issue, but we cannot use this programming pattern in general. In most cases, we don't want to have the timer to keep the object alive. Instead we want to cancel the timer when the object is destructed. }; Given the above, the current approach of using m_keepAlive seems reasonable to me.
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:141: GC_PLUGIN_IGNORE("http://crbug.com/353083") On 2014/06/11 06:38:41, Mads Ager (chromium) wrote: > I think this persistent should use a separate bug number and an explanation that > it is here to keep the ImageLoader alive when a pending timer is the only > reference to the imageloader and its element. This is the bug number for > allowing the persistent to ensure that weak processing takes place for a > collection also in the GC round where the containing object is unreachable. ok, will file new bug. https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; No, Timer is not related to this issue. As the code comment already says, I think this issue will be resolved when a loading ImageResource has strong references to ImageResourceClient objects.
Message was sent while issue was closed.
On 2014/06/11 06:45:03, haraken wrote: > https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... > File Source/core/loader/ImageLoader.h (right): > > https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... > Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> > m_keepAlive; > On 2014/06/11 06:35:14, Mads Ager (chromium) wrote: > > On 2014/06/10 23:56:57, tkent wrote: > > > On 2014/06/10 12:58:11, haraken wrote: > > > > On 2014/06/10 12:54:28, zerny-chromium wrote: > > > > > On 2014/06/10 12:47:59, haraken wrote: > > > > > > On 2014/06/10 12:34:33, Mads Ager (chromium) wrote: > > > > > > > I don't think this persistent is doing anything in the oilpan build? > > > > > > m_keepAlive > > > > > > > is always either 0 or equal to m_element. Since m_element is already > > > > traced > > > > > if > > > > > > > this object is alive it doesn't add anything to have the persistent > > > here. > > > > > The > > > > > > > persistent is holding something alive that is always alive anyway > when > > > > this > > > > > > > object is alive? > > > > > > > > > > > > Good point. I agree that we can remove this Persistent. > > > > > > > > > > Yes. But this does point at a behavior change. The behavior > > semi-preserving > > > > > change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive > > is > > > a > > > > > RefPtrWillBeMember. I can't really see if the m_elementIsProtected > implies > > > > that > > > > > the element must be allowed to die or if it is OK to strongly trace > > > m_element > > > > > regardless of m_elementIsProtected. > > > > > > > > You're right, but as far as I see, m_element is a strong, back pointer to > > the > > > > Element. (For example, we don't have any code that clears m_element in > > > > non-oilpan builds.) So I think it's safe to use a Member in oilpan and > thus > > > > remove m_keepAlive. > > > > > > Thank you for comments. > > > > > > If we don't have m_keepAlive and an ImageLoader and its owner Element are > > > unreachable from GC roots, they are swept. m_keepAlive prevents it because > it > > > makes them reachable. > > > > > > > OK, I see. This is needed because the ImageLoader is reachable from a > scheduled > > timer but oilpan does not know about that pointer. Maybe we can update the > > comment or bug report with that? We might want to consider implementing > support > > for HeapTimer which keeps its containing object alive when it is scheduled. > That > > would make sure that the containing object could never disappear while there > is > > a pending timer without relying on hacks like these. > > Just a drive-by comment not related to the essence of this CL: I considered > introducing HeapTimer, but I'm afraid that it won't solve our issue. > > class ImageLoader : public GarbageCollected<ImageLoader> { > HeapTimer<ImageLoader> m_timer; // This won't keep the ImageLoader alive, so > this won't solve our issue. > }; > > class ImageLoader : public GarbageCollected<ImageLoader> { > PersistentHeapTimer<ImageLoader> m_timer; // This will solve our issue, but > we cannot use this programming pattern in general. In most cases, we don't want > to have the timer to keep the object alive. Instead we want to cancel the timer > when the object is destructed. > }; > > Given the above, the current approach of using m_keepAlive seems reasonable to > me. Thanks Haraken. Yeah, you are right that it is probably a lot more common that the timer is just cancelled when the object dies. Alright, thanks for explaining this to me. :)
Message was sent while issue was closed.
https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/320253002/diff/60001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:142: RefPtrWillBePersistent<Element> m_keepAlive; On 2014/06/11 06:58:28, tkent wrote: > No, Timer is not related to this issue. > As the code comment already says, I think this issue will be resolved when a > loading ImageResource has strong references to ImageResourceClient objects. Thank you Kent. I misunderstood the code. The Timer is only used when we no longer need to keep the element alive. This persistent is here so that the ImageLoader can keep an unreachable ImageElement and itself alive until the image element has been loaded. That seems a little strange, but it is what other browsers do as well. Sorry for all the noise on this code review. I'll shut up now. ;-) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
