Revert "binding: Makes window/frames/self attributes return itself."

Revert "binding: Makes window/frames/self attributes return itself."

> Per HTML spec[1], window, frames, and self attributes must return
> the global proxy object.  This CL simply follows it.
>
> Note that these attributes are expected to never return null.
> We're currently breaking this assumption on detached frames.
>
> [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window

The current fix for this depends on a ScriptState being associated
with the window object. However, this prevents RemoteWindowProxy
from using v8::Context::NewRemoteContext(), as a RemoteWindowProxy
would then have no associated ScriptState.

BUG=618672, 527190
R=haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2643613003
Cr-Commit-Position: refs/heads/master@{#444517}
Committed: https://chromium.googlesource.com/chromium/src/+/a3e6b34d4b686ee9bc9addda6885750964638c39

[dcheng, 2017-01-18 08:06:20]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 08:06:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-18 08:07:36]

If we're actually OK with this, this is the simplest path forward.

In the meantime, I'm going to continue debugging
https://codereview.chromium.org/2642643002/: it looks like that it's possible
for objects to not be collected sometimes now =(

[Yuki, 2017-01-18 08:11:24]

LGTM, but if you can keep a layout test non-reverted (with an updated test
expectation of failures), that's great (otherwise, I need to update the test
again and I'm afraid of forgetting it).

[haraken, 2017-01-18 08:11:57]

LGTM

[haraken, 2017-01-18 08:12:43]

On 2017/01/18 08:11:24, Yuki wrote:
> LGTM, but if you can keep a layout test non-reverted (with an updated test
> expectation of failures), that's great (otherwise, I need to update the test
> again and I'm afraid of forgetting it).

Does it mean that we will regress the web-exposed behavior?

[Yuki, 2017-01-18 08:14:42]

On 2017/01/18 08:12:43, haraken wrote:
> On 2017/01/18 08:11:24, Yuki wrote:
> > LGTM, but if you can keep a layout test non-reverted (with an updated test
> > expectation of failures), that's great (otherwise, I need to update the test
> > again and I'm afraid of forgetting it).
> 
> Does it mean that we will regress the web-exposed behavior?

This revert itself regress a web-exposed behavior a little bit, but it has been
broken for a long time before the CL landed.  So, it's a kind of acceptable, I
think.

[Yuki, 2017-01-18 08:16:05]

On 2017/01/18 08:14:42, Yuki wrote:
> On 2017/01/18 08:12:43, haraken wrote:
> > On 2017/01/18 08:11:24, Yuki wrote:
> > > LGTM, but if you can keep a layout test non-reverted (with an updated test
> > > expectation of failures), that's great (otherwise, I need to update the
test
> > > again and I'm afraid of forgetting it).
> > 
> > Does it mean that we will regress the web-exposed behavior?
> 
> This revert itself regress a web-exposed behavior a little bit, but it has
been
> broken for a long time before the CL landed.  So, it's a kind of acceptable, I
> think.

s/it has been broken/it had been broken/

Anyway, we don't support detached frames gracefully, I don't think this revert
causes a big problem.

[haraken, 2017-01-18 08:28:07]

On 2017/01/18 08:16:05, Yuki wrote:
> On 2017/01/18 08:14:42, Yuki wrote:
> > On 2017/01/18 08:12:43, haraken wrote:
> > > On 2017/01/18 08:11:24, Yuki wrote:
> > > > LGTM, but if you can keep a layout test non-reverted (with an updated
test
> > > > expectation of failures), that's great (otherwise, I need to update the
> test
> > > > again and I'm afraid of forgetting it).
> > > 
> > > Does it mean that we will regress the web-exposed behavior?
> > 
> > This revert itself regress a web-exposed behavior a little bit, but it has
> been
> > broken for a long time before the CL landed.  So, it's a kind of acceptable,
I
> > think.
> 
> s/it has been broken/it had been broken/
> 
> Anyway, we don't support detached frames gracefully, I don't think this revert
> causes a big problem.

ok.

(But in general, it's not good to regress a web-exposed behavior because
RemoteFrames are not well supported.)

[dcheng, 2017-01-18 08:36:02]

On 2017/01/18 08:28:07, haraken wrote:
> On 2017/01/18 08:16:05, Yuki wrote:
> > On 2017/01/18 08:14:42, Yuki wrote:
> > > On 2017/01/18 08:12:43, haraken wrote:
> > > > On 2017/01/18 08:11:24, Yuki wrote:
> > > > > LGTM, but if you can keep a layout test non-reverted (with an updated
> test
> > > > > expectation of failures), that's great (otherwise, I need to update
the
> > test
> > > > > again and I'm afraid of forgetting it).
> > > > 
> > > > Does it mean that we will regress the web-exposed behavior?
> > > 
> > > This revert itself regress a web-exposed behavior a little bit, but it has
> > been
> > > broken for a long time before the CL landed.  So, it's a kind of
acceptable,
> I
> > > think.
> > 
> > s/it has been broken/it had been broken/
> > 
> > Anyway, we don't support detached frames gracefully, I don't think this
revert
> > causes a big problem.
> 
> ok.
> 
> (But in general, it's not good to regress a web-exposed behavior because
> RemoteFrames are not well supported.)

I've updated the CL to avoid reverting the test changes (and check in a failing
expectation to reflect that we're diverging from correct behavior).

I do agree that it's not great to break web-exposed behavior, and I'm hoping to
fix this ASAP in a followup.

[dcheng, 2017-01-18 08:36:12]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 08:36:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-18 08:40:11]

On 2017/01/18 08:36:02, dcheng wrote:
> On 2017/01/18 08:28:07, haraken wrote:
> > On 2017/01/18 08:16:05, Yuki wrote:
> > > On 2017/01/18 08:14:42, Yuki wrote:
> > > > On 2017/01/18 08:12:43, haraken wrote:
> > > > > On 2017/01/18 08:11:24, Yuki wrote:
> > > > > > LGTM, but if you can keep a layout test non-reverted (with an
updated
> > test
> > > > > > expectation of failures), that's great (otherwise, I need to update
> the
> > > test
> > > > > > again and I'm afraid of forgetting it).
> > > > > 
> > > > > Does it mean that we will regress the web-exposed behavior?
> > > > 
> > > > This revert itself regress a web-exposed behavior a little bit, but it
has
> > > been
> > > > broken for a long time before the CL landed.  So, it's a kind of
> acceptable,
> > I
> > > > think.
> > > 
> > > s/it has been broken/it had been broken/
> > > 
> > > Anyway, we don't support detached frames gracefully, I don't think this
> revert
> > > causes a big problem.
> > 
> > ok.
> > 
> > (But in general, it's not good to regress a web-exposed behavior because
> > RemoteFrames are not well supported.)
> 
> I've updated the CL to avoid reverting the test changes (and check in a
failing
> expectation to reflect that we're diverging from correct behavior).
> 
> I do agree that it's not great to break web-exposed behavior, and I'm hoping
to
> fix this ASAP in a followup.

I might be behind but if you can fix it in a follow-up, would it be hard to fix
it before landing this CL?

[dcheng, 2017-01-18 08:55:50]

On 2017/01/18 08:40:11, haraken wrote:
> On 2017/01/18 08:36:02, dcheng wrote:
> > On 2017/01/18 08:28:07, haraken wrote:
> > > On 2017/01/18 08:16:05, Yuki wrote:
> > > > On 2017/01/18 08:14:42, Yuki wrote:
> > > > > On 2017/01/18 08:12:43, haraken wrote:
> > > > > > On 2017/01/18 08:11:24, Yuki wrote:
> > > > > > > LGTM, but if you can keep a layout test non-reverted (with an
> updated
> > > test
> > > > > > > expectation of failures), that's great (otherwise, I need to
update
> > the
> > > > test
> > > > > > > again and I'm afraid of forgetting it).
> > > > > > 
> > > > > > Does it mean that we will regress the web-exposed behavior?
> > > > > 
> > > > > This revert itself regress a web-exposed behavior a little bit, but it
> has
> > > > been
> > > > > broken for a long time before the CL landed.  So, it's a kind of
> > acceptable,
> > > I
> > > > > think.
> > > > 
> > > > s/it has been broken/it had been broken/
> > > > 
> > > > Anyway, we don't support detached frames gracefully, I don't think this
> > revert
> > > > causes a big problem.
> > > 
> > > ok.
> > > 
> > > (But in general, it's not good to regress a web-exposed behavior because
> > > RemoteFrames are not well supported.)
> > 
> > I've updated the CL to avoid reverting the test changes (and check in a
> failing
> > expectation to reflect that we're diverging from correct behavior).
> > 
> > I do agree that it's not great to break web-exposed behavior, and I'm hoping
> to
> > fix this ASAP in a followup.
> 
> I might be behind but if you can fix it in a follow-up, would it be hard to
fix
> it before landing this CL?

So the problem is I want to land the remote context changes in M57 (which will
be cutin two days) so we can see how memory measurements are affected. I already
have the entire patch prepared, and testing locally, the detached window
bindings issue is the only blocker at this point. Note that the original CL
actually didn't make the M56 branch cut, so we won't be changing web-visible
behavior on the release branches (yet), just reverting it to the not-completely
working prior state.

The CL I eventually hope to land is https://codereview.chromium.org/2630693002/,
but there are some subtle tests around GC that I'm still trying to fix. The
problem is I'm not sure just much work is left to make all the edge cases work.

Basically, it's a tradeoff to get some memory savings in M57 now (and keep our
current incorrect behavior for an edge case that most sites shouldn't hit).

[haraken, 2017-01-18 08:57:08]

On 2017/01/18 08:55:50, dcheng wrote:
> On 2017/01/18 08:40:11, haraken wrote:
> > On 2017/01/18 08:36:02, dcheng wrote:
> > > On 2017/01/18 08:28:07, haraken wrote:
> > > > On 2017/01/18 08:16:05, Yuki wrote:
> > > > > On 2017/01/18 08:14:42, Yuki wrote:
> > > > > > On 2017/01/18 08:12:43, haraken wrote:
> > > > > > > On 2017/01/18 08:11:24, Yuki wrote:
> > > > > > > > LGTM, but if you can keep a layout test non-reverted (with an
> > updated
> > > > test
> > > > > > > > expectation of failures), that's great (otherwise, I need to
> update
> > > the
> > > > > test
> > > > > > > > again and I'm afraid of forgetting it).
> > > > > > > 
> > > > > > > Does it mean that we will regress the web-exposed behavior?
> > > > > > 
> > > > > > This revert itself regress a web-exposed behavior a little bit, but
it
> > has
> > > > > been
> > > > > > broken for a long time before the CL landed.  So, it's a kind of
> > > acceptable,
> > > > I
> > > > > > think.
> > > > > 
> > > > > s/it has been broken/it had been broken/
> > > > > 
> > > > > Anyway, we don't support detached frames gracefully, I don't think
this
> > > revert
> > > > > causes a big problem.
> > > > 
> > > > ok.
> > > > 
> > > > (But in general, it's not good to regress a web-exposed behavior because
> > > > RemoteFrames are not well supported.)
> > > 
> > > I've updated the CL to avoid reverting the test changes (and check in a
> > failing
> > > expectation to reflect that we're diverging from correct behavior).
> > > 
> > > I do agree that it's not great to break web-exposed behavior, and I'm
hoping
> > to
> > > fix this ASAP in a followup.
> > 
> > I might be behind but if you can fix it in a follow-up, would it be hard to
> fix
> > it before landing this CL?
> 
> So the problem is I want to land the remote context changes in M57 (which will
> be cutin two days) so we can see how memory measurements are affected. I
already
> have the entire patch prepared, and testing locally, the detached window
> bindings issue is the only blocker at this point. Note that the original CL
> actually didn't make the M56 branch cut, so we won't be changing web-visible
> behavior on the release branches (yet), just reverting it to the
not-completely
> working prior state.
> 
> The CL I eventually hope to land is
https://codereview.chromium.org/2630693002/,
> but there are some subtle tests around GC that I'm still trying to fix. The
> problem is I'm not sure just much work is left to make all the edge cases
work.
> 
> Basically, it's a tradeoff to get some memory savings in M57 now (and keep our
> current incorrect behavior for an edge case that most sites shouldn't hit).

Thanks for the clarification! LGTM.

[dcheng, 2017-01-18 08:58:14]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-01-18 08:58:15]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-18 08:58:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2643613003/#ps20001
(title: "Revert reverted test changes, add failing expectations")

