Fix DOMWindow::isCurrentlyDisplayedInFrame to return false when detached

Fix DOMWindow::isCurrentlyDisplayedInFrame to return false when detached

Right now, this can return true even if the frame is detached, since it
doesn't check that the frame has a non-null host.

The remaining changes in DOMWindow are to preserve current web visible
behavior, as well as fixing a null pointer dereferences that this exposes
in Performance and ScreenOrientation.

BUG=347331
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168256

[dcheng, 2014-01-14 00:51:31]

Hi Adam,

[dcheng, 2014-01-14 00:51:32]

Hi Adam,

[dcheng, 2014-01-14 00:58:07]

Argh, hit enter too early.

I've been investigating this after my changes to DOMWindow/Document and
WebFrame/Frame lifetime to verify assumptions we used to hold about frame
visibility are still valid.

DOM window properties on detached/navigated DOM windows are a particularly hairy
area. Do we have a preferred model to follow here? Firefox is pretty arbitrary,
here's some samples from a run of BarProp-after-frame-removed-1.html:

FAIL childWindow.screen.height should be 0. Threw exception [Exception...
"Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
[nsIDOMWindow.screen]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location:
"JS frame ::
file:///usr/local/google/ssd/src/chrome/src/third_party/WebKit/LayoutTests/resources/js-test.js
:: shouldBe :: line 216" data: no]
PASS childWindow.locationbar.visible is false
FAIL childWindow.scrollbars.visible should be false. Was true.
FAIL childWindow.console.memory.totalJSHeapSize should be 0. Threw exception
TypeError: childWindow.console is undefined
WARN: shouldBe() expects string arguments

So you can see some properties are null (e.g. childWindow.performance,
childWindow.location) while others are present and set to presumably safe
defaults for a detached DOMWindow (childWindow.locationbar), and yet others
aren't reset on detach (childWindow.scrollbars).

Chrome currently makes sure all these properties are present but their values
are set to defaults for a detached DOMWindow (e.g. navigator.userAgent is blank,
location.href is about:blank, etc). There's also a crash bug that I suspect is a
UaF when you access some of the performance properties...

What should we do here? Do the changes I've made so far in DOMWindow make sense?
While it doesn't really ever make sense to service these in a meaningful way, we
still have to handle the case where m_frame gets reset to 0 anyway, just in case
a page retains a reference to a window.scrollbars object (for an example of
this, see BarProp-after-frame-navigated-2.html).

[abarth-chromium, 2014-01-14 04:10:52]

On 2014/01/14 00:58:07, dcheng wrote:
> DOM window properties on detached/navigated DOM windows are a particularly
hairy
> area.

Yes, they are.  :)

> Do we have a preferred model to follow here? Firefox is pretty arbitrary,
> here's some samples from a run of BarProp-after-frame-removed-1.html:

I'd prefer to keep our current behavior exactly unless there's a reason to
change it.

> So you can see some properties are null (e.g. childWindow.performance,
> childWindow.location) while others are present and set to presumably safe
> defaults for a detached DOMWindow (childWindow.locationbar), and yet others
> aren't reset on detach (childWindow.scrollbars).

Yes, it's a crossroads of insanity.

> What should we do here?

Is there any reason to change our current behavior?  I'm unsure what problem
you're trying to solve.

> Do the changes I've made so far in DOMWindow make sense?

I'd rather return 0 than return objects after we've been detached.  It's harder
to get in trouble that way.

> While it doesn't really ever make sense to service these in a meaningful way,
we
> still have to handle the case where m_frame gets reset to 0 anyway, just in
case
> a page retains a reference to a window.scrollbars object (for an example of
> this, see BarProp-after-frame-navigated-2.html).

I still don't understand what problem you're trying to solve.

[dcheng, 2014-01-14 04:29:33]

Sorry I'm going to reply to your comments out of order so my reply hopefully
makes more sense.

On 2014/01/14 04:10:52, abarth wrote:
> I still don't understand what problem you're trying to solve.

