atomicDecrement() never reach 0 on Android so no deref() will be called
Created attachment 155265 [details] Patch
Created attachment 155268 [details] Patch
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.
Created attachment 155291 [details] Patch
(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 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/.
/me will fix up the ChangeLog for you.
Created attachment 155309 [details] Patch for landing
Comment on attachment 155309 [details] Patch for landing Clearing flags on attachment: 155309 Committed r124115: <http://trac.webkit.org/changeset/124115>
All reviewed patches have been landed. Closing bug.
Thanks for taking care of this. Our tests are passing now.