Opened 3 years ago

Closed 2 years ago

#4692 closed change (fixed)

Don't keep strong references to JsEngine in worker threads

Reported by: sergz Assignee: eric@…
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, oleksandr, eric@… Blocked By:
Blocking: #3595 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29369365/
https://codereview.adblockplus.org/29367507/
https://codereview.adblockplus.org/29369520/
https://codereview.adblockplus.org/29369557/
https://codereview.adblockplus.org/29370568/

Description

Background

Having JsEngine forever alive already hurts. If we stay with working threads for async operations (what is pretty likely for a near future) we will need to do it anyway since the lifetime of the threads is scoped by the lifetime of corresponding instance of JsEngine, otherwise we will get a circular reference, JsEngine<>-Threads<>-JsEngine.

What to change

Use

  • v8::Persistent instead of std::shared_ptr<JsValue>
  • std::weak_ptr<JsEngine> instead of std::shared_ptr<JsEngine>

as types of members of worker threads.

Change History (6)

comment:1 Changed 3 years ago by eric@…

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

Review https://codereview.adblockplus.org/29369365/ get the number of occurrences of shared_ptr<JsEngine> down to a single one that can't be removed until scheduling improves.

There was no need for weak_ptr; plain pointers were sufficient after careful examination of life cycle issues.

comment:2 Changed 3 years ago by eric@…

  • Owner set to eric@…
  • Review URL(s) modified (diff)

comment:3 Changed 2 years ago by sergz

  • Review URL(s) modified (diff)

comment:4 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4692 - Dont' keep a strong reference from timer thread

comment:5 Changed 2 years ago by sergz

  • Blocking 3593 removed

comment:6 Changed 2 years ago by sergz

  • Resolution set to fixed
  • Status changed from reviewing to closed

Currently the tasks for asynchronous operations don't keep permanently a strong reference to JsEngine.

Note: See TracTickets for help on using tickets.