Bug 74477 – [chromium] Set the CCLayerTreeHost pointer on LayerChromium instances eagerly

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Set the CCLayerTreeHost pointer on LayerChromium instances eagerly

[James Robinson]

Created attachment 119139 [details]
Patch

[James Robinson]

Comment on attachment 119139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119139&action=review

Some comments inline about bits that may not be very obvious

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:287
> +    if (!textureUpdater())
> +        createTextureUpdater(layerTreeHost());

this is not obvious. the reason this is here is because createTextureUpdater() is called in setLayerTreeHost, which previously was called in every paint loop immediately before we attempted to create any tiles.

in the lost context case, our behavior looks something like this:

1.) layer added to tree, setLayerTreeHost() called, createTextureUpdater() called to initialize the updater and figure out tiling params
2.) some point later on, context is lost+recreated. CCLayerTreeHost::didRecreateContext() calls cleanupResources() on all layers in tree, which destroys this layer's TextureUpdater (see ContentLayerChromium::cleanupResources()) and clear the tiler (see TiledLayerChromium::cleanupResources())
3.) we try to paint and since we've released the tiler we have to create new tiles and paint them

the old behavior was that between steps (2) and (3) we'd call setLayerTreeHost() and create a new textureUpdater. the new behavior is that we create it here. i believe this is OK but am open to other ways to make this work if someone has a better idea

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:107
> +        RefPtr<MockLayerTreeHost> mock = adoptRef(new MockLayerTreeHost(testHooks, client, settings));
> +        mock->setRootLayer(rootLayer);

this code is moved since setRootLayer() now takes a reference to the CCLayerTreeHost, via LayerChromium::setRootLayer(), and you can't take a reference on a RefCounted object in WebKit before the adoptRef() call or you hit a DCHECK. i think this code is better anyway since it's closer to what we do in the real CCLayerTreeHost initialization path

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:452
> +        m_layerTreeHost->setRootLayer(0);

the old code here was correct, but i think this is slightly cleaner and will work whether we've set a root layer or not. if people are going to cargo-cult shutdown code (which may happen) i'd rather they cargo-cult this version

> LayoutTests/platform/chromium/test_expectations.txt:2767
> +BUGJAMESR : compositing/iframes/iframe-content-flipping.html = IMAGE

the image diff here is that the final frame has 50% alpha with this patch applied due to there being no damage between the .display() and the end of the test.  the change in behavior here is in LayerChromium::setReplicaLayer(). before this patch, this would unconditionally call setNeedsCommit(). on this test, we end up calling setReplicaLayer(0) many times on layers whose replica layers are already NULL. this patch adds an equality check an early-out in this case so we don't call setNeedsCommit(). i believe the new behavior is correct and this test is wrong, there isn't any other fallout from the change.

[Nat Duca]

Comment on attachment 119139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119139&action=review

So happy to have this change happen... all those setLTH calls were me desperately trying to get some old rev out the door and not having a clue what was causing things to go wrong. :)

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:312
>  

How does the ~LayerChromium() flow work? Are we guaranteed to have a null LTH at that point?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:239
> +    if (m_rootLayer && !rootLayer)

Why are you null checking rootlayer here?

Does anything bad happen if you setRootLayer(rootLayer())?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:244
> +    if (!m_rootLayer)

More easily written if(m_rootLayer) m_rootLayer->setXXX()?

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:452
>> +        m_layerTreeHost->setRootLayer(0);
> 
> the old code here was correct, but i think this is slightly cleaner and will work whether we've set a root layer or not. if people are going to cargo-cult shutdown code (which may happen) i'd rather they cargo-cult this version

So, incidentally, what happens if someone tries to attach a root layer using the child?

rootLayer->setLayerTreeHost(m_layerTreeHost)?

For that matter, how would the other path have worked? The Layer::setLTH code doesn't seem to go up to the LTH for the root case and make sure the LTH is disconnected...

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:915
> +}

Is there a way to unit test that texture updater case you were trying to explain earlier? I am still trying to wrap my head around that case, tbh...

