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

Issue 29418610: Noissue - Create context in ~JsValue() before disposing value (Closed)

Created:
April 20, 2017, 12:45 p.m. by hub
Modified:
April 20, 2017, 1:31 p.m.
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Noissue - Create context in ~JsValue() before disposing value

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -0 lines) Patch
M src/JsValue.cpp View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 4
hub
April 20, 2017, 12:45 p.m. (2017-04-20 12:45:45 UTC) #1
sergei
LGTM, but I would like to explain it a bit in the commit message. We ...
April 20, 2017, 12:50 p.m. (2017-04-20 12:50:05 UTC) #2
hub
On 2017/04/20 12:50:05, sergei wrote: > LGTM, but I would like to explain it a ...
April 20, 2017, 1:13 p.m. (2017-04-20 13:13:21 UTC) #3
sergei
April 20, 2017, 1:16 p.m. (2017-04-20 13:16:55 UTC) #4
On 2017/04/20 13:13:21, hub wrote:
> On 2017/04/20 12:50:05, sergei wrote:
> > LGTM, but I would like to explain it a bit in the commit message.
> > 
> > We need it because it can be a reason of data race and of race condition.
> > Instances of JsValue can being destroyed from different threads or an
instance
> > of JsValue can being destroyed while some JS code is being executed and in
> both
> > such cases two threads will try to access and modify isolate at the same
> moment.
> 
> Here is the full commit message now: 
> 
> "
> Noissue - Create context in ~JsValue() before disposing value
> 
> This is needed to ensure there is no data race as JS code could
> be running on a different thread.
> "
> 
> Is this enough?

I would add where exactly the the race
Instances of JsValue can being destroyed from different threads or an instance
of JsValue can being destroyed while some JS code is being executed and in
both such cases two threads will try to access and modify isolate at the same
moment.

BTW, don't forget about review URL.

Powered by Google App Engine
This is Rietveld