The CL that 'breaks' the tests: https://chromium-review.googlesource.com/c/483199/

After that CL, the results are the same on chrome and firefox.

@kinuko, @falken: Does the change in behavior make sense to you? See https://storage.googleapis.com/chromium-layout-test-archives/V8-Blink_Linux_64/15092/layout-test-results/results.html for the test failures.

Sorry that I missed the issue, as we chatted offline I'm not yet 100% sure if (with or without your change) we're not leaking anything with the current code. The original change that was added with the test (i.e. http/tests/serviceworker/chromium/frame-detached-by-navigation.html) is no longer there and the 'expected' behavior of the test seems to be coming somewhere else now.  Let me investigate this / talk to a few more people and update this issue again.

Some notes:
https://codereview.chromium.org/1308723003
is the one that added the original service worker test, and 

https://codereview.chromium.org/1374533002
is the one that has redone the nullifying Frame in DOMWindow patch. So after detaching it there should be no chance that navigator is created with an unexpected frame (and in my understanding that's not about retaining access to the old frame that was available but about being able to access the old frame that shouldn't have become available from the beginning).

https://chromium-review.googlesource.com/c/483199/
This is the change that will make the test fail, but it shouldn't be adding any new threat by what the change is doing.

I verified that in either case (with or without the patch) the case that was originally reported doesn't work, and in the current code I also see that changing 'return navigator' to 'return this.navigator' makes it return non-null object.

All in all this change seems fine and the test's probably just not relevant any more, but because I don't think I fully understand what's happening cc-ing yukishiino@ (and yhirano@ too) just in case he might be able to give more insights.  Thanks!

+cc: domenic@ as an expert

My understanding is that we're discussing whether global proxy or global object should be used in the following three cases.

a) this.domApi (e.g. this.navigator)
b) domApi      (e.g. navigator without |this|)
c) nonDomApi   (e.g. JS global variable)

I'm thinking the following way.

a) this.domApi (e.g. this.navigator)
https://html.spec.whatwg.org/multipage/browsers.html#initialise-the-document-object
step 4-1. For the global this value, use browsingContext's WindowProxy object.

So, globalEnvRec.[[GlobalThisValue]] of ECMAScript is WindowProxy (global proxy) in HTML.  Thus |this| must be the global proxy, which points to the new global object after navigation.

b) domApi      (e.g. navigator without |this|)
c) nonDomApi   (e.g. JS global variable)
I'm not 100% sure, but I guess that, in these two cases, globalEnvRec.[[GlobalThisValue]] will not be used.  I guess that the global object is used as a lexical scope.  ECMAScript wouldn't say something special about DOM APIs, so we should use the global object in these two cases, right?


By the way, just FYI,
https://bugs.chromium.org/p/chromium/issues/detail?id=707486
we're discussing how to handle the named properties on window, and the idea shown at comment #9 and #12 expects that it's the global object (not the global proxy) in case b) and c).

The change in question changes the "receiver" to be the global proxy; and hence never be the global object. The holder (where the property was found) can still be the global object of course. This indeed makes "domApi" behaves the same as "this.domApi".

To be clear: I don't think that the global object should ever leak to JS. We should always use the global proxy. Otherwise

__defineGetter__("test", function() { return window == this });
test

would return false. And other issues like memory leaks just by handing out references to that version of "window".

The fact that it still is handed out to DOM APIs as receiver is odd; and is what I'm fixing here.

Ah, I see.  The look-up doesn't change at all.
The CL looks good to me.

However, I don't understand how the HTML spec guarantees that a receiver object is always a global proxy.  Probably I was wrong at #8.  b) and c) should use the global proxy?

domenic@, could you help me understand better?

Argh, I hope I get this stuff right...

In general script should never be able to get access to the global object, only the global proxy. This is ensured in two ways:

- In the ES spec, there is no way to get access to the actual global object. For example, `this` in a top-level context returns the global proxy. ("Global this value" in ES spec terms.) However, there are ways to manipulate it; e.g. declaring an implicit global variable (top-level `var x = 5`) will add a property to the global object.