[Vangelis Kokkevis]

Comment on attachment 119139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119139&action=review

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:114
>      // If we're changing layer renderers then we need to free up any resources

You should probably update this comment too.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:297
> +    if (m_maskLayer)

Do you want to clear out the current LTH from the old mask layer (if different)?  Same for the replica layer below.

[James Robinson]

Comment on attachment 119139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119139&action=review

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:114
>>      // If we're changing layer renderers then we need to free up any resources
> 
> You should probably update this comment too.

good idea

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:297
>> +    if (m_maskLayer)
> 
> Do you want to clear out the current LTH from the old mask layer (if different)?  Same for the replica layer below.

Ah yes, good catch. Definitely want to do that.  I'll add some logic and a unit test for mask and replicas.

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:312
>>  
> 
> How does the ~LayerChromium() flow work? Are we guaranteed to have a null LTH at that point?

due to reference cycles (see below) when we hit ~LayerChromium we will always have a null LTH

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:239
>> +    if (m_rootLayer && !rootLayer)
> 
> Why are you null checking rootlayer here?
> 
> Does anything bad happen if you setRootLayer(rootLayer())?

setRootLayer(rootLayer()) should be a no-op, since we end up doing m_rootLayer->setLayerTreeHost(this) and LayerChromium::setLTH() early-outs if the LTH pointer matches the LTH pointer it already had

i think this whole thing could be written more concisely as:

{
    if (m_rootLayer == rootLayer)
        return;

    if (m_rootLayer)
        m_rootLayer->setLayerTreeHost(0);

    m_rootLayer = rootLayer;

    if (m_rootLayer)
        m_rootLayer->setLayerTreeHost(this);
}

the interesting transitions are that if we are actually changing the root layer, we need to null out the LTH pointers on the old tree

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:244
>> +    if (!m_rootLayer)
> 
> More easily written if(m_rootLayer) m_rootLayer->setXXX()?

yes. i think i used to have more code here. will simplify

>>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:452
>>> +        m_layerTreeHost->setRootLayer(0);
>> 
>> the old code here was correct, but i think this is slightly cleaner and will work whether we've set a root layer or not. if people are going to cargo-cult shutdown code (which may happen) i'd rather they cargo-cult this version
> 
> So, incidentally, what happens if someone tries to attach a root layer using the child?
> 
> rootLayer->setLayerTreeHost(m_layerTreeHost)?
> 
> For that matter, how would the other path have worked? The Layer::setLTH code doesn't seem to go up to the LTH for the root case and make sure the LTH is disconnected...

the reason that anything at all is needed here is that there's a reference cycle between LayerChromium and CCLayerTreeHost. each LayerChromium in the tree holds a RefPtr<CCLayerTreeHost> and CCLayerTreeHost holds a RefPtr<LayerChromium> to the root layer. thus, simply dropping a reference to a CCLayerTreeHost is unsufficient to actually have it actually get freed.  there are two ways to break this cycle - drop the LTH's reference to the root layer, or drop the root layer's reference to the LTH. the latter only works when you actually have a root layer set - if you don't have one set this pointer walk just crashes.

calling LTH::setRootLayer(0) always works to break this cycle and lets everything get freed up, assuming you aren't holding refs to any other layers in the tree. this is how we actually free up the compositing tree:

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&exact_package=chromium&q=setRootLayer&type=cs&l=2823

otherwise the whole world would leak.

if you're asking what happens if you call setRootLayer(a) and then setRootLayer(b) where 'b' is a child of 'a' then the answer is on the second call we'd clear out the LTH pointer on 'a' and then set it for 'b' and all of its children. at that point, 'a' would be deleted as soon as there are no refs to it from the outside. 'b' and the LTH would then hold a cycle on each other until someone else broke it.

we only move layers between LTHs by calling setRootLayer(). in a world where we had a public API and a private one for CC, LayerChromium::setLayerTreeHost() would definitely *not* be public API, but LTH::setRootLayer() would be.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:915
>> +}
> 
> Is there a way to unit test that texture updater case you were trying to explain earlier? I am still trying to wrap my head around that case, tbh...

