Allow allocation and GC in access check callbacks.

Allow allocation and GC in access check callbacks.

R=ishell@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20730

[Yang, 2014-04-11 14:17:17]

On 2014/04/11 12:55:06, Yang wrote:

changed as suggested.

[Igor Sheludko, 2014-04-11 17:10:55]

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc#newcode696
src/isolate.cc:696: AccessCheckInfo* GetAccessCheckInfo(Isolate* isolate,
static inline?

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc#newcode722
src/isolate.cc:722: data = handle(GetAccessCheckInfo(this, receiver)->data(),
this);
I think you intended to use |access_check_info| here instead of
GetAccessCheckInfo(...).

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc#newcode5595
src/objects.cc:5595: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
While we are here: after majority of calls to ReportFailedAccessCheck() we check
for scheduled exception but...

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc#newcode6372
src/objects.cc:6372: isolate->ReportFailedAccessCheck(object, v8::ACCESS_SET);
... but here and in some other places we don't. And we don't even check for
scheduled exceptions after some calls to DefineAccessor(). Is it worth adding
the exception check to ReportFailedAccessCheck()?

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc#newcode13290
src/objects.cc:13290: isolate->ReportFailedAccessCheck(object, v8::ACCESS_HAS);
Same note here and below.

[Yang, 2014-04-14 06:22:51]

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc#newcode696
src/isolate.cc:696: AccessCheckInfo* GetAccessCheckInfo(Isolate* isolate,
On 2014/04/11 17:10:55, Igor Sheludko wrote:
> static inline?

Done.

https://codereview.chromium.org/234913003/diff/10001/src/isolate.cc#newcode722
src/isolate.cc:722: data = handle(GetAccessCheckInfo(this, receiver)->data(),
this);
On 2014/04/11 17:10:55, Igor Sheludko wrote:
> I think you intended to use |access_check_info| here instead of
> GetAccessCheckInfo(...). 

Done. Thanks for catching this.

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/234913003/diff/10001/src/objects.cc#newcode6372
src/objects.cc:6372: isolate->ReportFailedAccessCheck(object, v8::ACCESS_SET);
On 2014/04/11 17:10:55, Igor Sheludko wrote:
> ... but here and in some other places we don't. And we don't even check for
> scheduled exceptions after some calls to DefineAccessor(). Is it worth adding
> the exception check to ReportFailedAccessCheck()?

I know about this, but I have no idea whether this will cause actual problems,
and prefer to investigate this later on. I opened an issue for this and added
TODOs.

[Igor Sheludko, 2014-04-14 12:41:01]

lgtm

[Yang, 2014-04-14 14:03:52]

Committed patchset #3 manually as r20730 (presubmit successful).