91746 2012-07-19 08:35:04 -0700 set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android 2012-07-30 16:54:20 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 92635 89428 1 james.wei james.wei abarth aelias peter webkit.review.bot oldest_to_newest 673534 0 james.wei 2012-07-19 08:35:04 -0700 set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android 673535 1 153271 james.wei 2012-07-19 08:35:24 -0700 Created attachment 153271 Patch 678897 2 153271 abarth 2012-07-25 23:22:17 -0700 Comment on attachment 153271 Patch Why? 678915 3 james.wei 2012-07-25 23:35:15 -0700 (In reply to comment #2) > (From update of attachment 153271 [details]) > Why? it is part of the efforts to enable web audio for chromium android. as mentioned in https://bugs.webkit.org/show_bug.cgi?id=89428#c19 peter wants to seperate the patch to two pieces. the one in #89428 already landed webkit. but I cannot find digit@google.com on bugs.webkit.org so have to turn to you. :) 678933 4 abarth 2012-07-26 00:02:02 -0700 Would you be willing to include this information in the ChangeLog? As written the patch is very mysterious. 678934 5 abarth 2012-07-26 00:03:01 -0700 From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio. Are we sure we want those implications? Do we turn this on for Chromium on other operating systems? 678950 6 james.wei 2012-07-26 00:25:07 -0700 (In reply to comment #4) > Would you be willing to include this information in the ChangeLog? As written the patch is very mysterious. sure. I will update the patch to include this information. thanks 678953 7 james.wei 2012-07-26 00:31:24 -0700 (In reply to comment #5) > From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio. Are we sure we want those implications? Do we turn this on for Chromium on other operating systems? I was confused too when I saw this MACRO at the begining. this MACRO is to eanble using WTF::atomicDecrement and WTF::atomicIncreament in wtf/Atomics.h from wtf/Atomics.h, seems all other OS and platforms enabled this MACRO. 678955 8 james.wei 2012-07-26 00:33:04 -0700 (In reply to comment #5) > From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio. Are we sure we want those implications? Do we turn this on for Chromium on other operating systems? it is not just for web audio. it is for wtf::atomicDecrement and wtf::atomicIncrement. Web Audio just needs to use atomicDecrement and atomicIncrement. 678973 9 154566 james.wei 2012-07-26 00:55:45 -0700 Created attachment 154566 Patch 678974 10 154567 james.wei 2012-07-26 00:58:12 -0700 Created attachment 154567 Patch 678977 11 james.wei 2012-07-26 01:02:14 -0700 after reviewing the comments in #89428, it may make more sense to put this MACRO in Atomics.h. also, added more information in ChangeLog. thanks 679047 12 154567 abarth 2012-07-26 01:54:33 -0700 Comment on attachment 154567 Patch Thanks! This patch makes much more sense. 679053 13 peter 2012-07-26 01:55:55 -0700 I've asked David Turner to have a quick peek as well, as I vaguely recall issues with this. I can set CQ+ once we're good. Thank you, James Wei! 679056 14 james.wei 2012-07-26 01:57:30 -0700 (In reply to comment #12) > (From update of attachment 154567 [details]) > Thanks! This patch makes much more sense. thanks! 679057 15 james.wei 2012-07-26 01:57:47 -0700 (In reply to comment #13) > I've asked David Turner to have a quick peek as well, as I vaguely recall issues with this. I can set CQ+ once we're good. Thank you, James Wei! that's fine. thanks! 680523 16 peter 2012-07-27 06:14:42 -0700 The issues were fixed starting the release of r7b of the Android NDK, which is what WebKit uses. Setting cq+, thank you for the patch! 680600 17 154567 webkit.review.bot 2012-07-27 07:15:07 -0700 Comment on attachment 154567 Patch Clearing flags on attachment: 154567 Committed r123875: <http://trac.webkit.org/changeset/123875> 680602 18 webkit.review.bot 2012-07-27 07:15:11 -0700 All reviewed patches have been landed. Closing bug. 681756 19 aelias 2012-07-30 01:03:48 -0700 This is causing FunctionalTest.MemberFunctionBindRefDeref in the TestWebKitAPI test package to fail on our private bot. 681758 20 aelias 2012-07-30 01:08:46 -0700 Not sure of the reason why it has issues on Android, given that the setting is enabled on many other platforms without problems. Wei, could you investigate? 681761 21 aelias 2012-07-30 01:18:13 -0700 In webkit_unit_tests package, IDBDatabaseBackendTest.BackingStoreRetention is also failing with backingStore->hasOneRef() (Actual: false, Expected: true). 681771 22 james.wei 2012-07-30 01:30:18 -0700 (In reply to comment #20) > Not sure of the reason why it has issues on Android, given that the setting is enabled on many other platforms without problems. Wei, could you investigate? ok. I will investigate it. 681792 23 peter 2012-07-30 02:11:52 -0700 I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves. Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b.. 681815 24 james.wei 2012-07-30 02:53:54 -0700 (In reply to comment #23) > I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves. > > Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b.. peter, I can reproduce this issue in chromium upstream with ndk r8. it is strange as __atomic_inc is implemented with __sync_fetch_and_add in r8. 681846 25 james.wei 2012-07-30 04:11:08 -0700 (In reply to comment #24) > (In reply to comment #23) > > I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves. > > > > Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b.. > > peter, I can reproduce this issue in chromium upstream with ndk r8. > > it is strange as __atomic_inc is implemented with __sync_fetch_and_add in r8. I have found the root cause of this issue. __atomic_inc is safe to use in android. but in WebKit, it is assumed that __atomic_inc will return the new value. while in Android, __sync_fetch_and_add will return the old value. so ThreadSafeRefCounted.h, Line 108: should be atomicDecrement(&m_refCount) <= 1 in android instead of atomicDecrement(&m_refCount) <= 0 I will refine and submit the patch later for review. thanks 682536 26 peter 2012-07-30 16:54:20 -0700 Marking this as fixed again. 153271 2012-07-19 08:35:24 -0700 2012-07-26 00:55:40 -0700 Patch bug-91746-20120719233536.patch text/plain 1016 james.wei U3VidmVyc2lvbiBSZXZpc2lvbjogMTIzMDM1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDNkOTY2NDZjMWNlMjgxN2ViY2ZlZTBk NTkyYjQ5MDQ2MDU1NzNmNjUuLjM5NzNhZmI5NmZlOThlYjExNTk1ZWY0YTAwYTY2Yjk0ZDg0ZDBm Y2YgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTItMDctMTkgIFdlaSBKYW1lcyAgPGphbWVzLndl aUBpbnRlbC5jb20+CisKKyAgICAgICAgc2V0IFdURl9VU0VfTE9DS0ZSRUVfVEhSRUFEU0FGRVJF RkNPVU5URUQgZm9yIGNocm9taXVtIGFuZHJvaWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTkxNzQ2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgKiB3dGYvUGxhdGZvcm0uaDoKKwogMjAxMi0wNy0xOCAgWW9u ZyBMaSAgPHlvbGlAcmltLmNvbT4KIAogICAgICAgICBbQmxhY2tCZXJyeV0gSW1wbGVtZW50IGN1 cnJlbnRUaW1lKCkgYW5kIG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpIGZvciBPUyhRTlgp CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oIGIvU291cmNlL1dURi93dGYv UGxhdGZvcm0uaAppbmRleCBjMTdmM2UxYmMxZTFlYjdmYzkyZTY2YzVmNGYzZDdiNjQ4YWI1ZWU4 Li44ODgyYTgyODUzMTkyNzMyYmJlMzZhODIyZGIxMGRlOGUzZDgyOWNjIDEwMDY0NAotLS0gYS9T b3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL1BsYXRmb3JtLmgK QEAgLTc1MCw2ICs3NTAsNyBAQAogI2RlZmluZSBIQVZFX1NUUklOR1NfSCAxCiAjZGVmaW5lIEhB VkVfU1lTX1BBUkFNX0ggMQogI2RlZmluZSBIQVZFX1NZU19USU1FX0ggMQorI2RlZmluZSBXVEZf VVNFX0xPQ0tGUkVFX1RIUkVBRFNBRkVSRUZDT1VOVEVEIDEKIAogI2Vsc2UKIAo= 154566 2012-07-26 00:55:45 -0700 2012-07-26 00:58:06 -0700 Patch bug-91746-20120726155547.patch text/plain 1673 james.wei U3VidmVyc2lvbiBSZXZpc2lvbjogMTIzNjkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDhkMTIwNTgxYTlmODM1M2M4ODBlMTdm NjM3MTc2MjNmODQ4ZTViNTMuLmUzMzA3NGYzYTQ0Yzg3ZTM3NGViZmVjYjJjOTFmNDU3ZmE3MWY5 ZTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTItMDctMjYgIFdlaSBKYW1lcyAgPGphbWVzLndl aUBpbnRlbC5jb20+CisKKyAgICAgICAgc2V0IFdURl9VU0VfTE9DS0ZSRUVfVEhSRUFEU0FGRVJF RkNPVU5URUQgZm9yIGNocm9taXVtIGFuZHJvaWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTkxNzQ2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVGhpcyBwYXRjaCBpcyBwYXJ0IG9mIGVmZm9ydHMgdG8gZW5h YmxlIHdlYiBhdWRpbyBmb3IgY2hyb21pdW0gYW5kcm9pZC4KKyAgICAgICAgV2ViIGF1ZGlvIGNv bXBvbmVudCBuZWVkcyB0byB1c2UgYXRvbWljSW5jcmVtZW50IGFuZCBhdG9taWNEZWNyZW1lbnQs CisgICAgICAgIHdoaWNoIGFyZSBlbmFibGVkIGJ5IHRoaXMgTUFDUk8uCisKKyAgICAgICAgQXMg bWVudGlvbmVkIGluCWh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04OTQy OCNjMTksIG1ha2UKKyAgICAgICAgdGhpcyBwYXRjaCBzdGFuZGFsb25lLgorCisgICAgICAgICog d3RmL0F0b21pY3MuaDoKKyAgICAgICAgKFdURik6CisKIDIwMTItMDctMjUgIEJlbmphbWluIFBv dWxhaW4gIDxicG91bGFpbkBhcHBsZS5jb20+CiAKICAgICAgICAgSW5pdGlhbGl6ZSBRdWFsaWZp ZWROYW1lJ3Mgc3RyaW5ncyBmcm9tIHRoZSByZWFkIG9ubHkgZGF0YSBzZWdtZW50CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV1RGL3d0Zi9BdG9taWNzLmggYi9Tb3VyY2UvV1RGL3d0Zi9BdG9taWNzLmgK aW5kZXggOTU4YWJkMmZhZmQ3NjZiMDNkZjA1MzE3YjAxZGQyYzZlOTBlZTZmOC4uYTJlZmQ5NDhj NzNjNmRiZGQzYTI5YzYyM2YxZjU0MDk2NzFhNDZiZCAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93 dGYvQXRvbWljcy5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0F0b21pY3MuaApAQCAtMTA2LDYgKzEw Niw3IEBAIGlubGluZSBpbnQgYXRvbWljSW5jcmVtZW50KGludCB2b2xhdGlsZSogYWRkZW5kKSB7 IHJldHVybiBzdGF0aWNfY2FzdDxpbnQ+KGF0b21pCiBpbmxpbmUgaW50IGF0b21pY0RlY3JlbWVu dChpbnQgdm9sYXRpbGUqIGFkZGVuZCkgeyByZXR1cm4gc3RhdGljX2Nhc3Q8aW50PihhdG9taWNf c3ViX3ZhbHVlKHJlaW50ZXJwcmV0X2Nhc3Q8dW5zaWduZWQgdm9sYXRpbGUqPihhZGRlbmQpLCAx KSkgLSAxOyB9CiAKICNlbGlmIE9TKEFORFJPSUQpCisjZGVmaW5lIFdURl9VU0VfTE9DS0ZSRUVf VEhSRUFEU0FGRVJFRkNPVU5URUQgMQogCiBpbmxpbmUgaW50IGF0b21pY0luY3JlbWVudChpbnQg dm9sYXRpbGUqIGFkZGVuZCkgeyByZXR1cm4gX19hdG9taWNfaW5jKGFkZGVuZCk7IH0KIGlubGlu ZSBpbnQgYXRvbWljRGVjcmVtZW50KGludCB2b2xhdGlsZSogYWRkZW5kKSB7IHJldHVybiBfX2F0 b21pY19kZWMoYWRkZW5kKTsgfQo= 154567 2012-07-26 00:58:12 -0700 2012-07-27 07:15:07 -0700 Patch bug-91746-20120726155815.patch text/plain 1673 james.wei U3VidmVyc2lvbiBSZXZpc2lvbjogMTIzNjkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDhkMTIwNTgxYTlmODM1M2M4ODBlMTdm NjM3MTc2MjNmODQ4ZTViNTMuLmRiODBhNTBhM2Y5NmRhYjdiNDIxMmJmYWE0MTBmZWUwOTczNTc4 N2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTItMDctMjYgIFdlaSBKYW1lcyAgPGphbWVzLndl aUBpbnRlbC5jb20+CisKKyAgICAgICAgc2V0IFdURl9VU0VfTE9DS0ZSRUVfVEhSRUFEU0FGRVJF RkNPVU5URUQgZm9yIGNocm9taXVtIGFuZHJvaWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTkxNzQ2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVGhpcyBwYXRjaCBpcyBwYXJ0IG9mIGVmZm9ydHMgdG8gZW5h YmxlIHdlYiBhdWRpbyBmb3IgY2hyb21pdW0gYW5kcm9pZC4KKyAgICAgICAgV2ViIGF1ZGlvIGNv bXBvbmVudCBuZWVkcyB0byB1c2UgYXRvbWljSW5jcmVtZW50IGFuZCBhdG9taWNEZWNyZW1lbnQs CisgICAgICAgIHdoaWNoIGFyZSBlbmFibGVkIGJ5IHRoaXMgTUFDUk8uCisKKyAgICAgICAgQXMg bWVudGlvbmVkIGluIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04OTQy OCNjMTksIG1ha2UKKyAgICAgICAgdGhpcyBwYXRjaCBzdGFuZGFsb25lLgorCisgICAgICAgICog d3RmL0F0b21pY3MuaDoKKyAgICAgICAgKFdURik6CisKIDIwMTItMDctMjUgIEJlbmphbWluIFBv dWxhaW4gIDxicG91bGFpbkBhcHBsZS5jb20+CiAKICAgICAgICAgSW5pdGlhbGl6ZSBRdWFsaWZp ZWROYW1lJ3Mgc3RyaW5ncyBmcm9tIHRoZSByZWFkIG9ubHkgZGF0YSBzZWdtZW50CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV1RGL3d0Zi9BdG9taWNzLmggYi9Tb3VyY2UvV1RGL3d0Zi9BdG9taWNzLmgK aW5kZXggOTU4YWJkMmZhZmQ3NjZiMDNkZjA1MzE3YjAxZGQyYzZlOTBlZTZmOC4uYTJlZmQ5NDhj NzNjNmRiZGQzYTI5YzYyM2YxZjU0MDk2NzFhNDZiZCAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93 dGYvQXRvbWljcy5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0F0b21pY3MuaApAQCAtMTA2LDYgKzEw Niw3IEBAIGlubGluZSBpbnQgYXRvbWljSW5jcmVtZW50KGludCB2b2xhdGlsZSogYWRkZW5kKSB7 IHJldHVybiBzdGF0aWNfY2FzdDxpbnQ+KGF0b21pCiBpbmxpbmUgaW50IGF0b21pY0RlY3JlbWVu dChpbnQgdm9sYXRpbGUqIGFkZGVuZCkgeyByZXR1cm4gc3RhdGljX2Nhc3Q8aW50PihhdG9taWNf c3ViX3ZhbHVlKHJlaW50ZXJwcmV0X2Nhc3Q8dW5zaWduZWQgdm9sYXRpbGUqPihhZGRlbmQpLCAx KSkgLSAxOyB9CiAKICNlbGlmIE9TKEFORFJPSUQpCisjZGVmaW5lIFdURl9VU0VfTE9DS0ZSRUVf VEhSRUFEU0FGRVJFRkNPVU5URUQgMQogCiBpbmxpbmUgaW50IGF0b21pY0luY3JlbWVudChpbnQg dm9sYXRpbGUqIGFkZGVuZCkgeyByZXR1cm4gX19hdG9taWNfaW5jKGFkZGVuZCk7IH0KIGlubGlu ZSBpbnQgYXRvbWljRGVjcmVtZW50KGludCB2b2xhdGlsZSogYWRkZW5kKSB7IHJldHVybiBfX2F0 b21pY19kZWMoYWRkZW5kKTsgfQo=