Have window.closed return true when frame is closed.

Have window.closed return true when frame is closed.

This Window property currently returns true once the Frame it is attached
to is destroyed, which exposes an object memory management detail that's
hard to emulate with Oilpan.

Switch instead to having "closed" return true once the frame is externally
closed or the (i)frame is removed&detached.

Adjust tests to check for this instead -- no indication that this slight
adjustment in behavior will pose a compatibility problem.

R=haraken,ager,mkwst,tkent
BUG=414658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182148

[sof, 2014-09-16 16:42:28]

sigbjornf@opera.com changed reviewers:
+ ager@chromium.org, dcheng@chromium.org, haraken@chromium.org

[sof, 2014-09-16 16:42:29]

Please take a look.

[haraken, 2014-09-17 00:02:19]

How do other browsers behave?

[sof, 2014-09-17 07:36:16]

On 2014/09/17 00:02:19, haraken(OOO til Sep 21) wrote:
> How do other browsers behave?

Sorry, didn't prepare that data right away. See https://crbug.com/414658#c1

[haraken, 2014-09-17 07:40:00]

On 2014/09/17 07:36:16, sof wrote:
> On 2014/09/17 00:02:19, haraken(OOO til Sep 21) wrote:
> > How do other browsers behave?
> 
> Sorry, didn't prepare that data right away. See https://crbug.com/414658#c1

Thanks, I'm fine with the behavior change (it aligns with Firefox), but others
might have different thoughts.

[Mads Ager (chromium), 2014-09-17 07:47:39]

LGTM

I think the behavior change is a good thing here. No reason to tie window.closed
to the actual destruction of the frame which will make V8 (and later blink) GCs
observable. GCs should not be observable in the APIs if it can be avoided.

https://codereview.chromium.org/538323003/diff/1/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/538323003/diff/1/Source/core/frame/Frame.h#ne...
Source/core/frame/Frame.h:120: // other RefPtrs to the Frame, up the stack or
elsewhere. For Oilpan,
Remove the Oilpan reference in this patch since Frame is still not in the Oilpan
heap here?

[Mike West, 2014-09-17 07:49:38]

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

[Mike West, 2014-09-17 07:49:39]

LGTM2, for what it's worth.

[sof, 2014-09-17 08:08:28]

Cc +abarth: fyi

Thanks for the reviews.

ager wrote:
> I think the behavior change is a good thing here. No reason to tie
window.closed
> to the actual destruction of the frame which will make V8 (and later blink)
GCs
> observable. GCs should not be observable in the APIs if it can be avoided.

Yes, along with the data on how others behave, I think we come out ahead by
doing this.

This is corner case behavior, but it'd be preferable if that corner case was
stable across GCs..which it will be now. Will land.

https://codereview.chromium.org/538323003/diff/1/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/538323003/diff/1/Source/core/frame/Frame.h#ne...
Source/core/frame/Frame.h:120: // other RefPtrs to the Frame, up the stack or
elsewhere. For Oilpan,
On 2014/09/17 07:47:39, Mads Ager (chromium) wrote:
> Remove the Oilpan reference in this patch since Frame is still not in the
Oilpan
> heap here?

Thanks, that would premature to mention. Removed.

[sof, 2014-09-17 08:10:37]

sigbjornf@opera.com changed reviewers:
+ jochen@chromium.org, tkent@chromium.org

[sof, 2014-09-17 08:10:37]

tkent, jochen: when you next have some "platform owner" cycles available, could
you take a look at the Source/web/ changes? tia

[tkent, 2014-09-17 08:28:46]

The behavior change sounds reasonable.  lgtm.

https://codereview.chromium.org/538323003/diff/20001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/538323003/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.h:102: void setHasBeenClosed() {
ASSERT(!m_hasBeenClosed); m_hasBeenClosed = true; }
nit: We don't put multiple statements in a single line.

[sof, 2014-09-17 09:55:05]

https://codereview.chromium.org/538323003/diff/20001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/538323003/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.h:102: void setHasBeenClosed() {
ASSERT(!m_hasBeenClosed); m_hasBeenClosed = true; }
On 2014/09/17 08:28:46, tkent (overloaded) wrote:
> nit: We don't put multiple statements in a single line.

Thanks, addressed (&un-inlined.)