- In the world of web specs, we ensure that we never expose the global object, e.g. self/window/frames all return the global proxy, window.open() returns the WindowProxy of the new window, iframeEl.contentWindow returns the WindowProxy of the iframe, etc.

So:

a) this.domApi

`this` at top level is the global proxy per the ES spec. Get(this, "domApi") will end up in the WindowProxy [[GetOwnProperty]] at https://html.spec.whatwg.org/#windowproxy-getownproperty. But that will just end up in OrdinaryGetOwnProperty(the actual global object, "domApi").

b) domApi

I *believe* this ends up in https://tc39.github.io/ecma262/#sec-global-environment-records-getbindingvalue-n-s. If there is no let/const declaration for domApi, then that ends up in step 4/5, so it ends up basically doing a Get() on the global object (via https://tc39.github.io/ecma262/#sec-object-environment-records-getbindingvalue-n-s).

Here I think the global proxy is never involved.

c) nonDomApi

This again goes to https://tc39.github.io/ecma262/#sec-object-environment-records-getbindingvalue-n-s, and if there's no let/const declaration, it'll again go to the global object.

I don't know how this relates to the test failures in the OP, or issues of "receiver" vs. not. But I do know this stuff is generally observable... Happy to help with any more specific questions.

> By the way, just FYI,
https://bugs.chromium.org/p/chromium/issues/detail?id=707486
we're discussing how to handle the named properties on window, and the idea shown at comment #9 and #12 expects that it's the global object (not the global proxy) in case b) and c).

That is indeed how it is specced; the Window object has a named getter, and the WindowProxy's [[GetOwnProperty]] just passes through to that.

> __defineGetter__("test", function() { return window == this });

This defines the getter on the global object. But when the getter function is invoked, we end up in https://tc39.github.io/ecma262/#sec-ordinarycallbindthis, which in step 6.a.iv sets the `this` to the global this value, not the global object. So the == is true.

> b) domApi
>
> I *believe* this ends up in https://tc39.github.io/ecma262/#sec-global-environment-records-getbindingvalue-n-s. If there is no let/const declaration for domApi, then that ends up in step 4/5, so it ends up basically doing a Get() on the global object (via https://tc39.github.io/ecma262/#sec-object-environment-records-getbindingvalue-n-s).
>
> Here I think the global proxy is never involved.

Got it.  So, `domApi` is found on the global object.  And then, its accessor get function is called with the global proxy as `this` object due to OrdinaryCallBindThis, where the global proxy may point to another global object in case of navigation.

The CL is implementing OrdinaryCallBindThis part correctly.

Now everything makes sense to me.

Thanks Shiino-san and Domenic for decrypting this!  I think we'll just need to revise the test (e.g. maybe testing if navigator or this.navigator is actually an instance-of the new Navigator, which should the one created with null frame)

> > __defineGetter__("test", function() { return window == this });
>
> This defines the getter on the global object. But when the getter function is
> invoked, we end up in https://tc39.github.io/ecma262/#sec-ordinarycallbindthis, > which in step 6.a.iv sets the `this` to the global this value, not the global
> object. So the == is true.

It seems like [[Get]] (and the getter) is called with the "bindings object" as "this" through 6.2.4.1 step 6, which calls 8.1.1.2.6 step 5. And the bindings object is set in 8.2.3 step 5 to the globalObj.

That would make it seem like my interpretation is wrong and this should return false? Am I reading this wrong?

All of this is quite surprising to me, and seems to behave differently from what FF implements as well...

Actually it's 8.1.1.4.6 instead of 8.1.1.2.6 since it's a global reference, but that uses the [[ObjectRecord]] created in 8.1.2.5 which is an object record that has G as the bindings object as well. Which I suppose ends up in 8.1.1.2.6 anyway.

It looks like you are right. I've filed https://github.com/tc39/ecma262/issues/907 to see if we're missing something, or if this is a spec bug.

