Bug 91746

Summary: set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android
Product: WebKit Reporter: Wei James (wistoch) <james.wei>
Component: WebCore Misc.Assignee: Wei James (wistoch) <james.wei>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Bug Depends on: 92635    
Bug Blocks: 89428    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Wei James (wistoch) 2012-07-19 08:35:04 PDT
set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android
Comment 1 Wei James (wistoch) 2012-07-19 08:35:24 PDT
Created attachment 153271 [details]
Patch
Comment 2 Adam Barth 2012-07-25 23:22:17 PDT
Comment on attachment 153271 [details]
Patch

Why?
Comment 3 Wei James (wistoch) 2012-07-25 23:35:15 PDT
(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. :)
Comment 4 Adam Barth 2012-07-26 00:02:02 PDT
Would you be willing to include this information in the ChangeLog?  As written the patch is very mysterious.
Comment 5 Adam Barth 2012-07-26 00:03:01 PDT
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?
Comment 6 Wei James (wistoch) 2012-07-26 00:25:07 PDT
(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
Comment 7 Wei James (wistoch) 2012-07-26 00:31:24 PDT
(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.
Comment 8 Wei James (wistoch) 2012-07-26 00:33:04 PDT
(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.
Comment 9 Wei James (wistoch) 2012-07-26 00:55:45 PDT
Created attachment 154566 [details]
Patch
Comment 10 Wei James (wistoch) 2012-07-26 00:58:12 PDT
Created attachment 154567 [details]
Patch
Comment 11 Wei James (wistoch) 2012-07-26 01:02:14 PDT
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 12 Adam Barth 2012-07-26 01:54:33 PDT
Comment on attachment 154567 [details]
Patch

Thanks!  This patch makes much more sense.
Comment 13 Peter Beverloo 2012-07-26 01:55:55 PDT
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!
Comment 14 Wei James (wistoch) 2012-07-26 01:57:30 PDT
(In reply to comment #12)
> (From update of attachment 154567 [details])
> Thanks!  This patch makes much more sense.

thanks!
Comment 15 Wei James (wistoch) 2012-07-26 01:57:47 PDT
(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!
Comment 16 Peter Beverloo 2012-07-27 06:14:42 PDT
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 17 WebKit Review Bot 2012-07-27 07:15:07 PDT
Comment on attachment 154567 [details]
Patch

Clearing flags on attachment: 154567

Committed r123875: <http://trac.webkit.org/changeset/123875>
Comment 18 WebKit Review Bot 2012-07-27 07:15:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexandre Elias 2012-07-30 01:03:48 PDT
This is causing FunctionalTest.MemberFunctionBindRefDeref in the TestWebKitAPI test package to fail on our private bot.
Comment 20 Alexandre Elias 2012-07-30 01:08:46 PDT
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?
Comment 21 Alexandre Elias 2012-07-30 01:18:13 PDT
In webkit_unit_tests package, IDBDatabaseBackendTest.BackingStoreRetention is also failing with backingStore->hasOneRef() (Actual: false, Expected: true).
Comment 22 Wei James (wistoch) 2012-07-30 01:30:18 PDT
(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.
Comment 23 Peter Beverloo 2012-07-30 02:11:52 PDT
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..
Comment 24 Wei James (wistoch) 2012-07-30 02:53:54 PDT
(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.
Comment 25 Wei James (wistoch) 2012-07-30 04:11:08 PDT
(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
Comment 26 Peter Beverloo 2012-07-30 16:54:20 PDT
Marking this as fixed again.