Opened 2 years ago

Closed 20 months ago

#5118 closed change (fixed)

Eliminate possible access violation from worker threads while destroying of Platform

Reported by: sergz Assignee: hfiguiere
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, hfiguiere Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29543810

Description (last modified by sergz)

Background

If asynchronous operations like sending of web request or timers are executed in background threads then they can cause access violation while destroying of Platform. E.g. between destroying of Platform::webRequest and destroying of Platform::fileSystem some thread from FileSystem implementation can attempt to access already destroyed Platform::webRequest.

What to change

For present I propose to create one mutex which should be used each time we access an asynchronous interface and before accessing it we should check that the pointer is still not nullptr. In ~Platform we should synchronously (using that mutex) move smart pointers to asynchronous interfaces to local variables and let them destroy after releasing of that mutex. This trick in destructor is required to avoid the following dead-lock: let's say we destroy these implementations while the mutex is being acquired, then some async operation which is scheduling another asynchronous operation will be blocked by that mutex but implementation of async interface should wait for finishing of all background threads in the destructor. Thus we block the mutex and wait until finishing of a background thread which is waiting for the mutex.

Change History (7)

comment:1 follow-up: Changed 2 years ago by sergz

Basically, it's required only for present. After finishing with #4692 I would expect a payload of a callback passed to async operation to be executed only after acquiring of a strong reference to JsEngine what is already impossible when JsEngine is being destroyed.

comment:2 Changed 2 years ago by hfiguiere

  • Cc hfiguiere added

comment:3 in reply to: ↑ 1 Changed 21 months ago by sergz

  • Description modified (diff)
  • Summary changed from Eliminate possible access violation from worker threads while destroying of JsEngine to Eliminate possible access violation from worker threads while destroying of Platform

Replying to sergz:

Basically, it's required only for present. After finishing with #4692 I would expect a payload of a callback passed to async operation to be executed only after acquiring of a strong reference to JsEngine what is already impossible when JsEngine is being destroyed.

This comment seems incorrect because it's not JsEngine what is managing worker threads (#5198).

The code has also changed, so the description is accordingly updated.

In order to protect access to interface I propose to replace getters by functions acquiring the mutex and calling a callback if the interface is not null, e.g.

void Platform::WithWebRequest(const WithWebRequest& callback) {
  std::lock_guard<std::recusive_mutex> lock(interfacesMutex);
  if (webRequest && callback)
    callback(*webRequest);
}

and usage:

paltform->WithWebRequest([...](IWebRequest& webRequest){
  webRequest->Get(url, headers, [](...){
    ...
  });
});

comment:4 Changed 20 months ago by hfiguiere

  • Owner set to hfiguiere

comment:5 Changed 20 months ago by hfiguiere

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

comment:6 Changed 20 months ago by abpbot

A commit referencing this issue has landed:
Issue 5118 - Lock the platform interfaces before use

comment:7 Changed 20 months ago by hfiguiere

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