Fix issue 142

Fix issue 142:
- Removed the potential for a NULL pointer access in
  ContextSwitcher::PreemptionReceived.
- Removed a leak of the semaphore in the ContexSwitcher thread, by removing
  the need for this semaphore entirely.
- Added a regression test case which will catch accesses to the ContextSwitcher
  singleton after it has been stopped.


Committed: http://code.google.com/p/v8/source/detail?r=994

[Erik Corry, 2008-12-17 14:30:07]

LGTM.  We still need some better tests.

http://codereview.chromium.org/14483/diff/12/15
File src/v8threads.cc (right):

http://codereview.chromium.org/14483/diff/12/15#newcode310
Line 310: singleton_ = NULL;
Perhaps here we should clear a pending preemption.  It might be confusing to
have it discovered after this point?

http://codereview.chromium.org/14483/diff/12/16
File src/v8threads.h (right):

http://codereview.chromium.org/14483/diff/12/16#newcode91
Line 91: // multiple running V8 threads. Generally it is needed to call
StartPreemption
"it is necessary"

http://codereview.chromium.org/14483/diff/12/16#newcode92
Line 92: // if there is more than one thread running. If not a single JavaScript
can
"If not, a single JavaScript thread can"

http://codereview.chromium.org/14483/diff/12/16#newcode102
Line 102: // Preempted thread needs to calls back to the ContextSwitcher to
acknowlege
"to call back"

http://codereview.chromium.org/14483/diff/12/13
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/14483/diff/12/13#newcode3
Line 3: // Check that we can traverse very deep stacks of ConsStrings using
Stale comment (should be replaced with copyright boilerplate)

[iposva, 2008-12-17 17:39:24]

I agree that we need more tests. By adding this new cctest file we now have a
place where they can go.

-Ivan

http://codereview.chromium.org/14483/diff/12/15
File src/v8threads.cc (right):

http://codereview.chromium.org/14483/diff/12/15#newcode310
Line 310: singleton_ = NULL;
On 2008/12/17 14:30:07, Erik Corry wrote:
> Perhaps here we should clear a pending preemption.  It might be confusing to
> have it discovered after this point?

I disagree. To the running thread a pending preemption will appear like a
spurious interrupt, the thread has to be able to handle that anyway. Also you
would need to iterate through all the threads clearing all pending interrupts.

http://codereview.chromium.org/14483/diff/12/16
File src/v8threads.h (right):

http://codereview.chromium.org/14483/diff/12/16#newcode91
Line 91: // multiple running V8 threads. Generally it is needed to call
StartPreemption
On 2008/12/17 14:30:07, Erik Corry wrote:
> "it is necessary"

Done.

http://codereview.chromium.org/14483/diff/12/16#newcode92
Line 92: // if there is more than one thread running. If not a single JavaScript
can
On 2008/12/17 14:30:07, Erik Corry wrote:
> "If not, a single JavaScript thread can"

Done.

http://codereview.chromium.org/14483/diff/12/16#newcode102
Line 102: // Preempted thread needs to calls back to the ContextSwitcher to
acknowlege
On 2008/12/17 14:30:07, Erik Corry wrote:
> "to call back"

Done.

http://codereview.chromium.org/14483/diff/12/13
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/14483/diff/12/13#newcode3
Line 3: // Check that we can traverse very deep stacks of ConsStrings using
On 2008/12/17 14:30:07, Erik Corry wrote:
> Stale comment (should be replaced with copyright boilerplate)

Done.