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

Issue 908043002: Revert of Issue FrameDestructionObserver::frameDestroyed() notification on detach. (Closed)

Created:
5 years, 10 months ago by sof
Modified:
5 years, 10 months ago
CC:
blink-reviews, Lei Zhang
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Revert of Issue FrameDestructionObserver::frameDestroyed() notification on detach. (patchset #5 id:80001 of https://codereview.chromium.org/881683003/) Reason for revert: Linux ChromiumOS Tests (dbg) bot is timing out on ScreenOrientationBrowserTest.CrashTest_UseAfterDetach, http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/6186 Can't reproduce locally, but will revert & look into it tomorrow. Original issue's description: > Issue FrameDestructionObserver::frameDestroyed() notification on detach. > > Send this notification during detach instead of waiting until finalization, > which will typically proceed shortly afterwards (non-Oilpan.) The > supplements of the LocalFrame are cleared at the same time so as to > prevent FrameDestructionObservers that are also supplements from being > used in their detached state. > > Doing so makes the observable lifetimes for FrameDestructionObserver's > frame() identical, with and without Oilpan. Not having GC/finalization > of frames be observable to scripts is preferable. > > For LocalDOMWindow, a FrameDestructionObserver, its set of registered > DOMWindowProperty objects are also detached on frameDestroyed(), > clearing their frame references at the same time. > > R=haraken,dcheng > BUG=446452 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189837 TBR=dcheng@chromium.org,haraken@chromium.org,japhet@chromium.org,oilpan-reviews@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=446452 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189845

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -79 lines) Patch
M LayoutTests/OilpanExpectations View 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-expected.txt View 1 chunk +9 lines, -9 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt View 4 chunks +22 lines, -22 lines 0 comments Download
M Source/core/frame/DOMWindowProperty.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/frame/FrameDestructionObserver.h View 2 chunks +9 lines, -1 line 0 comments Download
M Source/core/frame/FrameDestructionObserver.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/frame/LocalDOMWindow.h View 2 chunks +0 lines, -2 lines 0 comments Download
M Source/core/frame/LocalDOMWindow.cpp View 2 chunks +23 lines, -34 lines 0 comments Download
M Source/core/frame/LocalFrame.cpp View 3 chunks +10 lines, -10 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
sof
Created Revert of Issue FrameDestructionObserver::frameDestroyed() notification on detach.
5 years, 10 months ago (2015-02-09 22:01:34 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908043002/1
5 years, 10 months ago (2015-02-09 22:02:25 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=189845
5 years, 10 months ago (2015-02-09 22:03:03 UTC) #3
haraken
LGTM
5 years, 10 months ago (2015-02-10 01:07:30 UTC) #4
Lei Zhang
FWIW, Google Chrome has some internal canary bots that builds against Blink ToT, and they ...
5 years, 10 months ago (2015-02-10 01:22:38 UTC) #5
sof
5 years, 10 months ago (2015-02-10 08:33:34 UTC) #6
Message was sent while issue was closed.
On 2015/02/10 01:22:38, Lei Zhang wrote:
> FWIW, Google Chrome has some internal canary bots that builds against Blink
ToT,
> and they all hit this problem. So in theory you should be able to repro this
on
> any of the desktop platforms.

Thanks, my apologies for breaking them yet again.

Have now repro'ed the timeout locally; test being adjusted via
https://codereview.chromium.org/909043002/

Powered by Google App Engine
This is Rietveld 408576698