Opened 2 years ago

Closed 2 years ago

#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):

https://codereview.adblockplus.org/29671561/

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

  1. Start navigate
  2. Close network connection (let's say disconnect Wi-Fi)
  3. 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)

WechatIMG172.png (16.5 KB) - added by asmirnov 2 years ago.
g2jOckli.png (202.9 KB) - added by asmirnov 2 years ago.
WechatIMG167.png (121.7 KB) - added by asmirnov 2 years ago.

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by asmirnov

comment:1 Changed 2 years ago by asmirnov

  • Description modified (diff)

comment:2 Changed 2 years ago by asmirnov

  • Cc vickyyu added

Changed 2 years ago by asmirnov

comment:3 Changed 2 years ago by asmirnov

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

Version 0, edited 2 years ago by asmirnov (next)

Changed 2 years ago by asmirnov

comment:4 Changed 2 years ago 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 2 years ago by asmirnov

BTW it was extremely rare case (thread both finishing and cancelling exactly at the same time).

comment:6 Changed 2 years ago by asmirnov

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 Changed 2 years ago by asmirnov

  • Description modified (diff)

comment:9 Changed 2 years ago by asmirnov

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.