Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22929012:
Change Canvas2DLayerBridge to stay alive until last mailbox is returned. (Closed)
|
Created:
7 years, 4 months ago by Justin Novosad Modified:
7 years, 3 months ago CC:
blink-reviews, eae+blinkwatch, dglazkov+blink, Rik, Stephen Chennney, jeez, pdr. Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionChange Canvas2DLayerBridge to stay alive until last mailbox is returned.
This change modifies the destruction mechanism for Canvas2DLayerBridge.
When the owning ImageBuffer object is destroyed, the Canvas2DLayerBridge
will no longer be destroyed if it has unreleased texture mailboxes.
Instead, the object is leaked and becomes responsible for self-destructing
as soon as its outstanding texture mailboxes are returned by the compositor.
BUG=274113
TEST=leak and use after free verified by valgrind and asan layout test runs. Proper tear-down inspected and validated manually using debugger.
R=senorblanco@chromium.org,piman@chromium.org,danakj@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157147
Patch Set 1 #
Total comments: 7
Patch Set 2 : Re-write to use RefPtr and empty mailbox #Patch Set 3 : Fixing unit tests #
Total comments: 2
Patch Set 4 : Make prepareMailbox return false during tear-down #
Total comments: 3
Patch Set 5 : ASSERT(!m_destructionInProgress) in prepareMailbox #Patch Set 6 : +1 -> -1 #Patch Set 7 : preparMailbox to handle destructionInProgress #Patch Set 8 : #Patch Set 9 : compile fix #Patch Set 10 : gcc build fix #
Total comments: 6
Patch Set 11 : applied senorblanco feedback #
Total comments: 1
Patch Set 12 : removed ->ref() #Patch Set 13 : Trivial fix for layout test failure #
Messages
Total messages: 32 (0 generated)
PTAL https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:106: m_layer->layer()->removeFromParent(); This line appears to be necessary if we want the mailboxes to be released without having to produce new ones. Open issue: are texture layers always leaf nodes in the compositor tree? If not, perhaps we need to worry about re attaching the children of this layer to the parent layer?
https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:106: m_layer->layer()->removeFromParent(); On 2013/08/20 15:51:10, junov wrote: > This line appears to be necessary if we want the mailboxes to be released > without having to produce new ones. Open issue: are texture layers always leaf > nodes in the compositor tree? If not, perhaps we need to worry about re > attaching the children of this layer to the parent layer? You could also set the texture mailbox on the layer to be empty, then that would be passed to the compositor thread and the old one would be released.
LGTM for the mailbox logic. One comment below. https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:106: m_layer->layer()->removeFromParent(); On 2013/08/20 15:51:10, junov wrote: > This line appears to be necessary if we want the mailboxes to be released > without having to produce new ones. Open issue: are texture layers always leaf > nodes in the compositor tree? If not, perhaps we need to worry about re > attaching the children of this layer to the parent layer? I'll let the blink experts confirm, but the fact that you GraphicsLayer::unregisterContentsLayer it either way means you're safe, I think. https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:134: delete this; Could it make sense to make Canvas2DLayerBridge refcounted, and have each of the MailboxInfo keep a ref while it's in the MailboxInUse state? You could move the waitSyncPoint / m_image reset logic to the destructor and that way you don't need to add deleteIfPossible logic in the various places.
On 2013/08/20 17:03:06, piman wrote: > > Could it make sense to make Canvas2DLayerBridge refcounted, and have each of the > MailboxInfo keep a ref while it's in the MailboxInUse state? Yes, that is an excellent idea. To be honest I didn't really like the special-purpose smart pointer thing I was doing.
On 2013/08/20 16:57:08, danakj wrote: > You could also set the texture mailbox on the layer to be empty, then that would > be passed to the compositor thread and the old one would be released. Ok, that sounds like a safer approach. What do you mean by an empty mailbox? Do I put texture id 0 in it? Do I give it a null mailbox name?
Please try to use refcounting, as piman suggests. The ownership model gets really muddy with this patch. https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:134: delete this; On 2013/08/20 17:03:07, piman wrote: > Could it make sense to make Canvas2DLayerBridge refcounted, and have each of the > MailboxInfo keep a ref while it's in the MailboxInUse state? > You could move the waitSyncPoint / m_image reset logic to the destructor and > that way you don't need to add deleteIfPossible logic in the various places. +1000 to this. This poor-man's refcounting here looks confusing and difficult to maintain. https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h (right): https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h:48: // Pointer class, for Canva2DLayerBridge. Similar to OwnPtr, with special Nit: Canva -> Canvas. https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h:151: m_ownPtr.leakPtr()->destroy(); This looks kind of error-prone to me.
On 2013/08/20 17:19:23, junov wrote: > On 2013/08/20 16:57:08, danakj wrote: > > You could also set the texture mailbox on the layer to be empty, then that > would > > be passed to the compositor thread and the old one would be released. > Ok, that sounds like a safer approach. What do you mean by an empty mailbox? Do > I put texture id 0 in it? Do I give it a null mailbox name? The default constructor for TextureMailbox will give you an empty one. It has a 0 texture id, and no release callback.
On 2013/08/20 17:33:35, danakj wrote: > On 2013/08/20 17:19:23, junov wrote: > > On 2013/08/20 16:57:08, danakj wrote: > > > You could also set the texture mailbox on the layer to be empty, then that > > would > > > be passed to the compositor thread and the old one would be released. > > Ok, that sounds like a safer approach. What do you mean by an empty mailbox? > Do > > I put texture id 0 in it? Do I give it a null mailbox name? > > The default constructor for TextureMailbox will give you an empty one. It has a > 0 texture id, and no release callback. s/0 texture id/empty mailbox string/
On 2013/08/20 17:03:06, piman wrote: > https://codereview.chromium.org/22929012/diff/1/Source/core/platform/graphics... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:106: > m_layer->layer()->removeFromParent(); > On 2013/08/20 15:51:10, junov wrote: > > This line appears to be necessary if we want the mailboxes to be released > > without having to produce new ones. Open issue: are texture layers always leaf > > nodes in the compositor tree? If not, perhaps we need to worry about re > > attaching the children of this layer to the parent layer? > > I'll let the blink experts confirm, but the fact that you > GraphicsLayer::unregisterContentsLayer it either way means you're safe, I think. Texture layers are always leaf nodes and the unregisterContentsLayer call makes sure that the layer won't be referenced. You don't need to ...->removeFromParent()
New patch. Using RefPtr to manage destruction. Using empty mailboxes to phase-out the layer's textures.
Another new patch: Fixed unit tests: Must not allocate RefCounted objects on the stack
https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:266: return true; Should this be false, re piman's earlier comment?
https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:266: return true; On 2013/08/20 21:45:17, danakj wrote: > Should this be false, re piman's earlier comment? Actually I'm not sure why this should be false, then it doesn't do anything. Maybe I misunderstood the earlier one.
On 2013/08/20 21:46:15, danakj wrote: > https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... > File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): > > https://codereview.chromium.org/22929012/diff/21001/Source/core/platform/grap... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:266: return true; > On 2013/08/20 21:45:17, danakj wrote: > > Should this be false, re piman's earlier comment? > > Actually I'm not sure why this should be false, then it doesn't do anything. > Maybe I misunderstood the earlier one. I just tried returning false in prepareMailbox, and the layer tear-down still works just fine, which contradicts what I was observing earlier. I investigated a bit further and it turns out that prepareMailbox is no-longer being invoked after the layer is unregistered. I am not sure what the code is doing differently WRT patch 1 to explain this change in behavior.
https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:119: if (this->refCount() == m_liveMailboxCount + 1 && !m_destructionInProgress) { I don't know if this is kosher in Webkit land to play with the refcount - it usually makes things very fragile if something else takes a reference. What I was thinking of was keeping an explicit "destroy" (or whatever) that sets m_destructionInProgress and removes from the tree, and refcount is just needed to keep the MailboxInfos alive while we're waiting for them to be returned. https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:266: return false; we called clearTexture() when setting m_destructionInProgress, so returning false means we're not setting a new texture. returning true would mean "yes we changed the mailbox, but hey, it's still null"
On 2013/08/20 22:09:00, piman wrote: > https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... > File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): > > https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:119: if > (this->refCount() == m_liveMailboxCount + 1 && !m_destructionInProgress) { > I don't know if this is kosher in Webkit land to play with the refcount - it > usually makes things very fragile if something else takes a reference. What I > was thinking of was keeping an explicit "destroy" (or whatever) that sets > m_destructionInProgress and removes from the tree, and refcount is just needed > to keep the MailboxInfos alive while we're waiting for them to be returned. IMHO this condition is actually less fragile than explicit destroy, especially in cases where there can be multiple references. The explicit destroy requires the call site to know (or detect) that it is about to release the last outstanding external reference. The code, as it is now, makes no assumption as to how many external references may exist, it simply triggers the tear-down when all remaining references (if any) are from MailboxInfo objects that are owned by the LayerBridge itself (internal references). The "+ 1" in the code is just because this condition is evaluated before calling the base class unref(). To make that more readable, perhaps I should make it a "- 1" on the other side of the ==
Try failures prove that that there is in fact a need to handle the m_destructionInProgress case in prepareMailbox. Sending an empty mailbox and returning true seems to be the right way to get outstanding mailboxes to be released ASAP.
On Wed, Aug 21, 2013 at 6:05 AM, <junov@chromium.org> wrote: > On 2013/08/20 22:09:00, piman wrote: > > https://codereview.chromium.**org/22929012/diff/1010/Source/** > core/platform/graphics/**chromium/Canvas2DLayerBridge.**cpp<https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp> > >> File Source/core/platform/graphics/**chromium/Canvas2DLayerBridge.**cpp >> (right): >> > > > https://codereview.chromium.**org/22929012/diff/1010/Source/** > core/platform/graphics/**chromium/Canvas2DLayerBridge.**cpp#newcode119<https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp#newcode119> > >> Source/core/platform/graphics/**chromium/Canvas2DLayerBridge.**cpp:119: >> if >> (this->refCount() == m_liveMailboxCount + 1 && !m_destructionInProgress) { >> I don't know if this is kosher in Webkit land to play with the refcount - >> it >> usually makes things very fragile if something else takes a reference. >> What I >> was thinking of was keeping an explicit "destroy" (or whatever) that sets >> m_destructionInProgress and removes from the tree, and refcount is just >> needed >> to keep the MailboxInfos alive while we're waiting for them to be >> returned. >> > > IMHO this condition is actually less fragile than explicit destroy, > especially > in cases where there can be multiple references. The explicit destroy > requires > the call site to know (or detect) that it is about to release the last > outstanding external reference. The code, as it is now, makes no > assumption as > to how many external references may exist, it simply triggers the > tear-down when > all remaining references (if any) are from MailboxInfo objects that are > owned by > the LayerBridge itself (internal references). The "+ 1" in the code is > just > because this condition is evaluated before calling the base class unref(). > To > make that more readable, perhaps I should make it a "- 1" on the other > side of > the == > > My point is that whether the layer is in the tree or not shouldn't depend on who has a reference on the C++ object. In Chrome speak, if you were to post a task referencing this object - that's unrelated to the ImageBuffer state - you'd arbitrarily keep the layer in the tree for longer than you want. Also, since the ImageBuffer is what owns the canvas, you'd have a chance to dereference it after it's destroyed. Your only defense is "I know exactly what keeps a reference to the Canvas2DLayerBridge". That's fragile wrt future changes. You want Canvas2DLayerBridge to be logically owned by ImageBuffer, so there's no question about who is supposed to call destroy, even if some other class keeps a ref. If you want to be even safer, you could have a tiny class, Canvas2DLayerBridgeHolder, that owns the reference on behalf of the ImageBuffer, and calls destroy in its destructor. You can then put the Canvas2DLayerBridgeHolder in an OwnPtr (or as an inline field?) in ImageBuffer. > > > https://codereview.chromium.**org/22929012/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/1010/Source/core/platform/graph... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:119: if (this->refCount() == m_liveMailboxCount + 1 && !m_destructionInProgress) { On 2013/08/20 22:09:00, piman wrote: > I don't know if this is kosher in Webkit land to play with the refcount - it > usually makes things very fragile if something else takes a reference. What I > was thinking of was keeping an explicit "destroy" (or whatever) that sets > m_destructionInProgress and removes from the tree, and refcount is just needed > to keep the MailboxInfos alive while we're waiting for them to be returned. I agree that this is an unfortunate pattern. Of the two options I think destroy() is less fragile than messing with the refcount, although still not ideal (since you have to remember to call it). Another option would be to split the LayerBridge in two, so that you can distinguish between the refcounts on one half versus the other.
Gotcha. That sounds good.
New Patch.
https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:51: m_ptr.clear(); MicroNit: This is probably redundant, since we'll be called from the destructor anyway. https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:63: const_cast<Canvas2DLayerBridgePtr*>(&other)->m_ptr.clear(); // transfer of ownership Hmm. I wonder if you could have a private const clear() function that breaks const-correctness, to avoid the const-cast here. Might need to make the refcount mutable, though. :( https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:345: m_liveMailboxCount++; m_liveMailboxCount seems to be only written now, and not read. Remove? https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h (right): https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h:58: void clear(); Nit: looks like this function is only called from the destructor; could just put its contents there and remove it. https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h:101: // Non-virtual overload of RefCounted::deref. RefPtr calls this one. Nit: stale comment? https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.h:107: bool isDead(); This doesn't seem to be defined anywhere.
On 2013/08/23 14:45:27, Stephen White wrote: > https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... > File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): > > https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:51: > m_ptr.clear(); > MicroNit: This is probably redundant, since we'll be called from the destructor > anyway. Not redundant, clear can be called directly, from the assignment operator, or from the copy constructor which uses the assignment operator. > > https://codereview.chromium.org/22929012/diff/52001/Source/core/platform/grap... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:63: > const_cast<Canvas2DLayerBridgePtr*>(&other)->m_ptr.clear(); // transfer of > ownership > Hmm. I wonder if you could have a private const clear() function that breaks > const-correctness, to avoid the const-cast here. Might need to make the refcount > mutable, though. :( But the semantics of clear are not const. I have an idea for preserving const integrity. We could instead have a release method that returns a PassRefPtr<Canvas2DLayerBridge>, and constructor and assignment operators that can take a PassRefPtr. That way transfer of ownership from one Canvas2DLayerBridgePtr to another would be explcit. Only danger is that if the PassRefPtr goes out of scope without being assigned to a new Canvas2DLayerBridgePtr, then destroy() won't get called. I think I can guard against that by putting the WARN_UNUSED_RETURN macro on the release method.
New Patch
https://codereview.chromium.org/22929012/diff/63001/Source/core/platform/grap... File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/22929012/diff/63001/Source/core/platform/grap... Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:335: this->ref(); // Because we are adopting an already referenced object I think you could avoid doing this explicit ref() by skipping the adoptRef() on the line below, and just assigning directly.
On 2013/08/26 21:15:43, Stephen White wrote: > https://codereview.chromium.org/22929012/diff/63001/Source/core/platform/grap... > File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right): > > https://codereview.chromium.org/22929012/diff/63001/Source/core/platform/grap... > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:335: this->ref(); > // Because we are adopting an already referenced object > I think you could avoid doing this explicit ref() by skipping the adoptRef() on > the line below, and just assigning directly. Oy, me so nooby! New Patch.
LGTM
LGTM2
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/22929012/70001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/22929012/91001
Message was sent while issue was closed.
Change committed as 157147 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
