Fixed global object leak

Fixed global object leak caused by overwriting the global receiver (the global proxy) in the global object with the global object itself.

This CL additionally removes the API function to reattach a global proxy to a
global object.

BUG=324812
LOG=y
R=dcarney@chromium.org, titzer@chromium.org

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

[Toon Verwaest, 2013-12-03 12:23:24]

PTAL

[dcarney, 2013-12-03 12:29:52]

On 2013/12/03 12:23:24, Toon Verwaest wrote:
> PTAL

lgtm, as long as you're sure that unit test is meaningless

[adamk, 2013-12-03 21:46:11]

https://chromiumcodereview.appspot.com/101733002/diff/20001/test/cctest/test-...
File test/cctest/test-object-observe.cc (left):

https://chromiumcodereview.appspot.com/101733002/diff/20001/test/cctest/test-...
test/cctest/test-object-observe.cc:262:
CHECK(inner_global->StrictEquals(CompileRun("records[1].object")));
Seems like we still want to have a test for what happens if you cause a mutation
to the global (via, e.g., a closure); we want to make sure Object.observe
doesn't leak the global object in ChangeRecord.object.

Can you at least leave a TODO here to test that case (if adding such a test now
is too burdensome)?

[dcarney, 2013-12-10 11:18:10]

ptal

[Toon Verwaest, 2013-12-10 12:11:02]

LGTM with nit.
In the future we probably want to remove the global-receiver/global-proxy and
global-object/global-context distinction...

https://chromiumcodereview.appspot.com/101733002/diff/80001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/101733002/diff/80001/src/api.cc#newcod...
src/api.cc:5416: global)->IsDetachedFrom(context->global_object())) {
That's weird indentation, what about
if (i::Handle<i::JSGlobalProxy>::cast(global)->IsDetachedFrom(
        context->global_object())) {
or just
i::Handle<i::JSGlobalProxy> global_proxy =
i::Handle<i::JSGlobalProxy>::cast(global);
if (global_proxy->IsDetachedFrom(context->global_object())) {

[titzer, 2013-12-11 13:34:40]

LGTM.
Sheriff's note: please run all webkit tests before landing

[dcarney, 2013-12-11 13:35:34]

On 2013/12/11 13:34:40, titzer wrote:
> LGTM.
> Sheriff's note: please run all webkit tests before landing

already done

[Toon Verwaest, 2013-12-11 13:52:03]

Committed patchset #6 manually as r18299.

[haraken, 2016-05-18 05:58:31]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-18 05:58:33]

(Reviving this very old CL...)

I'm now investigating security exploits on detached iframes.

https://chromiumcodereview.appspot.com/101733002/diff/100001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/101733002/diff/100001/src/api.cc#newco...
src/api.cc:5417: global = i::Handle<i::Object>(context->global_object(),
isolate);

It looks like that this is doing something dangerous. When Blink calls
Context::Global for an detached iframe, it returns a global object (which may be
already used by another iframe), not the global proxy object.

Is the TODO still valid?

[Toon Verwaest, 2016-05-30 21:34:12]

https://codereview.chromium.org/101733002/diff/100001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/101733002/diff/100001/src/api.cc#newcode5417
src/api.cc:5417: global = i::Handle<i::Object>(context->global_object(),
isolate);
On 2016/05/18 05:58:32, haraken wrote:
> 
> It looks like that this is doing something dangerous. When Blink calls
> Context::Global for an detached iframe, it returns a global object (which may
be
> already used by another iframe), not the global proxy object.
> 
> Is the TODO still valid?
> 

Yes. We should never leak the naked global object to blink.