[commit-bot: I haz the power, 2017-01-18 08:58:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 09:49:46]

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

[commit-bot: I haz the power, 2017-01-18 09:49:47]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-01-18 10:01:50]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-01-18 10:02:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 11:11:43]

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

[commit-bot: I haz the power, 2017-01-18 11:11:44]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-01-18 12:04:48]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 12:05:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-18 12:06:02]

PTAL. I've fixed the intersection observer test failure by adding a temporary
workaround in testharnessreport.js.

[Yuki, 2017-01-18 12:39:47]

On 2017/01/18 12:06:02, dcheng wrote:
> PTAL. I've fixed the intersection observer test failure by adding a temporary
> workaround in testharnessreport.js.

I'm uneasy to add a hack into testharnessreport.js.  Can we let intersection
observer test fail instead?

And anyway, I'd like to have TODO comments with this kind of hacks.

[commit-bot: I haz the power, 2017-01-18 13:59:26]

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

[commit-bot: I haz the power, 2017-01-18 13:59:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-01-18 18:21:07]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 18:21:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-18 18:24:57]

dcheng@chromium.org changed reviewers:
+ jsbell@chromium.org

[dcheng, 2017-01-18 18:24:57]

Note that testharnessreport.js is specifically provided to make it easy to
integrate testharness.js into the testing system. In other words, some amount of
custom code here is OK.