There are still NeedsManualRebaseline lines in TestExpectations associated with this bug; we should remember to follow up by manually rebaselining.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/ba8a753947736226cff422ba87f8f62feb00e12a

commit ba8a753947736226cff422ba87f8f62feb00e12a
Author: Toon Verwaest <verwaest@chromium.org>
Date: Thu Jun 01 09:07:50 2017

Reland "[runtime] Pass global proxy as receiver to native accessors in case of contextual access"

Based on past discussions I'm going to try to reland this change. This makes window.document and document behave the same after navigation, which is a change from what the spec says. If this works out though, it would greatly simplify the spec; and fix the fact that currently it's leaking the underlying global object, which we don't want for security and object-identity reasons.

Bug:  chromium:713732 
Change-Id: I835ef510fc78f04c602434a7cec6420e027c4012
Reviewed-on: https://chromium-review.googlesource.com/520764
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45654}
[modify] https://crrev.com/ba8a753947736226cff422ba87f8f62feb00e12a/src/objects.cc
[modify] https://crrev.com/ba8a753947736226cff422ba87f8f62feb00e12a/test/cctest/test-api.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c588bf858fa9cb28b1923d0ac0654ef2f345137c

commit c588bf858fa9cb28b1923d0ac0654ef2f345137c
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Jun 01 12:26:47 2017

Revert "Reland "[runtime] Pass global proxy as receiver to native accessors in case of contextual access""

This reverts commit ba8a753947736226cff422ba87f8f62feb00e12a.

Reason for revert: A layout test is unhappy:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/16010

Original change's description:
> Reland "[runtime] Pass global proxy as receiver to native accessors in case of contextual access"
> 
> Based on past discussions I'm going to try to reland this change. This makes window.document and document behave the same after navigation, which is a change from what the spec says. If this works out though, it would greatly simplify the spec; and fix the fact that currently it's leaking the underlying global object, which we don't want for security and object-identity reasons.
> 
> Bug:  chromium:713732 
> Change-Id: I835ef510fc78f04c602434a7cec6420e027c4012
> Reviewed-on: https://chromium-review.googlesource.com/520764
> Commit-Queue: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#45654}

TBR=haraken@chromium.org,verwaest@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:713732 

Change-Id: Iecde1cd855c21efa73939bbfbff0c26540ee2d98
Reviewed-on: https://chromium-review.googlesource.com/521045
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45659}
[modify] https://crrev.com/c588bf858fa9cb28b1923d0ac0654ef2f345137c/src/objects.cc
[modify] https://crrev.com/c588bf858fa9cb28b1923d0ac0654ef2f345137c/test/cctest/test-api.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4eeb1274a17cd247778ee11666e0b18d9114e0ea

commit 4eeb1274a17cd247778ee11666e0b18d9114e0ea
Author: verwaest <verwaest@chromium.org>
Date: Fri Jun 02 09:50:25 2017

Disable tests to fix contextual property access

BUG= chromium:713732 

Review-Url: https://codereview.chromium.org/2916713006
Cr-Commit-Position: refs/heads/master@{#476610}

[modify] https://crrev.com/4eeb1274a17cd247778ee11666e0b18d9114e0ea/third_party/WebKit/LayoutTests/TestExpectations

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1e813e5341ec74413ff52bce02788e499b994a72

commit 1e813e5341ec74413ff52bce02788e499b994a72
Author: Toon Verwaest <verwaest@chromium.org>
Date: Fri Jun 02 12:21:33 2017

Reland "[runtime] Pass global proxy as receiver to native accessors in case of contextual access"

Based on past discussions I'm going to try to reland this change. This makes window.document and document behave the same after navigation, which is a change from what the spec says. If this works out though, it would greatly simplify the spec; and fix the fact that currently it's leaking the underlying global object, which we don't want for security and object-identity reasons.

Bug:  chromium:713732 
Change-Id: I5ce89afb46349ff92b7f5a884a7c388fcff887bf
Reviewed-on: https://chromium-review.googlesource.com/522605
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45678}
[modify] https://crrev.com/1e813e5341ec74413ff52bce02788e499b994a72/src/objects.cc
[modify] https://crrev.com/1e813e5341ec74413ff52bce02788e499b994a72/test/cctest/test-api.cc

