Bug 92635 - atomicDecrement() never reach 0 on Android so no deref() will be called
Summary: atomicDecrement() never reach 0 on Android so no deref() will be called
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wei James (wistoch)
URL:
Keywords:
Depends on:
Blocks: 91746
  Show dependency treegraph
 
Reported: 2012-07-30 05:05 PDT by Wei James (wistoch)
Modified: 2012-07-30 17:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2012-07-30 05:08 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2012-07-30 05:14 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2012-07-30 08:01 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch for landing (1.68 KB, patch)
2012-07-30 10:02 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wei James (wistoch) 2012-07-30 05:05:05 PDT
atomicDecrement() never reach 0 on Android so no deref() will be called
Comment 1 Wei James (wistoch) 2012-07-30 05:08:37 PDT
Created attachment 155265 [details]
Patch
Comment 2 Wei James (wistoch) 2012-07-30 05:14:57 PDT
Created attachment 155268 [details]
Patch
Comment 3 Peter Beverloo 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.
Comment 4 Wei James (wistoch) 2012-07-30 08:01:17 PDT
Created attachment 155291 [details]
Patch
Comment 5 Wei James (wistoch) 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
Comment 6 Peter Beverloo 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/.
Comment 7 Adam Barth 2012-07-30 09:32:40 PDT
/me will fix up the ChangeLog for you.
Comment 8 Adam Barth 2012-07-30 10:02:28 PDT
Created attachment 155309 [details]
Patch for landing
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2012-07-30 16:53:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexandre Elias 2012-07-30 17:05:18 PDT
Thanks for taking care of this.  Our tests are passing now.