Bug 103498 – Regression(r135962): ASSERTION FAILED: !m_pedningSyncBackingStores.contains(layer)

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

After r135962, we often hit the following assertion in the layout tests:
18:07:40.667 21028   ASSERTION FAILED: !m_pedningSyncBackingStores.contains(layer)
18:07:40.667 21028   /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp(446) : void WebKit::LayerTreeRenderer::createBackingStoreIfNeeded(WebCore::GraphicsLayer*)
18:07:40.667 21028   1   0x7f592cdd759c WebKit::LayerTreeRenderer::createBackingStoreIfNeeded(WebCore::GraphicsLayer*)
18:07:40.667 21028   2   0x7f592cdd7455 WebKit::LayerTreeRenderer::prepareContentBackingStore(WebCore::GraphicsLayer*)
18:07:40.667 21028   3   0x7f592cdd6f83 WebKit::LayerTreeRenderer::setLayerState(unsigned int, WebKit::WebLayerInfo const&)
18:07:40.667 21028   4   0x7f592cdd57f3 WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)(unsigned int, WebKit::WebLayerInfo const&)>::operator()(WebKit::LayerTreeRenderer*, unsigned int, WebKit::WebLayerInfo const&)
18:07:40.667 21028   5   0x7f592cdd4ee8 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)(unsigned int, WebKit::WebLayerInfo const&)>, void (WebKit::LayerTreeRenderer*, unsigned int, WebKit::WebLayerInfo)>::operator()()
18:07:40.667 21028   6   0x7f5933e86bd6 WTF::Function<void ()>::operator()() const
18:07:40.667 21028   7   0x7f592cdd8750 WebKit::LayerTreeRenderer::syncRemoteContent()
18:07:40.667 21028   8   0x7f592cdd6027 WebKit::LayerTreeRenderer::paintToCurrentGLContext(WebCore::TransformationMatrix const&, float, WebCore::FloatRect const&, unsigned int)
18:07:40.668 21028   9   0x7f592cf07088 EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*)
18:07:40.668 21028   10  0x7f592cf0e60e WebCore::Timer<EwkViewImpl>::fired()
18:07:40.668 21028   11  0x7f5930347e86 WebCore::ThreadTimers::sharedTimerFiredInternal()
18:07:40.668 21028   12  0x7f5930347da7 WebCore::ThreadTimers::sharedTimerFired()

[Chris Dumez]

Created attachment 176441 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 176441 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422
> -    if (it != m_pedningSyncBackingStores.end())
> +    BackingStoreMap::iterator it = m_pendingSyncBackingStores.find(layer);
> +    if (it != m_pendingSyncBackingStores.end())

nice :-)

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:441
> +    if (getBackingStore(graphicsLayer))

shouldn't that be renamed to ensureBackingStore then?

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 176441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176441&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422
> > -    if (it != m_pedningSyncBackingStores.end())
> > +    BackingStoreMap::iterator it = m_pendingSyncBackingStores.find(layer);
> > +    if (it != m_pendingSyncBackingStores.end())
> 
> nice :-)
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:441
> > +    if (getBackingStore(graphicsLayer))
> 
> shouldn't that be renamed to ensureBackingStore then?

I'm not very familiar with this part of the code but ensureBackingStore() seems to make sense. However, I would do such refactoring in a separate patch since this one is really about the assertion hit that's making our bots red at the moment.

[Kenneth Rohde Christiansen]

Comment on attachment 176441 [details]
Patch

Noam should really have a look, but r=cq=me now as we cannot have red bots.

[WebKit Review Bot]

Comment on attachment 176441 [details]
Patch

Rejecting attachment 176441 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
it rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/15003819

[Jocelyn Turcotte]

Comment on attachment 176441 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:455
> +    if (!getBackingStore(graphicsLayer))
>          return;

If I understand correctly the ASSERT happens when two async operations happen before the commit happen.
In this case it's a bit weird, since you're going to delay setting this layers backing store to 0 while it's already 0. So basically you could just remove it's key from the map.

