Move StackGuard::InterruptRequested into StackLimitCheck.

Move StackGuard::InterruptRequested into StackLimitCheck.

R=yangguo@chromium.org

Committed: https://crrev.com/c9f3d892b20b15d256182295810c611aa7d31ebb
Cr-Commit-Position: refs/heads/master@{#30333}

[Michael Starzinger, 2015-08-24 14:40:06]

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h
File src/json-parser.h (right):

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h#newcode267
src/json-parser.h:267: ExecutionAccess access(isolate_);
Aehm, are we sure that acquiring the lock here is correct? This will hold on to
the lock while the GC is active. This looks majorly scary to me. The lock should
only be needed to do the interrupt checking, which is done inside
StackGuard::CheckAndClearInterrupt where the lock is acquired explicitly.

[Yang, 2015-08-24 14:50:43]

lgtm

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h
File src/json-parser.h (right):

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h#newcode267
src/json-parser.h:267: ExecutionAccess access(isolate_);
On 2015/08/24 14:40:06, Michael Starzinger wrote:
> Aehm, are we sure that acquiring the lock here is correct? This will hold on
to
> the lock while the GC is active. This looks majorly scary to me. The lock
should
> only be needed to do the interrupt checking, which is done inside
> StackGuard::CheckAndClearInterrupt where the lock is acquired explicitly.

yup that looks both somewhat wrong and also unnecessary. though i dont think
this will lead to a dead lock. we dont expect an interrupt to trigger during gc.

[Michael Starzinger, 2015-08-24 14:56:47]

Thanks.

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h
File src/json-parser.h (right):

https://codereview.chromium.org/1310253002/diff/1/src/json-parser.h#newcode267
src/json-parser.h:267: ExecutionAccess access(isolate_);
On 2015/08/24 14:50:43, Yang wrote:
> On 2015/08/24 14:40:06, Michael Starzinger wrote:
> > Aehm, are we sure that acquiring the lock here is correct? This will hold on
> to
> > the lock while the GC is active. This looks majorly scary to me. The lock
> should
> > only be needed to do the interrupt checking, which is done inside
> > StackGuard::CheckAndClearInterrupt where the lock is acquired explicitly.
> 
> yup that looks both somewhat wrong and also unnecessary. though i dont think
> this will lead to a dead lock. we dont expect an interrupt to trigger during
gc.

Acknowledged. I discussed this briefly with Hannes as well. We think that this
tried to fix a missing fence between the limit read in line 266 and the
potential limit reset in line 269, but are not sure yet. Anyways, I am not going
to touch it in this CL, to keep this CL a pure refactoring cleanup. But it still
scares me.

[Michael Starzinger, 2015-08-24 14:58:29]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2015-08-24 14:58:41]

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

[commit-bot: I haz the power, 2015-08-24 14:58:42]

This CL has an open dependency (Issue 1314623002 Patch 20001). Please resolve
the dependency and try again.

[Michael Starzinger, 2015-08-24 15:00:58]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2015-08-24 15:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1310253002/#ps20001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-08-24 15:01:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310253002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310253002/20001

[commit-bot: I haz the power, 2015-08-24 15:24:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-24 15:25:03]

Patchset 2 (id:??) landed as
https://crrev.com/c9f3d892b20b15d256182295810c611aa7d31ebb
Cr-Commit-Position: refs/heads/master@{#30333}