Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(270)

Issue 1421113006: Detach the globals of all frames, not just the main frame (Closed)

Created:
5 years, 1 month ago by jochen (gone - plz use gerrit)
Modified:
4 years, 7 months ago
Reviewers:
haraken, dcheng
CC:
blink-reviews, chromium-reviews, dcheng, mlamouri+watch-blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Detach the globals of all frames, not just the main frame No idea why we didn't do that before R=haraken@chromium.org BUG=none

Patch Set 1 #

Total comments: 1

Patch Set 2 : git cl try #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -43 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/ScriptController.h View 1 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp View 1 2 chunks +2 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.h View 1 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp View 1 3 chunks +3 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h View 1 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp View 1 1 chunk +3 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.cpp View 1 2 chunks +2 lines, -0 lines 1 comment Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/web/WebFrame.cpp View 1 2 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 9 (1 generated)
jochen (gone - plz use gerrit)
5 years, 1 month ago (2015-10-30 12:58:04 UTC) #1
haraken
LGTM but I want to have dcheng confirm this. Also maybe can we remove WindowProxy::clearForClose() ...
5 years, 1 month ago (2015-10-30 13:03:35 UTC) #3
jochen (gone - plz use gerrit)
ptal
5 years, 1 month ago (2015-10-30 13:47:45 UTC) #4
haraken
LGTM
5 years, 1 month ago (2015-10-30 13:52:55 UTC) #5
dcheng
lgtm!
5 years, 1 month ago (2015-10-30 15:49:06 UTC) #6
dcheng
https://codereview.chromium.org/1421113006/diff/20001/third_party/WebKit/Source/core/frame/Frame.cpp File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/1421113006/diff/20001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode94 third_party/WebKit/Source/core/frame/Frame.cpp:94: windowProxyManager()->clearWindowProxy(); Is there a reason to call this again ...
5 years, 1 month ago (2015-10-30 15:49:47 UTC) #7
jochen (gone - plz use gerrit)
well, well, guess I need to run those tests in Firefox and see what it ...
5 years, 1 month ago (2015-11-02 19:02:53 UTC) #8
haraken
4 years, 7 months ago (2016-05-09 09:06:22 UTC) #9
Message was sent while issue was closed.
I looked into the failing tests and investigated the behavior of Firefox, Safari
and IE.

Basically this CL changes the behavior of the following code:

<script>
onload = function() {
  var iframe = document.createElement("iframe");
  document.body.appendChild(iframe);
  var win = iframe.contentWindow;
  win.foo = "aaa";
  document.body.removeChild(iframe);
  console.log(win.foo);
}
</script>

Currently Chrome and Firefox return "aaa", and Firefox and IE return undefined.

Once we land this CL, all window.xxx accesses on the detached iframe start
throwing exceptions. So Chrome throws an exception for win.foo.

I'm not sure if this behavioral change is expected or not -- started a thread on
platform-architecture-dev@:
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/6I...

Powered by Google App Engine
This is Rietveld 408576698