Opened on 01/16/2018 at 10:37:41 AM
Closed on 01/17/2018 at 12:35:23 PM
#6274 closed defect (fixed)
ANR on cancelling elemhide thread for load error
| Reported by: | asmirnov | Assignee: | |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Libadblockplus-Android | Keywords: | |
| Cc: | vickyyu | Blocked By: | |
| Blocking: | Platform: | Android | |
| Ready: | no | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): | |||
Description (last modified by asmirnov)
Environment
If load error happens (onReceivedError) when elemhide thread is running, the thread is locked while trying to cancel elemhide thread.
How to reproduce
- Start navigate
- Close network connection (let's say disconnect Wi-Fi)
- See thread is locked in finish
Probably it's locked in https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java#L1045
Observed behaviour
Thread is locked
Expected behaviour
Thread is locked but then unlocked (when elemhide thread finishes).
Attachments (3)
Change History (12)
Changed on 01/16/2018 at 10:38:13 AM by asmirnov
comment:2 Changed on 01/16/2018 at 10:51:29 AM by asmirnov
- Cc vickyyu added
Changed on 01/16/2018 at 01:23:24 PM by asmirnov
comment:3 Changed on 01/16/2018 at 01:29:26 PM by asmirnov
Changed on 01/17/2018 at 06:46:39 AM by asmirnov
comment:4 Changed on 01/17/2018 at 07:01:12 AM by asmirnov
This is dealock happened:
elemhide thread: finish() -> in onFinished() enter [finishedRunnableLockObject] -> in elemHideThreadFinishedRunnable.run() enter [elemHideThreadLockObject] [LOCKED] main thread: stopAbpLoading() -> enter [elemHideThreadLockObject] -> in elemhideThread.cancel() -> finish() -> in onFinished() enter [finishedRunnableLockObject] [LOCKED]
So main thread locked elemHideThreadLockObject and it's locking bg thread. Meanwhile bg thread locked finishedRunnableLockObject and it's locking main thread.
The logical reason is that finishing (but not yet finished so thread ref is not null as it's set to null at the very end of finishing) thread is being cancelled.
There are multiple ways of fighting with it (see "Java concurrent in Practice") and i believe the right one is avoid cancelling thread that is finishing. So we should mark it's finishing and skip cancelling.
comment:5 Changed on 01/17/2018 at 07:02:23 AM by asmirnov
BTW it was extremely rare case (thread both finishing and cancelling exactly at the same time).
comment:6 Changed on 01/17/2018 at 07:13:18 AM by asmirnov
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:8 Changed on 01/17/2018 at 12:34:52 PM by abpbot
Some commits referencing this issue have landed:
comment:9 Changed on 01/17/2018 at 12:35:23 PM by asmirnov
- Resolution set to fixed
- Status changed from reviewing to closed
According to attachment №2 locked object is java.lang.Object. In onFinished() it can be only finishedRunnableLockObject so it's locked in https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java#L1045. It means that https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java#L1049 locks current thread.