atomicDecrement() never reach 0 on Android so no deref() will be called

• Component: New Bugs
• Status: RESOLVED FIXED

[Wei James (wistoch)]

atomicDecrement() never reach 0 on Android so no deref() will be called

[Wei James (wistoch)]

Created attachment 155265 [details]
Patch

[Wei James (wistoch)]

Created attachment 155268 [details]
Patch

[Peter Beverloo]

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)]

Created attachment 155291 [details]
Patch

[Wei James (wistoch)]

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

/me will fix up the ChangeLog for you.

[Adam Barth]

Created attachment 155309 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155309 [details]
Patch for landing

Clearing flags on attachment: 155309

Committed r124115: <http://trac.webkit.org/changeset/124115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandre Elias]

Thanks for taking care of this.  Our tests are passing now.