Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 131113003: Fix DOMWindow::isCurrentlyDisplayedInFrame to return false when detached (Closed)

Created:
7 years ago by dcheng
Modified:
6 years, 10 months ago
Reviewers:
abarth-chromium
CC:
blink-reviews, eseidel
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : Fix ScreenOrientation + test window.open. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+503 lines, -77 lines) Patch
D LayoutTests/fast/dom/Window/BarInfo-after-frame-removed.html View 1 chunk +0 lines, -35 lines 0 comments Download
D LayoutTests/fast/dom/Window/BarInfo-after-frame-removed-expected.txt View 1 chunk +0 lines, -3 lines 0 comments Download
A LayoutTests/fast/dom/Window/open-after-frame-detached.html View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/open-after-frame-detached-expected.txt View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html View 1 2 3 4 1 chunk +43 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt View 1 2 3 4 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-expected.txt View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html View 1 2 3 1 chunk +38 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt View 1 2 3 4 1 chunk +62 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed.html View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt View 1 2 3 4 1 chunk +57 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/Window/resources/window-property-collector.js View 1 2 3 4 1 chunk +88 lines, -0 lines 0 comments Download
M Source/core/frame/BarProp.cpp View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/frame/DOMWindow.cpp View 1 2 3 4 15 chunks +2 lines, -31 lines 2 comments Download
M Source/core/timing/Performance.cpp View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M Source/modules/screen_orientation/ScreenOrientation.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/screen_orientation/ScreenOrientation.cpp View 1 2 3 4 2 chunks +7 lines, -4 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
dcheng
Hi Adam,
7 years ago (2014-01-14 00:51:31 UTC) #1
dcheng
Hi Adam,
7 years ago (2014-01-14 00:51:32 UTC) #2
dcheng
Argh, hit enter too early. I've been investigating this after my changes to DOMWindow/Document and ...
7 years ago (2014-01-14 00:58:07 UTC) #3
abarth-chromium
On 2014/01/14 00:58:07, dcheng wrote: > DOM window properties on detached/navigated DOM windows are a ...
7 years ago (2014-01-14 04:10:52 UTC) #4
dcheng
Sorry I'm going to reply to your comments out of order so my reply hopefully ...
7 years ago (2014-01-14 04:29:33 UTC) #5
abarth-chromium
On 2014/01/14 04:29:33, dcheng wrote: > On 2014/01/14 04:10:52, abarth wrote: > > I still ...
7 years ago (2014-01-14 05:46:29 UTC) #6
dcheng
On 2014/01/14 05:46:29, abarth wrote: > On 2014/01/14 04:29:33, dcheng wrote: > > On 2014/01/14 ...
7 years ago (2014-01-14 18:04:26 UTC) #7
abarth-chromium
On 2014/01/14 18:04:26, dcheng wrote: > If we fix "isCurrentlyDisplayedInFrame" to not return true for ...
7 years ago (2014-01-14 18:15:11 UTC) #8
dcheng
PTAL. In the long term, I'd like to fix this so that DOMWindow does clear ...
6 years, 11 months ago (2014-02-28 23:55:55 UTC) #9
abarth-chromium
https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWindow.cpp#oldcode609 Source/core/frame/DOMWindow.cpp:609: return 0; Why doesn't this change web-visible behavior? Previously, ...
6 years, 11 months ago (2014-03-01 07:00:31 UTC) #10
dcheng
https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/131113003/diff/300001/Source/core/frame/DOMWindow.cpp#oldcode609 Source/core/frame/DOMWindow.cpp:609: return 0; On 2014/03/01 07:00:32, abarth wrote: > Why ...
6 years, 11 months ago (2014-03-01 09:41:53 UTC) #11
abarth-chromium
I see. We broke the check earlier and no one noticed. Ok. LGTM
6 years, 11 months ago (2014-03-01 09:55:29 UTC) #12
dcheng
The CQ bit was checked by dcheng@chromium.org
6 years, 11 months ago (2014-03-01 18:28:55 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcheng@chromium.org/131113003/300001
6 years, 11 months ago (2014-03-01 18:29:01 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 11 months ago (2014-03-01 19:47:35 UTC) #15
commit-bot: I haz the power
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout&number=24149
6 years, 11 months ago (2014-03-01 19:47:36 UTC) #16
dcheng
The CQ bit was checked by dcheng@chromium.org
6 years, 10 months ago (2014-03-02 19:47:19 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcheng@chromium.org/131113003/300001
6 years, 10 months ago (2014-03-02 19:47:35 UTC) #18
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-03-02 21:19:09 UTC) #19
commit-bot: I haz the power
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_rel&number=20645
6 years, 10 months ago (2014-03-02 21:19:09 UTC) #20
dcheng
6 years, 10 months ago (2014-03-03 00:01:03 UTC) #21
Message was sent while issue was closed.
Committed patchset #5 manually as r168256 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698