Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29361562: Issue 3594 - remove circular references JsEngine-JsValue-JsEngine (Closed)

Created:
Nov. 3, 2016, 9:09 a.m. by sergei
Modified:
Dec. 2, 2016, 8:20 p.m.
Visibility:
Public.

Description

- stop sharing of v8::Isolate among tests (test/BaseJsTest.cpp::CreateJsEngine). - JsValue now holds a weak_ptr<JsEngine> instead of a shared_ptr, so now there is no circular references. ~JsValue is also now guarded by JsContext because we should not try to Dispose underlying value if there is no longer JsEngine. - add JsEngine::JsEngineNotAvailableException which is thrown when JsEngine is not available. - Ignore JsEngine::JsEngineNotAvailableException if it's thrown inside Thread::Run. - IoThread (used in FileSystemJsObject.cpp) and WebRequestJsObject now hold a weak_ptr<JsEngine> instead of shared_ptr. Indirect strong references to JsEngine held by JsValue members are also removed by changes in JsValue. So even if there is a long running IO operation the application is still controlling the lifetime of corresponding instance of JsEngine. Although it's not an issue for current FileSystem, it's vital for tests with LazyWebRequest (FileSystem will likely also have asynchronous interface in a future). LazyWebRequest sleeps for infinity time thus preventing the thread from finishing, so with shared_ptr member the instance of JsEngine is never deleted, weak_ptr allows to free resources. The disadvantage of the solution is that if we must do something else in the callback to stay consistent, there is a possibility that the persistent state will be inconsistent because the callback is not called after finishing of IO operation if there is already no JsEngine. - add member "JsEnginePtr jsEngine;" with corresponding getter to JsContext and change constructor to accept weak_ptr<JsEngine>. We use JsContext as a scoped object defining when JsEngine can be used by obtaining of certain lock and creating scopes for underlying v8 structures. It seems make sense to include JsEngine into that context and it looks convenient and descriptive in code when JsEngine is actually available only if JsContext is successfully instantiated. If weak_ptr references already destroyed JsEngine then it throws exception JsEngine::JsEngineNotAvailableException. - move creating of JsContext at the beginning of some methods of JsValue because it avoids additional pairs of release/lock operations. - add std::shared_ptr<JsEngine> Utils::lockJsEngine(const std::weak_ptr<JsEngine>& jsEngine) which throws JsEngine::JsEngineNotAvailableException if it cannot obtain not nullptr. - add a couple of tests against regressions involving the leakage of JsEngine. - move "jsEngine = CreateJsEngine();" into SetUp of the fixture for Prefs tests. # depends on https://codereview.adblockplus.org/29361582/

Patch Set 1 #

Total comments: 10

Patch Set 2 : address comments #

Total comments: 8

Patch Set 3 : address comments #

Patch Set 4 : temporary workaround for race condition #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -99 lines) Patch
M include/AdblockPlus/JsEngine.h View 1 chunk +15 lines, -0 lines 0 comments Download
M include/AdblockPlus/JsValue.h View 1 1 chunk +3 lines, -3 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 6 chunks +15 lines, -13 lines 0 comments Download
M src/FilterEngine.cpp View 1 8 chunks +16 lines, -8 lines 0 comments Download
M src/JsContext.h View 1 1 chunk +5 lines, -2 lines 0 comments Download
M src/JsContext.cpp View 1 1 chunk +7 lines, -4 lines 0 comments Download
M src/JsValue.cpp View 1 2 4 chunks +65 lines, -57 lines 1 comment Download
M src/Notification.cpp View 1 3 chunks +5 lines, -2 lines 0 comments Download
M src/Thread.cpp View 2 chunks +8 lines, -1 line 0 comments Download
M src/Utils.h View 2 chunks +3 lines, -0 lines 0 comments Download
M src/Utils.cpp View 2 chunks +8 lines, -0 lines 0 comments Download
M src/WebRequestJsObject.cpp View 1 2 chunks +6 lines, -6 lines 0 comments Download
M test/BaseJsTest.cpp View 1 chunk +1 line, -2 lines 0 comments Download
M test/FilterEngine.cpp View 1 2 3 2 chunks +26 lines, -0 lines 0 comments Download
M test/JsEngine.cpp View 1 chunk +20 lines, -0 lines 0 comments Download
M test/Prefs.cpp View 2 chunks +1 line, -1 line 0 comments Download

