101341 2012-11-06 05:52:24 -0800 [CoordinatedGraphics] Access to LayerTreeRenderer::m_renderQueue should be thread safe 2012-11-26 11:46:22 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 kbalazs kbalazs levin+threading luiz noam rafael.lobo webkit.review.bot zeno oldest_to_newest 759572 0 kbalazs 2012-11-06 05:52:24 -0800 If I'm not wrong, access to the queue can be a race condition across syncRemoteContent and some methods of LayerTreeCoordinatorProxy that update it from the main thread, like setContentsSize or setVisibleContentsRect. Those are used by public API so we should not take any preassumption about when will they be called. 759577 1 172562 kbalazs 2012-11-06 05:56:37 -0800 Created attachment 172562 Patch 759623 2 172562 kbalazs 2012-11-06 06:40:35 -0800 Comment on attachment 172562 Patch Clearing flags on attachment: 172562 Committed r133599: <http://trac.webkit.org/changeset/133599> 759624 3 kbalazs 2012-11-06 06:40:39 -0800 All reviewed patches have been landed. Closing bug. 775406 4 172562 rafael.lobo 2012-11-26 06:03:02 -0800 Comment on attachment 172562 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172562&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:476 > m_renderQueue.clear(); This call here to clear the m_renderQueue should be removed. When we do the swap between a new vector and the m_renderQueue, the resulting queue on m_renderQueue is an empty one. And still you will do that when you have the lock, rather than releasing the lock and doing it afterwards. Should I upload a patch to fix this? 775635 5 kbalazs 2012-11-26 10:39:34 -0800 (In reply to comment #4) > (From update of attachment 172562 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172562&action=review > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:476 > > m_renderQueue.clear(); > > This call here to clear the m_renderQueue should be removed. When we do the swap between a new vector and the m_renderQueue, the resulting queue on m_renderQueue is an empty one. And still you will do that when you have the lock, rather than releasing the lock and doing it afterwards. Should I upload a patch to fix this? Just removing the clear is enough. There is no need to hold the lock after filling the local vector. It is not a problem if we add new tasks in the main thread, those will be flushed later. Fell free to update the patch. 775682 6 rafael.lobo 2012-11-26 11:07:16 -0800 Reopening to attach new patch. 775683 7 176037 rafael.lobo 2012-11-26 11:07:20 -0800 Created attachment 176037 Patch I've decided to keep this follow up patch here in the same bug to give a better context. I'm just removing the unnecessary clear of m_renderQueue with the same reasoning of this bug title. 775729 8 176037 webkit.review.bot 2012-11-26 11:46:18 -0800 Comment on attachment 176037 Patch Clearing flags on attachment: 176037 Committed r135748: <http://trac.webkit.org/changeset/135748> 775730 9 webkit.review.bot 2012-11-26 11:46:22 -0800 All reviewed patches have been landed. Closing bug. 172562 2012-11-06 05:56:37 -0800 2012-11-26 06:03:02 -0800 Patch bug-101341-20121106145441.patch text/plain 3080 kbalazs U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzNTc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggOWY2ZDA1NjNhMzM5NTUz MzkwMzgxMzU1NmRmNjU4NDJlYmViNmM3NC4uZjhkYzBmN2NjMDYyMGIyYWFlNjVmMjEyODFiNjZh YzJmNWFhYjRkOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIxIEBACiAyMDEyLTExLTA2ICBCYWxh enMgS2VsZW1lbiAgPGtiYWxhenNAd2Via2l0Lm9yZz4KIAorICAgICAgICBbQ29vcmRpbmF0ZWRH cmFwaGljc10gQWNjZXNzIHRvIExheWVyVHJlZVJlbmRlcmVyOjptX3JlbmRlclF1ZXVlIHNob3Vs ZCBiZSB0aHJlYWQgc2FmZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTAxMzQxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgVGhlIHF1ZXVlIGNhbiBiZSBhY2Nlc3NlZCBmcm9tIHB1YmxpYyBBUEkgc28gd2Ug c2hvdWxkIG1ha2UKKyAgICAgICAgc3VyZSBpdCBpcyBub3QgdXBkYXRlZCBjb25jdXJyZW50bHkg d2l0aCBhIHRocmVhZGVkIHJlbmRlciBsb29wLgorCisgICAgICAgICogVUlQcm9jZXNzL0Nvb3Jk aW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVlUmVuZGVyZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpM YXllclRyZWVSZW5kZXJlcjo6c3luY1JlbW90ZUNvbnRlbnQpOgorICAgICAgICAoV2ViS2l0OjpM YXllclRyZWVSZW5kZXJlcjo6YXBwZW5kVXBkYXRlKToKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29v cmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVSZW5kZXJlci5oOgorICAgICAgICAoTGF5ZXJUcmVl UmVuZGVyZXIpOgorCisyMDEyLTExLTA2ICBCYWxhenMgS2VsZW1lbiAgPGtiYWxhenNAd2Via2l0 Lm9yZz4KKwogICAgICAgICBbQ29vcmRpbmF0ZWRHcmFwaGljc10gY29tcG9zaXRpbmcvaWZyYW1l cy9jb25uZWN0LWNvbXBvc2l0aW5nLWlmcmFtZS5odG1sIGNyYXNoZXMKICAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMTIzMgogCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVSZW5k ZXJlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQ29vcmRpbmF0ZWRHcmFwaGljcy9M YXllclRyZWVSZW5kZXJlci5jcHAKaW5kZXggMWNlMjJiODMxZWJhYWM0ZTI3ZGIxMzAwYWM5NjFk ZGE2OGJjOWQxMC4uYmQ5MmY3MjQ5MzE0ZTdkMmFhNjllZGRkMzY5Njk4Yjk3MjdlOTQzMyAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5 ZXJUcmVlUmVuZGVyZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9Db29yZGlu YXRlZEdyYXBoaWNzL0xheWVyVHJlZVJlbmRlcmVyLmNwcApAQCAtNDYyLDggKzQ2MiwxNiBAQCB2 b2lkIExheWVyVHJlZVJlbmRlcmVyOjpzeW5jUmVtb3RlQ29udGVudCgpCiAgICAgLy8gV2UgZW5x dWV1ZSBtZXNzYWdlcyBhbmQgZXhlY3V0ZSB0aGVtIGR1cmluZyBwYWludCwgYXMgdGhleSByZXF1 aXJlIGFuIGFjdGl2ZSBHTCBjb250ZXh0LgogICAgIGVuc3VyZVJvb3RMYXllcigpOwogCi0gICAg Zm9yIChzaXplX3QgaSA9IDA7IGkgPCBtX3JlbmRlclF1ZXVlLnNpemUoKTsgKytpKQotICAgICAg ICBtX3JlbmRlclF1ZXVlW2ldKCk7CisgICAgVmVjdG9yPEZ1bmN0aW9uPHZvaWQoKT4gPiByZW5k ZXJRdWV1ZTsKKyAgICBib29sIGNhbGxlZE9uTWFpblRocmVhZCA9IFdURjo6aXNNYWluVGhyZWFk KCk7CisgICAgaWYgKCFjYWxsZWRPbk1haW5UaHJlYWQpCisgICAgICAgIG1fcmVuZGVyUXVldWVN dXRleC5sb2NrKCk7CisgICAgcmVuZGVyUXVldWUuc3dhcChtX3JlbmRlclF1ZXVlKTsKKyAgICBp ZiAoIWNhbGxlZE9uTWFpblRocmVhZCkKKyAgICAgICAgbV9yZW5kZXJRdWV1ZU11dGV4LnVubG9j aygpOworCisgICAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCByZW5kZXJRdWV1ZS5zaXplKCk7ICsr aSkKKyAgICAgICAgcmVuZGVyUXVldWVbaV0oKTsKIAogICAgIG1fcmVuZGVyUXVldWUuY2xlYXIo KTsKIH0KQEAgLTUyMiw2ICs1MzAsOCBAQCB2b2lkIExheWVyVHJlZVJlbmRlcmVyOjphcHBlbmRV cGRhdGUoY29uc3QgRnVuY3Rpb248dm9pZCgpPiYgZnVuY3Rpb24pCiAgICAgaWYgKCFtX2lzQWN0 aXZlKQogICAgICAgICByZXR1cm47CiAKKyAgICBBU1NFUlQoaXNNYWluVGhyZWFkKCkpOworICAg IE11dGV4TG9ja2VyIGxvY2tlcihtX3JlbmRlclF1ZXVlTXV0ZXgpOwogICAgIG1fcmVuZGVyUXVl dWUuYXBwZW5kKGZ1bmN0aW9uKTsKIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQ cm9jZXNzL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVlUmVuZGVyZXIuaCBiL1NvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9Db29yZGluYXRlZEdyYXBoaWNzL0xheWVyVHJlZVJlbmRlcmVyLmgK aW5kZXggODU5NjAyMjk2MjQxZTAxNGM2YTcyZjg2NzI2OWE2ZGI0ZTE2Y2NlYS4uZWJlNGNhMzkx NTFjNzIzMDU1NDRiNDBlNmQzY2I3ODkxMmMxNDEwYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktp dDIvVUlQcm9jZXNzL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVlUmVuZGVyZXIuaAorKysg Yi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVS ZW5kZXJlci5oCkBAIC0xMjYsNiArMTI2LDcgQEAgcHJpdmF0ZToKIAogICAgIC8vIFJlbmRlciBx dWV1ZSBjYW4gYmUgYWNjZXNzZWQgb255IGZyb20gbWFpbiB0aHJlYWQgb3IgdXBkYXRlUGFpbnRO b2RlIGNhbGwgc3RhY2shCiAgICAgVmVjdG9yPEZ1bmN0aW9uPHZvaWQoKT4gPiBtX3JlbmRlclF1 ZXVlOworICAgIE11dGV4IG1fcmVuZGVyUXVldWVNdXRleDsKIAogI2lmIFVTRShURVhUVVJFX01B UFBFUikKICAgICBPd25QdHI8V2ViQ29yZTo6VGV4dHVyZU1hcHBlcj4gbV90ZXh0dXJlTWFwcGVy Owo= 176037 2012-11-26 11:07:20 -0800 2012-11-26 11:46:18 -0800 Patch bug-101341-20121126160504.patch text/plain 1565 rafael.lobo U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NzM1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggODliOGIxMDYzZmEwMGFj MjIwZGMyMzgxOGE0MjFkOTg5OTAwZGZiZi4uZTA1MTBjNzIxYjVkZTY4MDc4YmEyNzg3ODI0Zjcx NzZjZDU1NzdlMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTExLTI2ICBSYWZh ZWwgQnJhbmRhbyAgPHJhZmFlbC5sb2JvQG9wZW5ib3NzYS5vcmc+CisKKyAgICAgICAgW0Nvb3Jk aW5hdGVkR3JhcGhpY3NdIEFjY2VzcyB0byBMYXllclRyZWVSZW5kZXJlcjo6bV9yZW5kZXJRdWV1 ZSBzaG91bGQgYmUgdGhyZWFkIHNhZmUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEwMTM0MQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogVUlQcm9jZXNzL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVl UmVuZGVyZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpMYXllclRyZWVSZW5kZXJlcjo6c3luY1Jl bW90ZUNvbnRlbnQpOiBUaGUgcHJldmlvdXMgc3dhcCBhbHJlYWR5IGNsZWFycyB0aGUKKyAgICAg ICAgdmVjdG9yIG9uIG1fcmVuZGVyUXVldWUuIEl0IGlzIGFsc28gZG9uZSBpbiBhIHRocmVhZC1z YWZlIHdheSwgc28gY2xlYXJpbmcgaXQgYWZ0ZXJ3YXJkCisgICAgICAgIGNhbiBiZSBoYXJtZnVs IGFzIHdlJ3ZlIGFscmVhZHkgcmVsZWFzZWQgdGhlIGxvY2suCisKIDIwMTItMTEtMjYgIEphZWh1 biBMaW0gIDxsamFlaHVuLmxpbUBzYW1zdW5nLmNvbT4KIAogICAgICAgICBUZXh0IEF1dG9zaXpp bmc6IEFkZCBUZXh0IEF1dG9zaXppbmcgQVBJcyBmb3IgV0syCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViS2l0Mi9VSVByb2Nlc3MvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVSZW5kZXJlci5j cHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRy ZWVSZW5kZXJlci5jcHAKaW5kZXggMGMwMDllYzQ4M2FkZGIzYTZmZjVhZDU5OWQ4ZmFlZGIzNDcy MGRkZS4uMGRjODhkMDhmYWNlN2I0NjdiMDk5ZTIwZWU5YzUyN2VhMjY1OTVmNCAxMDA2NDQKLS0t IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVl UmVuZGVyZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9Db29yZGluYXRlZEdy YXBoaWNzL0xheWVyVHJlZVJlbmRlcmVyLmNwcApAQCAtNTg2LDggKzU4Niw2IEBAIHZvaWQgTGF5 ZXJUcmVlUmVuZGVyZXI6OnN5bmNSZW1vdGVDb250ZW50KCkKIAogICAgIGZvciAoc2l6ZV90IGkg PSAwOyBpIDwgcmVuZGVyUXVldWUuc2l6ZSgpOyArK2kpCiAgICAgICAgIHJlbmRlclF1ZXVlW2ld KCk7Ci0KLSAgICBtX3JlbmRlclF1ZXVlLmNsZWFyKCk7CiB9CiAKIHZvaWQgTGF5ZXJUcmVlUmVu ZGVyZXI6OnB1cmdlR0xSZXNvdXJjZXMoKQo=