+jsbell, as a past reviewer on some other testharness changes, how do you feel
about this particular hack? We gained the dependency on this behavior in
https://codereview.chromium.org/2547653005, which happened to land shortly after
the patch that is now being reverted.

[jsbell, 2017-01-18 18:28:38]

On 2017/01/18 18:24:57, dcheng wrote:
> Note that testharnessreport.js is specifically provided to make it easy to
> integrate testharness.js into the testing system. In other words, some amount
of
> custom code here is OK.
> 
> +jsbell, as a past reviewer on some other testharness changes, how do you feel
> about this particular hack? We gained the dependency on this behavior in
> https://codereview.chromium.org/2547653005, which happened to land shortly
after
> the patch that is now being reverted.

lgtm - this seems fine.

[commit-bot: I haz the power, 2017-01-18 20:18:55]

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

[commit-bot: I haz the power, 2017-01-18 20:18:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-01-18 21:05:36]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-18 21:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2643613003/#ps60001
(title: "whack-a-mole")

[commit-bot: I haz the power, 2017-01-18 21:06:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 22:35:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484773536966810,
"parent_rev": "22d68c2c24b7c8a4d65802d2b385d09d50e575c9", "commit_rev":
"a3e6b34d4b686ee9bc9addda6885750964638c39"}

