Fire 'stack' getter of error objects after GC.

Fire 'stack' getter of error objects after GC.

BUG=v8:2340

[Yang, 2012-11-15 10:44:32]

I would be really happy if you could take a look at this.

[Vyacheslav Egorov (Google), 2012-11-16 01:09:57]

LGTM iff the concern about .stack getter replacement can be addressed [recognize
one shot accessor?]

http://codereview.chromium.org/11377158/diff/5001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11377158/diff/5001/src/heap.cc#newcode7444
src/heap.cc:7444: Object::GetProperty(js_object, js_object, &lookup, stack_key,
&attr);
here is two interesting questions:

1) what if somebody _replaces_ the .stack getter? can they do it? then they can
observe GC.

2) what if scavenge happens here? scavenges move things down in this array so
our write index might start pointing to something that we did not process and we
end up overwriting it. this does not matter much (there are no correctness
concerns it seems). but it might be worth a comment

[Yang, 2012-11-16 10:45:43]

On 2012/11/16 01:09:57, Vyacheslav Egorov (Google) wrote:
> LGTM iff the concern about .stack getter replacement can be addressed
[recognize
> one shot accessor?]
> 
> http://codereview.chromium.org/11377158/diff/5001/src/heap.cc
> File src/heap.cc (right):
> 
> http://codereview.chromium.org/11377158/diff/5001/src/heap.cc#newcode7444
> src/heap.cc:7444: Object::GetProperty(js_object, js_object, &lookup,
stack_key,
> &attr);
> here is two interesting questions:
> 
> 1) what if somebody _replaces_ the .stack getter? can they do it? then they
can
> observe GC.
> 
> 2) what if scavenge happens here? scavenges move things down in this array so
> our write index might start pointing to something that we did not process and
we
> end up overwriting it. this does not matter much (there are no correctness
> concerns it seems). but it might be worth a comment

Both very valid points, I hope I sufficiently addressed them:
1) I'm "abusing" the native flag to recognize the one shot getters.
2) If the scavenge is triggered by formatting, I recognize that the nested_ flag
is true and don't move things down the list.

Additionally, I changed the behavior of our stack trace API so that if a custom
Error.prepareStackTrace function is defined, we format the stack trace
immediately so that GC cannot be observed this way.

Please take another look.

[Vyacheslav Egorov (Google), 2012-11-20 17:30:42]

sorry for the review delay, was travelling.

https://codereview.chromium.org/11377158/diff/11001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11377158/diff/11001/src/heap.cc#newcode7459
src/heap.cc:7459: if (!getter_fun->shared()->native()) continue;
I don't think this can guard you well enough. Here is an idea: I steal a
function that is known to be native and that has a very predictable behavior
e.g. Array.prototype.pop and then I install my own getter on .length because pop
is known to access it. This way I can track when pop is called (JavaScript is
fun).

What about using function IDs (that Crankshaft uses for optimization) to detect
this case safely? You'll need to introduce %SetFunctionId thingy though or
something...

[Yang, 2012-11-22 12:31:52]

On 2012/11/20 17:30:42, Vyacheslav Egorov (Google) wrote:
> sorry for the review delay, was travelling.
> 
> https://codereview.chromium.org/11377158/diff/11001/src/heap.cc
> File src/heap.cc (right):
> 
> https://codereview.chromium.org/11377158/diff/11001/src/heap.cc#newcode7459
> src/heap.cc:7459: if (!getter_fun->shared()->native()) continue;
> I don't think this can guard you well enough. Here is an idea: I steal a
> function that is known to be native and that has a very predictable behavior
> e.g. Array.prototype.pop and then I install my own getter on .length because
pop
> is known to access it. This way I can track when pop is called (JavaScript is
> fun).
> 
> What about using function IDs (that Crankshaft uses for optimization) to
detect
> this case safely? You'll need to introduce %SetFunctionId thingy though or
> something...

On 2012/11/20 17:30:42, Vyacheslav Egorov (Google) wrote:
> sorry for the review delay, was travelling.
> 
> https://codereview.chromium.org/11377158/diff/11001/src/heap.cc
> File src/heap.cc (right):
> 
> https://codereview.chromium.org/11377158/diff/11001/src/heap.cc#newcode7459
> src/heap.cc:7459: if (!getter_fun->shared()->native()) continue;
> I don't think this can guard you well enough. Here is an idea: I steal a
> function that is known to be native and that has a very predictable behavior
> e.g. Array.prototype.pop and then I install my own getter on .length because
pop
> is known to access it. This way I can track when pop is called (JavaScript is
> fun).
> 
> What about using function IDs (that Crankshaft uses for optimization) to
detect
> this case safely? You'll need to introduce %SetFunctionId thingy though or
> something...

