Bug 92635

Summary: atomicDecrement() never reach 0 on Android so no deref() will be called
Product: WebKit Reporter: Wei James (wistoch) <james.wei>
Component: New BugsAssignee: Wei James (wistoch) <james.wei>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, levin+threading, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 91746    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Wei James (wistoch)
Reported 2012-07-30 05:05:05 PDT
atomicDecrement() never reach 0 on Android so no deref() will be called
Attachments
Patch (1.72 KB, patch)
2012-07-30 05:08 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Patch (1.72 KB, patch)
2012-07-30 05:14 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Patch (1.81 KB, patch)
2012-07-30 08:01 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Patch for landing (1.68 KB, patch)
2012-07-30 10:02 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Wei James (wistoch)
Comment 1 2012-07-30 05:08:37 PDT
Created attachment 155265 [details] Patch
Wei James (wistoch)
Comment 2 2012-07-30 05:14:57 PDT
Created attachment 155268 [details] Patch
Peter Beverloo
Comment 3 2012-07-30 06:38:29 PDT
Comment on attachment 155268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155268&action=review This probably is something we should modify in the atomicDecrement() implementation in Atomics.h by substracting one from __atomic_dec()'s return value. Otherwise other potential other users of atomicDecrement() have to keep this in mind as well, while it actually is an issue with the implementation of that function. That also means that we'll only touch Chromium on Android specific code, while this solution touches general WTF code. > Source/WTF/wtf/ThreadSafeRefCounted.h:108 > +// atomicDecrement() on Android is implemented with __atomic_inc(), which is __atomic_inc -> __atomic_dec.
Wei James (wistoch)
Comment 4 2012-07-30 08:01:17 PDT
Created attachment 155291 [details] Patch
Wei James (wistoch)
Comment 5 2012-07-30 08:01:56 PDT
(In reply to comment #3) > (From update of attachment 155268 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155268&action=review > > This probably is something we should modify in the atomicDecrement() implementation in Atomics.h by substracting one from __atomic_dec()'s return value. > Otherwise other potential other users of atomicDecrement() have to keep this in mind as well, while it actually is an issue with the implementation of that function. That also means that we'll only touch Chromium on Android specific code, while this solution touches general WTF code. > > > Source/WTF/wtf/ThreadSafeRefCounted.h:108 > > +// atomicDecrement() on Android is implemented with __atomic_inc(), which is > > __atomic_inc -> __atomic_dec. fixed. thanks
Peter Beverloo
Comment 6 2012-07-30 08:56:06 PDT
Comment on attachment 155291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155291&action=review Looks good to me, thank you James! I also verified that this fixes the test failure in TestWebKitAPI. Adam, could you please do a formal review? I've set cq? on the patch too. > Source/WTF/ChangeLog:3 > + atomicDecrement() never reach 0 on Android so no deref() will be called nit: Prefixing bug titles with [Chromium] if all code-changes are Chromium specific generally is good practice. This already mentions "Android", though, so you're good. > Source/WTF/ChangeLog:8 > + With Android NDK 7rb and later, __atomic_dec() is implemented by nit: s/7rb/r7b/.
Adam Barth
Comment 7 2012-07-30 09:32:40 PDT
/me will fix up the ChangeLog for you.
Adam Barth
Comment 8 2012-07-30 10:02:28 PDT
Created attachment 155309 [details] Patch for landing
WebKit Review Bot
Comment 9 2012-07-30 16:53:35 PDT
Comment on attachment 155309 [details] Patch for landing Clearing flags on attachment: 155309 Committed r124115: <http://trac.webkit.org/changeset/124115>
WebKit Review Bot
Comment 10 2012-07-30 16:53:38 PDT
All reviewed patches have been landed. Closing bug.
Alexandre Elias
Comment 11 2012-07-30 17:05:18 PDT
Thanks for taking care of this. Our tests are passing now.
Note You need to log in before you can comment on or make changes to this bug.