Messages

Total messages: 9
sergei
Nov. 3, 2016, 11:30 a.m. (2016-11-03 11:30:04 UTC) #1
Oleksandr
I think I see the reason for this change. Weird how this hasn't been noticed ...
Nov. 25, 2016, 10:38 a.m. (2016-11-25 10:38:04 UTC) #2
sergei
https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus/JsValue.h#newcode137 include/AdblockPlus/JsValue.h:137: std::weak_ptr<JsEngine> m_jsEngine; On 2016/11/25 10:38:04, Oleksandr wrote: > Nit: ...
Nov. 25, 2016, 12:04 p.m. (2016-11-25 12:04:46 UTC) #3
Eric
This review is addressing concerns outside the scope of issue #3594. The topic of the ...
Nov. 28, 2016, 2:21 p.m. (2016-11-28 14:21:13 UTC) #4
sergei
On 2016/11/28 14:21:13, Eric wrote: > This review is addressing concerns outside the scope of ...
Nov. 29, 2016, 10:45 a.m. (2016-11-29 10:45:19 UTC) #5
sergei
https://codereview.adblockplus.org/29361562/diff/29366535/src/JsValue.cpp File src/JsValue.cpp (right): https://codereview.adblockplus.org/29361562/diff/29366535/src/JsValue.cpp#newcode42 src/JsValue.cpp:42: JsContext context(jsEngine); Although I have not seen a crash ...
Dec. 1, 2016, 10:24 p.m. (2016-12-01 22:24:38 UTC) #6
Eric
On 2016/11/29 10:45:19, sergei wrote: > Yes, it addresses also other questions but it still ...
Dec. 2, 2016, 4:41 p.m. (2016-12-02 16:41:04 UTC) #7
sergei
On 2016/12/02 16:41:04, Eric wrote: > On 2016/11/29 10:45:19, sergei wrote: > > Yes, it ...
Dec. 2, 2016, 6:47 p.m. (2016-12-02 18:47:11 UTC) #8
Eric
Dec. 2, 2016, 8:20 p.m. (2016-12-02 20:20:22 UTC) #9
Message was sent while issue was closed.
On 2016/12/02 18:47:11, sergei wrote:
> The actual issue is #3593 and I planned to have for presence the proposed hack
> with weak pointers while we properly address #3593 at another level.

Ah. I think your idea to use weak pointers in some way might still be useful, so
let's keep it mind. 

> We have already issue #3595 and we should never kill threads, they should
> gracefully finish.

I used "kill" rather loosely. Certain threads might be informed that they needs
to gracefully finish immediately, even if they think they're not done yet. We're
not using C++11 threads in libadblockplus, I see. Do we have build limitations
with <thread>? If we don't, using <mutex> and <condition_variable> would make
all this much easier. If not C++11 threads, we could use Boost threads, which
are almost identical (since that's where they were incubated). I'm fairly sure
that this would work even with an older Boost version, since its thread library
has been around for years.

For example, in TimeoutThread we're currently calling `Sleep()`. If that call
could wait on a condition variable (with timeout) instead, we could terminate
the thread externally. There's some delicacy about ensuring the function and its
argument aren't being used, but nothing too difficult. We'd need the
function/arguments to be weak pointers so the engine could die, but we know how
to do that. 

> >  What situation do we
> > have now where an engine (or related) disappears out from under a value?
> I'm not sure that I understood the question correctly. In the proposed changes
> JsEngine can go away from JsValue held by working threads.

You understood well enough. It sounds like there's no current behavior like
that, so anything like it would be new code. I wanted to make sure we weren't
working on any existing problem here.

> What about having https://issues.adblockplus.org/ticket/4692 for present
instead
> of the current changes?

That's a good start. I think it would be good to state in the description that
the issue refer both to direct references and indirect chains of reference. Not
sure we have any indirect references, but if we do, they'd be part of the
problem.

I'd also suggest a [meta] issue for all these v8 version upgrade issues. It
would make them easier to find. Maybe two, one for code and one for build.

Powered by Google App Engine
This is Rietveld