Full Text Bug Listing

Chris Dumez

Reported 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()

Chris Dumez

Comment 1 2012-11-28 03:17:17 PST

Created attachment 176441 [details] Patch

Kenneth Rohde Christiansen

Comment 2 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?

Chris Dumez

Comment 3 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.

Kenneth Rohde Christiansen

Comment 4 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.

WebKit Review Bot

Comment 5 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

Jocelyn Turcotte

Comment 6 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).

Chris Dumez

Comment 7 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.

Chris Dumez

Comment 8 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).

Kenneth Rohde Christiansen

Comment 9 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?

Chris Dumez

Comment 10 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)

Dongseong Hwang

Comment 11 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.

Jocelyn Turcotte

Comment 12 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?

Chris Dumez

Comment 13 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

Dongseong Hwang

Comment 14 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.

Chris Dumez

Comment 15 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.

Dongseong Hwang

Comment 16 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.

Chris Dumez

Comment 17 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).

Chris Dumez

Comment 18 2012-11-28 05:49:56 PST

Created attachment 176469 [details] Patch Fixed a typo in one of the comments.

Dongseong Hwang

Comment 19 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 :)

Chris Dumez

Comment 20 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

Jussi Kukkonen (jku)

Comment 21 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

Chris Dumez

Comment 22 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.

WebKit Review Bot

Comment 23 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>

WebKit Review Bot

Comment 24 2012-11-28 06:52:11 PST

All reviewed patches have been landed. Closing bug.

Chris Dumez

Comment 25 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.

Chris Dumez

Comment 26 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Bug 103498