My original goal was to make DOMWindow::isCurrentlyDisplayedInFrame actually do
what it says it does. Right now, you can have detached frames and this function
will still happily return true for them. As far as I can tell, this has always
been the case. I think it's reasonable to define this to return false for
DOMWindows associated with detached Frames.

> I'd prefer to keep our current behavior exactly unless there's a reason to
> change it.
> I'd rather return 0 than return objects after we've been detached.  It's
harder
> to get in trouble that way.

These two statements are somewhat contradictory. The original test I removed in
this patch demonstrates that the original behavior of WebKit/Blink is to *not*
return 0 in this case, since window.toolbar is non-null and accessible after the
iframe is removed from the DOM. Are you referring to detach in another sense?

Also, as I mentioned, it doesn't matter if we return 0 in this case. Things that
implement DOMWindowProperty *must* handle the detached case even if we do this.
Simple example:

var f = document.getElementById('myFrame');
var t = f.contentWindow.toolbars;
f.parentNode.removeChild(f);
t.visible; // Must do something sensible even though f is detached.

Thus, I think it's simpler to just force DOMWindowProperty implementations to
recognize this fact instead of having a null check in the property getter and
then null checks in all the property implementation methods.

> Is there any reason to change our current behavior?  I'm unsure what problem
you're trying to solve.

Our current behavior crashes the renderer =)

[abarth-chromium, 2014-01-14 05:46:29]

On 2014/01/14 04:29:33, dcheng wrote:
> On 2014/01/14 04:10:52, abarth wrote:
> > I still don't understand what problem you're trying to solve.
> 
> My original goal was to make DOMWindow::isCurrentlyDisplayedInFrame actually
do
> what it says it does. Right now, you can have detached frames and this
function
> will still happily return true for them. As far as I can tell, this has always
> been the case. I think it's reasonable to define this to return false for
> DOMWindows associated with detached Frames.

That seems like a worthwhile goal.

> > I'd prefer to keep our current behavior exactly unless there's a reason to
> > change it.
> > I'd rather return 0 than return objects after we've been detached.  It's
> harder
> > to get in trouble that way.
> 
> These two statements are somewhat contradictory. The original test I removed
in
> this patch demonstrates that the original behavior of WebKit/Blink is to *not*
> return 0 in this case, since window.toolbar is non-null and accessible after
the
> iframe is removed from the DOM. Are you referring to detach in another sense?

Oh, I was just reading your CL and saw that you were removing a bunch of "return
0" statements.  We generally prefer to return 0 in these cases if compatibility
permits it.

> Also, as I mentioned, it doesn't matter if we return 0 in this case. Things
that
> implement DOMWindowProperty *must* handle the detached case even if we do
this.

Yes, I understand that.

> Thus, I think it's simpler to just force DOMWindowProperty implementations to
> recognize this fact instead of having a null check in the property getter and
> then null checks in all the property implementation methods.

I don't follow this step in your argument.

> > Is there any reason to change our current behavior?  I'm unsure what problem
> you're trying to solve.
> 
> Our current behavior crashes the renderer =)

I'm certainly in favor of not crashing.  However, I don't think we should change
any other behavior in this area without a reason.

[dcheng, 2014-01-14 18:04:26]

