Bug 103498 - Regression(r135962): ASSERTION FAILED: !m_pedningSyncBackingStores.contains(layer)
Summary: Regression(r135962): ASSERTION FAILED: !m_pedningSyncBackingStores.contains(l...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 103171
  Show dependency treegraph
 
Reported: 2012-11-28 03:03 PST by Chris Dumez
Modified: 2012-11-28 07:44 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2012-11-28 03:17 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2012-11-28 04:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.10 KB, patch)
2012-11-28 05:47 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.09 KB, patch)
2012-11-28 05:49 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-11-28 03:03:17 PST
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()
Comment 1 Chris Dumez 2012-11-28 03:17:17 PST
Created attachment 176441 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-11-28 03:19:11 PST
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?
Comment 3 Chris Dumez 2012-11-28 03:25:01 PST
(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.
Comment 4 Kenneth Rohde Christiansen 2012-11-28 03:32:51 PST
Comment on attachment 176441 [details]
Patch

Noam should really have a look, but r=cq=me now as we cannot have red bots.
Comment 5 WebKit Review Bot 2012-11-28 03:55:03 PST
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
Comment 6 Jocelyn Turcotte 2012-11-28 04:10:05 PST
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).
Comment 7 Chris Dumez 2012-11-28 04:17:41 PST
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.
Comment 8 Chris Dumez 2012-11-28 04:44:25 PST
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).
Comment 9 Kenneth Rohde Christiansen 2012-11-28 04:45:56 PST
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?
Comment 10 Chris Dumez 2012-11-28 04:47:12 PST
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)
Comment 11 Dongseong Hwang 2012-11-28 04:59:47 PST
(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.
Comment 12 Jocelyn Turcotte 2012-11-28 05:02:00 PST
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?
Comment 13 Chris Dumez 2012-11-28 05:06:42 PST
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
Comment 14 Dongseong Hwang 2012-11-28 05:24:19 PST
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.
Comment 15 Chris Dumez 2012-11-28 05:30:52 PST
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.
Comment 16 Dongseong Hwang 2012-11-28 05:40:49 PST
(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.
Comment 17 Chris Dumez 2012-11-28 05:47:11 PST
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).
Comment 18 Chris Dumez 2012-11-28 05:49:56 PST
Created attachment 176469 [details]
Patch

Fixed a typo in one of the comments.
Comment 19 Dongseong Hwang 2012-11-28 06:02:59 PST
(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 :)
Comment 20 Chris Dumez 2012-11-28 06:05:12 PST
(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
Comment 21 Jussi Kukkonen (jku) 2012-11-28 06:13:41 PST
(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
Comment 22 Chris Dumez 2012-11-28 06:15:29 PST
(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.
Comment 23 WebKit Review Bot 2012-11-28 06:52:06 PST
Comment on attachment 176469 [details]
Patch

Clearing flags on attachment: 176469

Committed r136009: <http://trac.webkit.org/changeset/136009>
Comment 24 WebKit Review Bot 2012-11-28 06:52:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Chris Dumez 2012-11-28 07:00:45 PST
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.
Comment 26 Chris Dumez 2012-11-28 07:44:11 PST
(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.