i'm not sure how possible it is to have unit tests for specific subtypes of LayerChromium (like ContentLayerChromium and TiledLayerChromium). we do have pretty reasonable layout test coverage for the lost context cases that come into play there

[Antoine Labour]

This is great, I like this change very much.

[Nat Duca]

> > So, incidentally, what happens if someone tries to attach a root layer using the child?
> > 
> > rootLayer->setLayerTreeHost(m_layerTreeHost)?
> > 
> > For that matter, how would the other path have worked? The Layer::setLTH code doesn't seem to go up to the LTH for the root case and make sure the LTH is disconnected...
> 
> the reason that anything at all is needed here is...

Maybe I didn't articulate myself clearly here, or maybe I got confused by your reply... I was just wondering, what happens to the poor sod who is new to our codebase and tries to attach the root layer to the tree host by using LayerChromium:::setLayerTreeHost ***INSTEAD*** of calling CCLTH::setRootLayer. If I read the code right, that will point the layer at the host, but the HOST wont get a pointer to the layer?

[Adrienne Walker]

Comment on attachment 119139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119139&action=review

This patch is excellent.  Setting the layer tree host has always been a bit of a mess.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:287
>> +        createTextureUpdater(layerTreeHost());
> 
> this is not obvious. the reason this is here is because createTextureUpdater() is called in setLayerTreeHost, which previously was called in every paint loop immediately before we attempted to create any tiles.
> 
> in the lost context case, our behavior looks something like this:
> 
> 1.) layer added to tree, setLayerTreeHost() called, createTextureUpdater() called to initialize the updater and figure out tiling params
> 2.) some point later on, context is lost+recreated. CCLayerTreeHost::didRecreateContext() calls cleanupResources() on all layers in tree, which destroys this layer's TextureUpdater (see ContentLayerChromium::cleanupResources()) and clear the tiler (see TiledLayerChromium::cleanupResources())
> 3.) we try to paint and since we've released the tiler we have to create new tiles and paint them
> 
> the old behavior was that between steps (2) and (3) we'd call setLayerTreeHost() and create a new textureUpdater. the new behavior is that we create it here. i believe this is OK but am open to other ways to make this work if someone has a better idea

It seems a little awkward to do this all the way down in createTile.  Would this make more sense at the top of prepareToUpdate, where we also lazily create the tiler?

(Was it not possible to remove cleanupResources or did you just want to leave that for another patch?)

[James Robinson]

