Bug 91746

Created attachment 153271 [details] Patch

Comment on attachment 153271 [details] Patch Why?

(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. :)

Would you be willing to include this information in the ChangeLog? As written the patch is very mysterious.

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?

(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

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

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

Created attachment 154566 [details] Patch

Created attachment 154567 [details] Patch

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

Comment on attachment 154567 [details] Patch Thanks! This patch makes much more sense.

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!

(In reply to comment #12) > (From update of attachment 154567 [details]) > Thanks! This patch makes much more sense. thanks!

(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!

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!

Comment on attachment 154567 [details] Patch Clearing flags on attachment: 154567 Committed r123875: <http://trac.webkit.org/changeset/123875>

All reviewed patches have been landed. Closing bug.

This is causing FunctionalTest.MemberFunctionBindRefDeref in the TestWebKitAPI test package to fail on our private bot.

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?

In webkit_unit_tests package, IDBDatabaseBackendTest.BackingStoreRetention is also failing with backingStore->hasOneRef() (Actual: false, Expected: true).

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

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

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

(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

Marking this as fixed again.