[sof, 2014-09-17 09:55:21]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-17 09:56:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/538323003/40001

[dcheng, 2014-09-17 10:56:23]

Why do we need a new boolean for this? How come we can't do something like check
that Frame::host() is not null?

[dcheng, 2014-09-17 10:58:45]

not lgtm, since I don't think we should be adding another bit of parallel state.

[commit-bot: I haz the power, 2014-09-17 11:02:50]

Committed patchset #3 (id:40001) as 182148

[sof, 2014-09-17 11:06:58]

On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> Why do we need a new boolean for this? How come we can't do something like
check
> that Frame::host() is not null?

m_host wouldn't be set to null upon calling close(), or would it?

[dcheng, 2014-09-17 11:09:56]

On 2014/09/17 at 11:06:58, sigbjornf wrote:
> On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > Why do we need a new boolean for this? How come we can't do something like
check
> > that Frame::host() is not null?
> 
> m_host wouldn't be set to null upon calling close(), or would it?

close() is called by the embedder as one of the final steps of frame detach.
m_host is nulled out as part of frame detach.

[sof, 2014-09-17 11:12:24]

On 2014/09/17 11:09:56, dcheng (OOO) wrote:
> On 2014/09/17 at 11:06:58, sigbjornf wrote:
> > On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > > Why do we need a new boolean for this? How come we can't do something like
> check
> > > that Frame::host() is not null?
> > 
> > m_host wouldn't be set to null upon calling close(), or would it?
> 
> close() is called by the embedder as one of the final steps of frame detach.
> m_host is nulled out as part of frame detach.

So window.closed should become true upon detaching instead?

[dcheng, 2014-09-17 11:20:35]

On 2014/09/17 at 11:12:24, sigbjornf wrote:
> On 2014/09/17 11:09:56, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:06:58, sigbjornf wrote:
> > > On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > > > Why do we need a new boolean for this? How come we can't do something
like
> > check
> > > > that Frame::host() is not null?
> > > 
> > > m_host wouldn't be set to null upon calling close(), or would it?
> > 
> > close() is called by the embedder as one of the final steps of frame detach.
> > m_host is nulled out as part of frame detach.
> 
> So window.closed should become true upon detaching instead?

That's essentially what this patch is--I just don't think we actually need the
bool. WebFrame::close() is somewhat of a misleading name in this context. When
we refactored WebFrame, we didn't want to expose the refcounted nature to the
embedder. So instead, the embedder pairs the concepts create() and close()--if
it creates()s a WebFrame or WebView, it must close() it or else it will leak.

When a frame is being detached for any reason (window.close(), iframe detached,
webview closed), it goes through Frame::detach(). Blink finishes all its cleanup
(which includes setting Frame::m_host to null) before signaling to the embedder
that the frame is detached via WebFrameClient::frameDetached(). The embedder
then performs its cleanup, and the as the final step, calls WebFrame::close() to
match the original call to WebFrame::create().

[sof, 2014-09-17 11:24:30]

On 2014/09/17 11:20:35, dcheng (OOO) wrote:
> On 2014/09/17 at 11:12:24, sigbjornf wrote:
> > On 2014/09/17 11:09:56, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:06:58, sigbjornf wrote:
> > > > On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > > > > Why do we need a new boolean for this? How come we can't do something
> like
> > > check
> > > > > that Frame::host() is not null?
> > > > 
> > > > m_host wouldn't be set to null upon calling close(), or would it?
> > > 
> > > close() is called by the embedder as one of the final steps of frame
detach.
> > > m_host is nulled out as part of frame detach.
> > 
> > So window.closed should become true upon detaching instead?
> 
> That's essentially what this patch is--I just don't think we actually need the
> bool. WebFrame::close() is somewhat of a misleading name in this context. When
> we refactored WebFrame, we didn't want to expose the refcounted nature to the
> embedder. So instead, the embedder pairs the concepts create() and close()--if
> it creates()s a WebFrame or WebView, it must close() it or else it will leak.
> 
> When a frame is being detached for any reason (window.close(), iframe
detached,
> webview closed), it goes through Frame::detach(). Blink finishes all its
cleanup
> (which includes setting Frame::m_host to null) before signaling to the
embedder
> that the frame is detached via WebFrameClient::frameDetached(). The embedder
> then performs its cleanup, and the as the final step, calls WebFrame::close()
to
> match the original call to WebFrame::create().