Maybe it would be better to explicitely check if the layer is already in m_pendingSyncBackingStores instead of assuming that getBackingStore will do (and won't change in the future).

[Chris Dumez]

Comment on attachment 176441 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:455
>>          return;
> 
> If I understand correctly the ASSERT happens when two async operations happen before the commit happen.
> In this case it's a bit weird, since you're going to delay setting this layers backing store to 0 while it's already 0. So basically you could just remove it's key from the map.
> 
> Maybe it would be better to explicitely check if the layer is already in m_pendingSyncBackingStores instead of assuming that getBackingStore will do (and won't change in the future).

You are right, I can improve this. I'll upload a new proposal soon.

[Chris Dumez]

Created attachment 176457 [details]
Patch

New proposal based on Jocelyn's feedback.

I'm now checking the pending map explicitly which allows for some optimizations in the case where we have both a pending creation AND a pending removal (in which case we can simply cancel the pending operation and return early).

[Kenneth Rohde Christiansen]

Comment on attachment 176457 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:473
> +    // Check if the layout already has a backing store (committed or pending).
> +    CoordinatedBackingStore* backingStore = static_cast<CoordinatedBackingStore*>(layer->backingStore().get());
> +    BackingStoreMap::iterator it = m_pendingSyncBackingStores.find(layer);
> +    if (it != m_pendingSyncBackingStores.end()) {
> +        CoordinatedBackingStore* pendingBackingStore = it->value.get();

Isnt that the same code as above? separate out in a function?

[Chris Dumez]

Comment on attachment 176457 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:473
>> +        CoordinatedBackingStore* pendingBackingStore = it->value.get();
> 
> Isnt that the same code as above? separate out in a function?

It's not the same. It is actually the opposite:
(backingStore && !pendingBackingStore) vs (!backingStore && pendingBackingStore)

[Dongseong Hwang]

(In reply to comment #7)
> (From update of attachment 176441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176441&action=review
> 
> >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:455
> >>          return;
> > 
> > If I understand correctly the ASSERT happens when two async operations happen before the commit happen.
> > In this case it's a bit weird, since you're going to delay setting this layers backing store to 0 while it's already 0. So basically you could just remove it's key from the map.
> > 
> > Maybe it would be better to explicitely check if the layer is already in m_pendingSyncBackingStores instead of assuming that getBackingStore will do (and won't change in the future).
> 
> You are right, I can improve this. I'll upload a new proposal soon.

Thank you so much for reporting and fixing!

Jocelyn is right.

LayerTreeCoordinator::performScheduleLayer(? - sorry, now I can not see code.) flushes all layer as traversing layer tree and then send DidRenderFrame message.
It is why request to sync layer states can be called once per layer.

[Jocelyn Turcotte]

Comment on attachment 176457 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:482
> +    }
> +    if (!backingStore)

Unless I'm missing something it feels like this fall through is impossible if (it != m_pendingSyncBackingStores.end());
This is assuming that only backingStore XOR pendingBackingStore can be non-null.

If we want to create and the layer is in the map then we either need to "remove the removal" if pendingBackingStore == 0 or else do nothing.
If we want to remove, same thing but remove the layer from the map only if pendingBackingStore != 0.

In other words, couldn't you could simplify the two first ifs with "if (it != m_pendingSyncBackingStores.end() && !it->value.get())" and always return in that block?

[Chris Dumez]

Comment on attachment 176457 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:482
>> +    if (!backingStore)
> 
> Unless I'm missing something it feels like this fall through is impossible if (it != m_pendingSyncBackingStores.end());
> This is assuming that only backingStore XOR pendingBackingStore can be non-null.
> 
> If we want to create and the layer is in the map then we either need to "remove the removal" if pendingBackingStore == 0 or else do nothing.
> If we want to remove, same thing but remove the layer from the map only if pendingBackingStore != 0.
> 
> In other words, couldn't you could simplify the two first ifs with "if (it != m_pendingSyncBackingStores.end() && !it->value.get())" and always return in that block?

Actually, I believe it is possible. This handles 2 cases:
1. The layer has no backing store and no pending operation (layer->backingStore() returns NULL and m_pendingSyncBackingStores.find(layer) does not find anything)
2. The layer has a pending backing store removal

[Dongseong Hwang]

Comment on attachment 176457 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:461
> +    m_pendingSyncBackingStores.add(layer, newBackingStore);

How about changing m_pendingSyncBackingStores.set(layer, newBackingStore) and remove ASSERT.
AFAIK, HashMap::set means
if (!map.contain(key))
   map.add(key, value)
else
   map.set(key, value)

It can reduce code. but we need above comment yet, because this situation is a bit hard to understand.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:486
> +    m_pendingSyncBackingStores.add(layer, 0);

How about changing m_pendingSyncBackingStores.set(layer, 0);

I really appreciate fixing this bug.

However, in my understanding, I can not understand how this case occurs. Of course, I may misunderstand something.
This case means LayerTreeRenderer::setLayerState() can be called more than twice for single layer before next flush.
LayerTreeRenderer::setLayerState() only calls LayerTreeRenderer::createBackingStoreIfNeeded or LayerTreeRenderer::removeBackingStoreIfNeeded.

In the detail, LayerTreeCooridinator::performScheduleFlush(?) traverses layer tree and send sync layer states message.
After traversing, LayerTreeCooridinator::performScheduleFlush(?) sends DidRenderFrame message.

This bug implies two possibilities.
1. LayerTreeCooridinator::performScheduleFlush(?) does not send DidRenderFrame message although traversing layer tree to sync layer state.
2. LayerTreeCooridinator::performScheduleFlush(?) can pass by single layer twice during traversing.

I think both are impossible.

[Chris Dumez]

Comment on attachment 176457 [details]
Patch

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

>>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:482
>>> +    if (!backingStore)
>> 
>> Unless I'm missing something it feels like this fall through is impossible if (it != m_pendingSyncBackingStores.end());
>> This is assuming that only backingStore XOR pendingBackingStore can be non-null.
>> 
>> If we want to create and the layer is in the map then we either need to "remove the removal" if pendingBackingStore == 0 or else do nothing.
>> If we want to remove, same thing but remove the layer from the map only if pendingBackingStore != 0.
>> 
>> In other words, couldn't you could simplify the two first ifs with "if (it != m_pendingSyncBackingStores.end() && !it->value.get())" and always return in that block?
> 
> Actually, I believe it is possible. This handles 2 cases:
> 1. The layer has no backing store and no pending operation (layer->backingStore() returns NULL and m_pendingSyncBackingStores.find(layer) does not find anything)
> 2. The layer has a pending backing store removal

I think I have just understood your comment, sorry for the delay :) I'll try to simplify as advised.

[Dongseong Hwang]

(In reply to comment #15)
> (From update of attachment 176457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176457&action=review
> I think I have just understood your comment, sorry for the delay :) I'll try to simplify as advised.

Why you said apologize. I'm one who made this bug :) I'm sorry folks.

Jocelyn and Kenneth, do you think Comment #14 is valid? Today, I have no confidence for my coding.

[Chris Dumez]

Created attachment 176467 [details]
Patch

Simplify the checks a bit based on Jocelyn's feedback. This is still not exactly what Jocelyn hoped but somehow I don't understand how I can simplify more and not loose functionality (yet).

[Chris Dumez]

Created attachment 176469 [details]
Patch

Fixed a typo in one of the comments.

[Dongseong Hwang]

(In reply to comment #18)
> Created an attachment (id=176469) [details]
> Patch
> 
> Fixed a typo in one of the comments.

LGTM

Could you tell me what layout test hits this assertion. I want to test too :)

[Chris Dumez]

(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=176469) [details] [details]
> > Patch
> > 
> > Fixed a typo in one of the comments.
> 
> LGTM
> 
> Could you tell me what layout test hits this assertion. I want to test too :)

It is not always the same tests asserting. But basically, if you run all compositing/ tests, you should be able to reproduce it.

See example of crashes on the bot:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r135996%20(6437)/results.html

[Jussi Kukkonen (jku)]

(In reply to comment #19)
> Could you tell me what layout test hits this assertion. I want to test too :)

I happened to have a list so here you go, but you really should be looking at the build bots after your commit lands, that's where you'd see the failures...

compositing/geometry/ancestor-overflow-change.html
compositing/geometry/bounds-clipped-composited-child.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/layer-creation/fixed-position-out-of-view.html
compositing/overflow/fixed-position-ancestor-clip.
compositing/overflow/overflow-visible-with-touch.html
compositing/repaint/invalidations-on-composited-layers.html
compositing/repaint/layer-repaint-rects.html
compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html
compositing/rtl/rtl-iframe-absolute-overflow.html
compositing/rtl/rtl-iframe-fixed-overflow-scrolled.html
compositing/tiling/huge-layer-add-remove-child.html
compositing/tiling/huge-layer-img.html
compositing/tiling/huge-layer-with-layer-children-resize.html
compositing/visibility/layer-visible-content.html
compositing/visibility/visibility-composited-transforms.html

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #19)
> > Could you tell me what layout test hits this assertion. I want to test too :)
> 
> I happened to have a list so here you go, but you really should be looking at the build bots after your commit lands, that's where you'd see the failures...
> 
> compositing/geometry/ancestor-overflow-change.html
> compositing/geometry/bounds-clipped-composited-child.html
> compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
> compositing/layer-creation/fixed-position-out-of-view.html
> compositing/overflow/fixed-position-ancestor-clip.
> compositing/overflow/overflow-visible-with-touch.html
> compositing/repaint/invalidations-on-composited-layers.html
> compositing/repaint/layer-repaint-rects.html
> compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html
> compositing/rtl/rtl-iframe-absolute-overflow.html
> compositing/rtl/rtl-iframe-fixed-overflow-scrolled.html
> compositing/tiling/huge-layer-add-remove-child.html
> compositing/tiling/huge-layer-img.html
> compositing/tiling/huge-layer-with-layer-children-resize.html
> compositing/visibility/layer-visible-content.html
> compositing/visibility/visibility-composited-transforms.html

I'll watch the bots when it lands. But as I said, I can reproduce the assert hit locally by running all compositing/ tests and I confirmed that I no longer hit it with my fix.

[WebKit Review Bot]

Comment on attachment 176469 [details]
Patch

Clearing flags on attachment: 176469

Committed r136009: <http://trac.webkit.org/changeset/136009>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Just FYI, it appears there is still a bug somewhere (not exactly related to this assertion problem).

We sometimes (rarely) crash in TextureMapperLayer::setBackingStore(PassRefPtr<TextureMapperBackingStore>) due to unrefing a m_backingStore that already has a ref count of 0. This happens when you try to assign the same backing store to itself.

Therefore, I added the following assertion:
ASSERT(it->key->backingStore() != it->value);

in LayerTreeRenderer::commitPendingBackingStoreOperations(), right before the it->key->setBackingStore(it->value); to make sure we never reassign the same backing store to a layer. Sadly, we sometimes hit this assertion :/

I'm still investigating to find how this is possible since we carefully control what goes into m_pendingSyncBackingStores.

[Chris Dumez]

(In reply to comment #25)
> Just FYI, it appears there is still a bug somewhere (not exactly related to this assertion problem).
> 
> We sometimes (rarely) crash in TextureMapperLayer::setBackingStore(PassRefPtr<TextureMapperBackingStore>) due to unrefing a m_backingStore that already has a ref count of 0. This happens when you try to assign the same backing store to itself.
> 
> Therefore, I added the following assertion:
> ASSERT(it->key->backingStore() != it->value);
> 
> in LayerTreeRenderer::commitPendingBackingStoreOperations(), right before the it->key->setBackingStore(it->value); to make sure we never reassign the same backing store to a layer. Sadly, we sometimes hit this assertion :/
> 
> I'm still investigating to find how this is possible since we carefully control what goes into m_pendingSyncBackingStores.

This may be caused by https://bugs.webkit.org/show_bug.cgi?id=103527
I need to try a bit more (since it is not easy to reproduce) but in any case, I believe the fix from Bug 103527 is needed.