Note, there are still NeedsManualRebaseline lines in TestExpectations associated with this bug; if the current actual results should be correct, you could rebaseline based on current continuous builder results via webkit-patch rebaseline:

./webkit-patch rebaseline fast/dom/Window/dom-access-from-closure-iframe.html fast/dom/Window/dom-access-from-closure-window-with-gc.html fast/dom/Window/dom-access-from-closure-window.html

./webkit-patch rebaseline http/tests/serviceworker/chromium/frame-detached-by-navigation.html

./webkit-patch rebaseline virtual/mojo-loading/http/tests/serviceworker/chromium/frame-detached-by-navigation.html virtual/off-main-thread-fetch/http/tests/serviceworker/chromium/frame-detached-by-navigation.html virtual/service-worker-script-streaming/http/tests/serviceworker/chromium/frame-detached-by-navigation.html

IIRC, verwaest@ is ooo for a month or two.  falken@ or kinuko@, could you take care of this?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8af846a42f5d526148e97385ecdcc35184e08b45

commit 8af846a42f5d526148e97385ecdcc35184e08b45
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Nov 09 11:00:38 2017

service worker: Upstream frame removal/detached tests.

This expands and upstreams:
* chromium/frame-detached-by-navigation.html
* chromium/frame-removed.html

Chromium behavior seems a bit weird but for now upload the WPT
to get the test visible, and we can refine the expectations later.

Bug:  713732 ,  522791 ,  688116 
Change-Id: I4af7fb668f9ef2e4aaff580ba129e6ae353f0e18
Reviewed-on: https://chromium-review.googlesource.com/760061
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515134}
[modify] https://crrev.com/8af846a42f5d526148e97385ecdcc35184e08b45/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/8af846a42f5d526148e97385ecdcc35184e08b45/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/8af846a42f5d526148e97385ecdcc35184e08b45/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/detached-context.https.html
[delete] https://crrev.com/bd4e3a9b4d6469e2e191747bb00c3779d9d46e10/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/frame-detached-by-navigation.html
[delete] https://crrev.com/bd4e3a9b4d6469e2e191747bb00c3779d9d46e10/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/frame-removed.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/770dd75a3e0a6d60fbebb072053f82591a167976

commit 770dd75a3e0a6d60fbebb072053f82591a167976
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Nov 09 20:31:34 2017

Gardening: Rebaseline dom-access-from-closure* tests.

This removes the failing expectations so we have test coverage:
* fast/dom/Window/dom-access-from-closure-iframe.html [ NeedsManualRebaseline ]
* fast/dom/Window/dom-access-from-closure-window-with-gc.html [ NeedsManualRebaseline ]
* fast/dom/Window/dom-access-from-closure-window.html [ NeedsManualRebaseline ]

Bug:  713732 
Change-Id: Icb2e7f23854a96affb32cdff6ff3a2bf832daa19
TBR: verwaest
Reviewed-on: https://chromium-review.googlesource.com/760658
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515267}
[modify] https://crrev.com/770dd75a3e0a6d60fbebb072053f82591a167976/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/770dd75a3e0a6d60fbebb072053f82591a167976/third_party/WebKit/LayoutTests/fast/dom/Window/dom-access-from-closure-iframe-expected.txt
[modify] https://crrev.com/770dd75a3e0a6d60fbebb072053f82591a167976/third_party/WebKit/LayoutTests/fast/dom/Window/dom-access-from-closure-window-expected.txt
[modify] https://crrev.com/770dd75a3e0a6d60fbebb072053f82591a167976/third_party/WebKit/LayoutTests/fast/dom/Window/dom-access-from-closure-window-with-gc-expected.txt

- service worker tests were upstreamed to WPT and don't need expectations
- I just rebaselined the dom-access* tests without understanding what the test is doing. Please check the expected.txt files if you are concerned.