On 2014/01/14 05:46:29, abarth wrote:
> On 2014/01/14 04:29:33, dcheng wrote:
> > On 2014/01/14 04:10:52, abarth wrote:
> > > I still don't understand what problem you're trying to solve.
> > 
> > My original goal was to make DOMWindow::isCurrentlyDisplayedInFrame actually
> do
> > what it says it does. Right now, you can have detached frames and this
> function
> > will still happily return true for them. As far as I can tell, this has
always
> > been the case. I think it's reasonable to define this to return false for
> > DOMWindows associated with detached Frames.
> 
> That seems like a worthwhile goal.
> 
> > > I'd prefer to keep our current behavior exactly unless there's a reason to
> > > change it.
> > > I'd rather return 0 than return objects after we've been detached.  It's
> > harder
> > > to get in trouble that way.
> > 
> > These two statements are somewhat contradictory. The original test I removed
> in
> > this patch demonstrates that the original behavior of WebKit/Blink is to
*not*
> > return 0 in this case, since window.toolbar is non-null and accessible after
> the
> > iframe is removed from the DOM. Are you referring to detach in another
sense?
> 
> Oh, I was just reading your CL and saw that you were removing a bunch of
"return
> 0" statements.  We generally prefer to return 0 in these cases if
compatibility
> permits it.
> 
> > Also, as I mentioned, it doesn't matter if we return 0 in this case. Things
> that
> > implement DOMWindowProperty *must* handle the detached case even if we do
> this.
> 
> Yes, I understand that.
> 
> > Thus, I think it's simpler to just force DOMWindowProperty implementations
to
> > recognize this fact instead of having a null check in the property getter
and
> > then null checks in all the property implementation methods.
> 
> I don't follow this step in your argument.
> 
> > > Is there any reason to change our current behavior?  I'm unsure what
problem
> > you're trying to solve.
> > 
> > Our current behavior crashes the renderer =)
> 
> I'm certainly in favor of not crashing.  However, I don't think we should
change
> any other behavior in this area without a reason.

If we fix "isCurrentlyDisplayedInFrame" to not return true for detached frames,
we have to get rid of the early return 0 in the property getters. Otherwise,
there is a web visible change. A simple example is
BarProp-after-frame-removed-1.html changes from
detachedChildWindow.toolbar.visible returning false to
detachedChildWindow.toolbar being null. So either we change our behavior for
detached frames and don't change the C++ outside of
isCurrentlyDisplayedInFrame... or we change the C++ implementation to keep the
web behavior the same. As far as I can tell, we can't have both though.

[abarth-chromium, 2014-01-14 18:15:11]

On 2014/01/14 18:04:26, dcheng wrote:
> If we fix "isCurrentlyDisplayedInFrame" to not return true for detached
frames,
> we have to get rid of the early return 0 in the property getters. Otherwise,
> there is a web visible change.

That's fine.  I'm happy changing the code as long as the web-visible behavior
remains the same.

> or we change the C++ implementation to keep the web behavior the same.

^^^ Sounds good.

[dcheng, 2014-02-28 23:55:55]

PTAL. In the long term, I'd like to fix this so that DOMWindow does clear its
pointer back to m_frame and all properties rooted off DOMWindow implement
DOMWindowProperty, etc, but I think this is the best interim solution.

[abarth-chromium, 2014-03-01 07:00:31]

https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (left):

https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:609: return 0;
Why doesn't this change web-visible behavior?  Previously, this property would
return |null| on detached DOMWindows.  Now it returns something non-null....

[dcheng, 2014-03-01 09:41:53]

https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (left):

https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:609: return 0;
On 2014/03/01 07:00:32, abarth wrote:
> Why doesn't this change web-visible behavior?  Previously, this property would
> return |null| on detached DOMWindows.  Now it returns something non-null....

It doesn't change web visible behavior because the check was busted. It would
return true for detached frames when the Frame hadn't yet been destroyed, since
m_frame && m_frame->domWindow() == this would both be true in that case.

[abarth-chromium, 2014-03-01 09:55:29]

I see.  We broke the check earlier and no one noticed.  Ok.  LGTM

[dcheng, 2014-03-01 18:28:55]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-03-01 18:29:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/131113003/300001

[commit-bot: I haz the power, 2014-03-01 19:47:35]

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

[commit-bot: I haz the power, 2014-03-01 19:47:36]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[dcheng, 2014-03-02 19:47:19]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-03-02 19:47:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/131113003/300001

[commit-bot: I haz the power, 2014-03-02 21:19:09]

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

[commit-bot: I haz the power, 2014-03-02 21:19:09]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[dcheng, 2014-03-03 00:01:03]

Committed patchset #5 manually as r168256 (presubmit successful).