(In reply to comment #7)
> > > So, incidentally, what happens if someone tries to attach a root layer using the child?
> > > 
> > > rootLayer->setLayerTreeHost(m_layerTreeHost)?
> > > 
> > > For that matter, how would the other path have worked? The Layer::setLTH code doesn't seem to go up to the LTH for the root case and make sure the LTH is disconnected...
> > 
> > the reason that anything at all is needed here is...
> 
> Maybe I didn't articulate myself clearly here, or maybe I got confused by your reply... I was just wondering, what happens to the poor sod who is new to our codebase and tries to attach the root layer to the tree host by using LayerChromium:::setLayerTreeHost ***INSTEAD*** of calling CCLTH::setRootLayer. If I read the code right, that will point the layer at the host, but the HOST wont get a pointer to the layer?

Don't do that - it doesn't work. We will have to catch that in review until we get a public/private API split on LayerChromium and can hide setLayerTreeHost().

[Dana Jansens]

(In reply to comment #9)
> Don't do that - it doesn't work. We will have to catch that in review until we get a public/private API split on LayerChromium and can hide setLayerTreeHost().

It could be private and make CCLTH::setRootLayer a friend to prevent this happening?

[James Robinson]

(In reply to comment #8)
> It seems a little awkward to do this all the way down in createTile.  Would this make more sense at the top of prepareToUpdate, where we also lazily create the tiler?
> 

ImageLayerChromium does things a bit differently, it access the updater before calling prepareToUpdate()

Enne and I discussed this a bit offline. I think what I'll do instead is clear/create the updater whenever the LayerTreeHost changes and on context recreation clear and then reset the LTH pointer for the whole tree. That way the TextureUpdater is always available when a layer is in a tree and we have a good hook point to re-detect any capabilities on the LTH that may have changed due to the context loss and recreation.

[James Robinson]

Created attachment 119271 [details]
Patch

[James Robinson]

Any further comments on this?

[Adrienne Walker]

(In reply to comment #13)
> Any further comments on this?

LGTM.  Thanks for going back and cleaning up the texture updater initialization too.  :)

[Kenneth Russell]

Comment on attachment 119271 [details]
Patch

If it's good enough for enne it's good enough for me.

[Nat Duca]

(In reply to comment #15)
> (From update of attachment 119271 [details])
> If it's good enough for enne it's good enough for me.

What they said. :)

[Antoine Labour]

LGTM

[James Robinson]

Committed r103011: <http://trac.webkit.org/changeset/103011>

[Adrienne Walker]

Reverted http://trac.webkit.org/changeset/103150 because it causes Aura tests to time out.  See http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Aura%20dbg%29/builds/2281.

I bisected WebKit revs to determine this was the offending commit.  Since WebKit can't be rolled with Aura breaking, I reverted this and its dependent patch in bug 74376.

[James Robinson]

This broke aura because of a subtle change in LayerChromium:setLayerTreeHost(). When transitioning from a valid LTH to another LTH, or from a valid LTH to nil, this patch caused a call to setNeedsDisplay() in the middle of the setLayerTreeHost() function. This is indirectly called when removing a layer from the tree.  In aura, setNeedsDisplay() maps to their ScheduleDraw() function which when running in the test harness synchronously triggers a redraw. Since the LayerChromium tree is in an inconsistent state at this point calculateDraw..() barfs.

Since LayerChromium::setLayerTreeHost() is a completely internal function in this patch I think the right fix is to simply not call out to the client from that function and move the setNeedsCommit()/setNeedsRedraw() calls to the appropriate public APIs.  I've confirmed that doing this fixes the aura tests and will post a new patch ASAP.

[James Robinson]

Created attachment 119961 [details]
Patch

[James Robinson]

Updated patch posted for posterity, not requesting an additional review.

Changes from the previous version are:
1.) Remove setNeedsDisplay() from LayerChromium::setLayerTreeHost, it's not needed
2.) Moved setRootLayer() call from inside WebViewLayerTreeHostImpl's c'tor to its create() function so it runs after the adoptRef() call to avoid triggering m_adoptionIsRequired assertion

Tested with webkit_unit_tests, compositing layout tests, aura_unittests, views_unittests, compositor_unittests, and aura_shell_unittests on linux with use_aura=1.

[James Robinson]

Committed r103290: <http://trac.webkit.org/changeset/103290>

[Nat Duca]

Did we get a new unit test to cover this corner case? It sounds nasty.

[James Robinson]

Unfortunately no, I didn't do that. Could you file a bug on me to write some tests on that?

[Eric Penner]

(In reply to comment #11)
> (In reply to comment #8)
> > It seems a little awkward to do this all the way down in createTile.  Would this make more sense at the top of prepareToUpdate, where we also lazily create the tiler?
> > 
> 
> ImageLayerChromium does things a bit differently, it access the updater before calling prepareToUpdate()
> 
> Enne and I discussed this a bit offline. I think what I'll do instead is clear/create the updater whenever the LayerTreeHost changes and on context recreation clear and then reset the LTH pointer for the whole tree. That way the TextureUpdater is always available when a layer is in a tree and we have a good hook point to re-detect any capabilities on the LTH that may have changed due to the context loss and recreation.

There is at least one other case when the cleanupResources get's called (TiledLayerChromium::prepareToUpdateTiles) and thus resets the TextureUpdater. So I think we should be creating it lazily like was originally discussed (or refactoring cleanupResources and being very careful about when it is called):
https://bugs.webkit.org/show_bug.cgi?id=75288