[commit-bot: I haz the power, 2017-01-18 22:36:09]

Description was changed from

==========
Revert "binding: Makes window/frames/self attributes return itself."

> Per HTML spec[1], window, frames, and self attributes must return
> the global proxy object.  This CL simply follows it.
>
> Note that these attributes are expected to never return null.
> We're currently breaking this assumption on detached frames.
>
> [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window

The current fix for this depends on a ScriptState being associated
with the window object. However, this prevents RemoteWindowProxy
from using v8::Context::NewRemoteContext(), as a RemoteWindowProxy
would then have no associated ScriptState.

BUG=618672,527190
R=haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
Revert "binding: Makes window/frames/self attributes return itself."

> Per HTML spec[1], window, frames, and self attributes must return
> the global proxy object.  This CL simply follows it.
>
> Note that these attributes are expected to never return null.
> We're currently breaking this assumption on detached frames.
>
> [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window

The current fix for this depends on a ScriptState being associated
with the window object. However, this prevents RemoteWindowProxy
from using v8::Context::NewRemoteContext(), as a RemoteWindowProxy
would then have no associated ScriptState.

BUG=618672,527190
R=haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2643613003
Cr-Commit-Position: refs/heads/master@{#444517}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3e6b34d4b686ee9bc9addda6885...
==========

[commit-bot: I haz the power, 2017-01-18 22:36:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a3e6b34d4b686ee9bc9addda6885...

[Yuki, 2017-01-19 06:07:10]

LGTM.  Sorry being too nervous.
Given the expert says okay, I'm fine.
And thanks for adding TODO comments.