We can certainly try to simplify it along those lines; it would be for the
better if it works out.

[sof, 2014-09-17 11:42:04]

On 2014/09/17 11:24:30, sof wrote:
> On 2014/09/17 11:20:35, dcheng (OOO) wrote:
> > On 2014/09/17 at 11:12:24, sigbjornf wrote:
> > > On 2014/09/17 11:09:56, dcheng (OOO) wrote:
> > > > On 2014/09/17 at 11:06:58, sigbjornf wrote:
> > > > > On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > > > > > Why do we need a new boolean for this? How come we can't do
something
> > like
> > > > check
> > > > > > that Frame::host() is not null?
> > > > > 
> > > > > m_host wouldn't be set to null upon calling close(), or would it?
> > > > 
> > > > close() is called by the embedder as one of the final steps of frame
> detach.
> > > > m_host is nulled out as part of frame detach.
> > > 
> > > So window.closed should become true upon detaching instead?
> > 
> > That's essentially what this patch is--I just don't think we actually need
the
> > bool. WebFrame::close() is somewhat of a misleading name in this context.
When
> > we refactored WebFrame, we didn't want to expose the refcounted nature to
the
> > embedder. So instead, the embedder pairs the concepts create() and
close()--if
> > it creates()s a WebFrame or WebView, it must close() it or else it will
leak.
> > 
> > When a frame is being detached for any reason (window.close(), iframe
> detached,
> > webview closed), it goes through Frame::detach(). Blink finishes all its
> cleanup
> > (which includes setting Frame::m_host to null) before signaling to the
> embedder
> > that the frame is detached via WebFrameClient::frameDetached(). The embedder
> > then performs its cleanup, and the as the final step, calls
WebFrame::close()
> to
> > match the original call to WebFrame::create().
> 
> We can certainly try to simplify it along those lines; it would be for the
> better if it works out.

Looks encouraging, preparing a follow up CL. Good stuff, thanks for pointing
this out.

[sof, 2014-09-17 13:47:58]

On 2014/09/17 11:42:04, sof wrote:
> On 2014/09/17 11:24:30, sof wrote:
> > On 2014/09/17 11:20:35, dcheng (OOO) wrote:
> > > On 2014/09/17 at 11:12:24, sigbjornf wrote:
> > > > On 2014/09/17 11:09:56, dcheng (OOO) wrote:
> > > > > On 2014/09/17 at 11:06:58, sigbjornf wrote:
> > > > > > On 2014/09/17 10:56:23, dcheng (OOO) wrote:
> > > > > > > Why do we need a new boolean for this? How come we can't do
> something
> > > like
> > > > > check
> > > > > > > that Frame::host() is not null?
> > > > > > 
> > > > > > m_host wouldn't be set to null upon calling close(), or would it?
> > > > > 
> > > > > close() is called by the embedder as one of the final steps of frame
> > detach.
> > > > > m_host is nulled out as part of frame detach.
> > > > 
> > > > So window.closed should become true upon detaching instead?
> > > 
> > > That's essentially what this patch is--I just don't think we actually need
> the
> > > bool. WebFrame::close() is somewhat of a misleading name in this context.
> When
> > > we refactored WebFrame, we didn't want to expose the refcounted nature to
> the
> > > embedder. So instead, the embedder pairs the concepts create() and
> close()--if
> > > it creates()s a WebFrame or WebView, it must close() it or else it will
> leak.
> > > 
> > > When a frame is being detached for any reason (window.close(), iframe
> > detached,
> > > webview closed), it goes through Frame::detach(). Blink finishes all its
> > cleanup
> > > (which includes setting Frame::m_host to null) before signaling to the
> > embedder
> > > that the frame is detached via WebFrameClient::frameDetached(). The
embedder
> > > then performs its cleanup, and the as the final step, calls
> WebFrame::close()
> > to
> > > match the original call to WebFrame::create().
> > 
> > We can certainly try to simplify it along those lines; it would be for the
> > better if it works out.
> 
> Looks encouraging, preparing a follow up CL. Good stuff, thanks for pointing
> this out.

For cross-reference, https://codereview.chromium.org/544013005/