Please take yet another look...
- I use a IsBuiltin() check instead of native() to see whether the oneshot
getter is the expected one.
- I changed FormatRawStackTrace to use %_CallFunction instead of
Array.prototype.{push,join} so that you can't observe it being called lazily by
overriding Array.prototype.{push,join}
- I changed FormatRawStackTrace so that the toString method of the error object
is not called lazily, but at the time when the error object is created, so that
the formatting the stack trace is not observable because of stringifying the
error.

[Yang, 2012-12-03 15:01:44]

On 2012/11/22 12:31:52, Yang wrote:
> On 2012/11/20 17:30:42, Vyacheslav Egorov (Google) wrote:
> > sorry for the review delay, was travelling.
> > 
> > https://codereview.chromium.org/11377158/diff/11001/src/heap.cc
> > File src/heap.cc (right):
> > 
> > https://codereview.chromium.org/11377158/diff/11001/src/heap.cc#newcode7459
> > src/heap.cc:7459: if (!getter_fun->shared()->native()) continue;
> > I don't think this can guard you well enough. Here is an idea: I steal a
> > function that is known to be native and that has a very predictable behavior
> > e.g. Array.prototype.pop and then I install my own getter on .length because
> pop
> > is known to access it. This way I can track when pop is called (JavaScript
is
> > fun).
> > 
> > What about using function IDs (that Crankshaft uses for optimization) to
> detect
> > this case safely? You'll need to introduce %SetFunctionId thingy though or
> > something...
> 
> On 2012/11/20 17:30:42, Vyacheslav Egorov (Google) wrote:
> > sorry for the review delay, was travelling.
> > 
> > https://codereview.chromium.org/11377158/diff/11001/src/heap.cc
> > File src/heap.cc (right):
> > 
> > https://codereview.chromium.org/11377158/diff/11001/src/heap.cc#newcode7459
> > src/heap.cc:7459: if (!getter_fun->shared()->native()) continue;
> > I don't think this can guard you well enough. Here is an idea: I steal a
> > function that is known to be native and that has a very predictable behavior
> > e.g. Array.prototype.pop and then I install my own getter on .length because
> pop
> > is known to access it. This way I can track when pop is called (JavaScript
is
> > fun).
> > 
> > What about using function IDs (that Crankshaft uses for optimization) to
> detect
> > this case safely? You'll need to introduce %SetFunctionId thingy though or
> > something...
> 
> Please take yet another look...
> - I use a IsBuiltin() check instead of native() to see whether the oneshot
> getter is the expected one.
> - I changed FormatRawStackTrace to use %_CallFunction instead of
> Array.prototype.{push,join} so that you can't observe it being called lazily
by
> overriding Array.prototype.{push,join}
> - I changed FormatRawStackTrace so that the toString method of the error
object
> is not called lazily, but at the time when the error object is created, so
that
> the formatting the stack trace is not observable because of stringifying the
> error.

This has been a while...
Switched to hidden properties to mark one shot getters. Please take another
look.

[Vyacheslav Egorov (Google), 2012-12-10 14:46:45]

lgtm

https://codereview.chromium.org/11377158/diff/18002/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode815
src/messages.js:815: var ownName = this.fun.name;
If we ever change property descriptor for our 'name' property on functions then
I would be able to detect GC by placing getter on it.

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode823
src/messages.js:823: this.receiver[ownName] === this.fun)) {
I can detect GC through here by doing:

var obj = {
  foo: function foo() { throw new Error(); };
};

try {
  obj.foo();
} catch (e) {
  delete obj.foo;
  Object.defineProperty(obj, 'foo', {
    get: function () { print('GC!'); }
  });
  ERROR = e;
}

Needs a guard of the same sort as done below in for-in loop.

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1031
src/messages.js:1031: 
one more empty line

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1040
src/messages.js:1040: %_CallFunction(frames, new CallSite(recv, fun, pos),
ArrayPush)
This code can still be broken (formatting detected) if one installs setters on
"0", "1", ... on Array.prototype or Object.prototype (ArrayPush calls normal
[[Put]]).

If we want to ensure that this code works despite any modifications that one
somebody does to array then most probably we also need to make frames
InternalArray and then convert it to a normal array via elements transfer before
the return.

The same goes for the lines below.

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1044
src/messages.js:1044: 
ditto

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1065
src/messages.js:1065: function GetTypeName(obj, requireConstructor) {
ditto

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1079
src/messages.js:1079: function captureStackTrace(obj, cons_opt) {
ditto

https://codereview.chromium.org/11377158/diff/18002/src/messages.js#newcode1091
src/messages.js:1091: obj.stack = $Error.prepareStackTrace(obj,
GetStackFrames(raw_stack));
should we maybe catch an error if it flies from prepareStackTrace and replace
obj.stack with it or some other text to indicate "recursive" throw?

do we have a test case that checks such re-entrant throw?

[Yang, 2012-12-10 16:47:40]

Uploaded a new patch. I'll try to find more places where FormatStackTrace could
cause side-effects before landing.