d8: Fix some TSAN bugs

d8: Fix some TSAN bugs

* Fix embarrassing bug in DeserializeValue, using a static buffer in
multithreaded code.
* Fix thread leak when Worker.terminate() is not called.

R=jarin@chromium.org

Committed: https://crrev.com/7036a0b1b8206c9d971a28bc523c8d7a2d529e43
Cr-Commit-Position: refs/heads/master@{#29626}

[binji, 2015-07-09 00:13:12]

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc
File src/d8.cc (left):

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#oldcode2146
src/d8.cc:2146: static char s_buffer[128];
I'm pretty ashamed of this one... :-|

[Jarin, 2015-07-10 10:43:18]

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1703
src/d8.cc:1703: case STOPPED:
Why do we want to retry when STOPPED?

Actually, I am even confused why we are trying to go through the TERMINATING
state in WaitForThread. Why (instead of calling Terminate) don't you CAS
RUNNING->JOINING?

It would be helpful if you could describe what was wrong with the original state
machine.

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1708
src/d8.cc:1708: case JOINING:
Is not this only called when the shell finishes, so there should not be anyone
else joining?

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode2187
src/d8.cc:2187: char* p = new char[length];
How about using std::vector<char> here, instead of the naked pointer?

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode2268
src/d8.cc:2268: // Now that all workers are terminated, we can re-enable Worker
creation.
I know this has not be introduced by this CL, but why do we want to re-enable
worker creation? At this point, the process is about to die, no?

https://codereview.chromium.org/1226143003/diff/1/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.h#newcode261
src/d8.h:261: enum State { IDLE, RUNNING, TERMINATING, STOPPED, JOINING, JOINED
};
Could we have some descriptions of what the states mean?

[binji, 2015-07-10 17:40:46]

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1703
src/d8.cc:1703: case STOPPED:
On 2015/07/10 at 10:43:17, jarin wrote:
> Why do we want to retry when STOPPED?
> 
> Actually, I am even confused why we are trying to go through the TERMINATING
state in WaitForThread. Why (instead of calling Terminate) don't you CAS
RUNNING->JOINING?
> 
> It would be helpful if you could describe what was wrong with the original
state machine.

You're right, this is too complicated. I refactored this into a much simpler
change.

Basically, the problem was that the pthread object would leak if
Worker.terminate() was never called. Really all that needed to be done was call
base::Thread::Join(), but that crashes if you call it twice. I wanted to catch
that programmer error, but adding it to the state_ variable made it too complex.

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1708
src/d8.cc:1708: case JOINING:
On 2015/07/10 at 10:43:17, jarin wrote:
> Is not this only called when the shell finishes, so there should not be anyone
else joining?

True, this was just caution to prevent multiple joiners (which crashes). I've
refactored this into a much simpler change.

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode2187
src/d8.cc:2187: char* p = new char[length];
On 2015/07/10 at 10:43:17, jarin wrote:
> How about using std::vector<char> here, instead of the naked pointer?

Done (though I was not sure that std containers were allowed in this file, it
seems like they were explicitly avoided).

https://codereview.chromium.org/1226143003/diff/1/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.h#newcode261
src/d8.h:261: enum State { IDLE, RUNNING, TERMINATING, STOPPED, JOINING, JOINED
};
On 2015/07/10 at 10:43:17, jarin wrote:
> Could we have some descriptions of what the states mean?

Done.

[Jarin, 2015-07-13 06:08:53]

lgtm

https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc#newcode1693
src/d8.cc:1693: if (base::NoBarrier_CompareAndSwap(&join_called_, false, true)
== false) {
It looks like join_called_ is only used for debugging. Could we perhaps just
DCHECK here? (Hmm, the compiler might complain about join_called_ not being
used.)

[binji, 2015-07-13 19:45:48]

The CQ bit was checked by binji@chromium.org

[binji, 2015-07-13 19:45:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1226143003/#ps80001
(title: "feedback")

[commit-bot: I haz the power, 2015-07-13 19:46:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226143003/80001

[commit-bot: I haz the power, 2015-07-13 19:51:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 19:51:05]

Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

(exceeded global retry quota)

[binji, 2015-07-13 19:57:55]

https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc#newcode1693
src/d8.cc:1693: if (base::NoBarrier_CompareAndSwap(&join_called_, false, true)
== false) {
On 2015/07/13 at 06:08:53, jarin wrote:
> It looks like join_called_ is only used for debugging. Could we perhaps just
DCHECK here? (Hmm, the compiler might complain about join_called_ not being
used.)

Done. Oops, yes, looks like there are compiler errors.

[binji, 2015-07-13 19:58:02]

The CQ bit was checked by binji@chromium.org

[binji, 2015-07-13 19:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1226143003/#ps100001
(title: "can't use DCHECK for join_called_, unused in NDEBUG builds")

[commit-bot: I haz the power, 2015-07-13 19:58:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226143003/100001

[commit-bot: I haz the power, 2015-07-13 21:05:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-13 21:05:16]

Patchset 6 (id:??) landed as
https://crrev.com/7036a0b1b8206c9d971a28bc523c8d7a2d529e43
Cr-Commit-Position: refs/heads/master@{#29626}