Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
881683003:
Issue FrameDestructionObserver::frameDestroyed() notification on detach. (Closed)
DescriptionIssue 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=189906
Patch Set 1 #Patch Set 2 : inline temporary helper, tidy #
Total comments: 5
Patch Set 3 : Detach DOMWindowProperty objects also + supplements #Patch Set 4 : Clear out destruction observers #
Total comments: 2
Patch Set 5 : weak references no longer needed #Messages
Total messages: 40 (9 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look. This seems workable and the preferable option (issue frameDestroyed at the same time, Oilpan and not.)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... File LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt (right): https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: FAIL childWindow.speechSynthesis.speaking should be false. Threw exception TypeError: Cannot read property 'speaking' of null Does the new behavior align with the spec and other browsers? If so, we might want to update the test so that the result becomes PASS. https://codereview.chromium.org/881683003/diff/20001/Source/core/frame/LocalF... File Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/881683003/diff/20001/Source/core/frame/LocalF... Source/core/frame/LocalFrame.cpp:290: frameDestructionObserver->frameDestroyed(); Nit: In a follow-up CL, we might want to replace FrameDestructionObserver with LifecycleObserver.
haraken@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... File LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt (right): https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: FAIL childWindow.speechSynthesis.speaking should be false. Threw exception TypeError: Cannot read property 'speaking' of null On 2015/01/31 at 01:30:01, haraken wrote: > Does the new behavior align with the spec and other browsers? > > If so, we might want to update the test so that the result becomes PASS. I don't think this area is specced out well. I have a small test page, and in Firefox, window.location on a detached window returns null. So there's some precedent for making these inaccessible on frame detach. However, the behavior across different properties is not consistent. For example, window.name seems to persist on a detached window. I have a long-term goal to make Blink clean up the DOMWindow::m_frame back pointer more aggressively (on DOMWindow detach, rather than on DOMWindow destruction). This implies more things will be null/undefined after the window is detached, but I haven't gotten around to sending an email to whatwg@ to see if this is feasible or desirable.
https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... File LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt (right): https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: FAIL childWindow.speechSynthesis.speaking should be false. Threw exception TypeError: Cannot read property 'speaking' of null On 2015/01/31 01:30:01, haraken wrote: > > Does the new behavior align with the spec and other browsers? > > If so, we might want to update the test so that the result becomes PASS. The lack of "childWindow.location" here is due to the binding security check for Frames; it returning false when there being no frame. All else being equal, it would be preferable to keep ".location" accessible, but I'm not sure we want to tweak that security check. fwiw, the location behavior now aligns with Gecko (but not Presto nor Trident), but this is less-than-well-defined terrain spec-wise.
https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... File LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt (right): https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: FAIL childWindow.speechSynthesis.speaking should be false. Threw exception TypeError: Cannot read property 'speaking' of null On 2015/01/31 01:30:01, haraken wrote: > > Does the new behavior align with the spec and other browsers? > > If so, we might want to update the test so that the result becomes PASS. Also, the use of "FAIL" for these property-collector tests is a bit non-standard (at least that's how I think of the FAIL usage here) -- they're in the sense of "not the same value as before a frame/window operation" rather than "globally&inherently wrong". cf. expected applicationCache results for this particular variant.
On 2015/01/31 at 08:13:55, sigbjornf wrote: > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > File LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt (right): > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: FAIL childWindow.speechSynthesis.speaking should be false. Threw exception TypeError: Cannot read property 'speaking' of null > On 2015/01/31 01:30:01, haraken wrote: > > > > Does the new behavior align with the spec and other browsers? > > > > If so, we might want to update the test so that the result becomes PASS. > > Also, the use of "FAIL" for these property-collector tests is a bit non-standard (at least that's how I think of the FAIL usage here) -- they're in the sense of "not the same value as before a frame/window operation" rather than "globally&inherently wrong". > > cf. expected applicationCache results for this particular variant. The use of FAIL here is intentional. abarth@, japhet@, myself, and probably others, have wanted to make this behavior more consistent for detached windows- and in general, that means nulling, zeroing, or otherwise wiping the fields of a detached window.
On 2015/01/31 09:02:41, dcheng wrote: > On 2015/01/31 at 08:13:55, sigbjornf wrote: > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > File > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt > (right): > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: > FAIL childWindow.speechSynthesis.speaking should be false. Threw exception > TypeError: Cannot read property 'speaking' of null > > On 2015/01/31 01:30:01, haraken wrote: > > > > > > Does the new behavior align with the spec and other browsers? > > > > > > If so, we might want to update the test so that the result becomes PASS. > > > > Also, the use of "FAIL" for these property-collector tests is a bit > non-standard (at least that's how I think of the FAIL usage here) -- they're in > the sense of "not the same value as before a frame/window operation" rather than > "globally&inherently wrong". > > > > cf. expected applicationCache results for this particular variant. > > The use of FAIL here is intentional. abarth@, japhet@, myself, and probably > others, have wanted to make this behavior more consistent for detached windows- > and in general, that means nulling, zeroing, or otherwise wiping the fields of a > detached window. Thanks for clarifying; in short, nothing to attend to here?
On 2015/01/31 at 09:17:38, sigbjornf wrote: > On 2015/01/31 09:02:41, dcheng wrote: > > On 2015/01/31 at 08:13:55, sigbjornf wrote: > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > File > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt > > (right): > > > > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: > > FAIL childWindow.speechSynthesis.speaking should be false. Threw exception > > TypeError: Cannot read property 'speaking' of null > > > On 2015/01/31 01:30:01, haraken wrote: > > > > > > > > Does the new behavior align with the spec and other browsers? > > > > > > > > If so, we might want to update the test so that the result becomes PASS. > > > > > > Also, the use of "FAIL" for these property-collector tests is a bit > > non-standard (at least that's how I think of the FAIL usage here) -- they're in > > the sense of "not the same value as before a frame/window operation" rather than > > "globally&inherently wrong". > > > > > > cf. expected applicationCache results for this particular variant. > > > > The use of FAIL here is intentional. abarth@, japhet@, myself, and probably > > others, have wanted to make this behavior more consistent for detached windows- > > and in general, that means nulling, zeroing, or otherwise wiping the fields of a > > detached window. > > Thanks for clarifying; in short, nothing to attend to here? Yes, I think this is fine.
On 2015/02/02 18:59:09, dcheng wrote: > On 2015/01/31 at 09:17:38, sigbjornf wrote: > > On 2015/01/31 09:02:41, dcheng wrote: > > > On 2015/01/31 at 08:13:55, sigbjornf wrote: > > > > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > > File > > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > > > > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: > > > FAIL childWindow.speechSynthesis.speaking should be false. Threw exception > > > TypeError: Cannot read property 'speaking' of null > > > > On 2015/01/31 01:30:01, haraken wrote: > > > > > > > > > > Does the new behavior align with the spec and other browsers? > > > > > > > > > > If so, we might want to update the test so that the result becomes PASS. > > > > > > > > Also, the use of "FAIL" for these property-collector tests is a bit > > > non-standard (at least that's how I think of the FAIL usage here) -- they're > in > > > the sense of "not the same value as before a frame/window operation" rather > than > > > "globally&inherently wrong". > > > > > > > > cf. expected applicationCache results for this particular variant. > > > > > > The use of FAIL here is intentional. abarth@, japhet@, myself, and probably > > > others, have wanted to make this behavior more consistent for detached > windows- > > > and in general, that means nulling, zeroing, or otherwise wiping the fields > of a > > > detached window. > > > > Thanks for clarifying; in short, nothing to attend to here? > > Yes, I think this is fine. haraken@: is leaving the expected test output as-is acceptable to you also?
On 2015/02/03 14:29:35, sof wrote: > On 2015/02/02 18:59:09, dcheng wrote: > > On 2015/01/31 at 09:17:38, sigbjornf wrote: > > > On 2015/01/31 09:02:41, dcheng wrote: > > > > On 2015/01/31 at 08:13:55, sigbjornf wrote: > > > > > > > > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > > > File > > > > > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt > > > > (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/881683003/diff/20001/LayoutTests/fast/dom/Win... > > > > > > > > > > > > LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt:200: > > > > FAIL childWindow.speechSynthesis.speaking should be false. Threw exception > > > > TypeError: Cannot read property 'speaking' of null > > > > > On 2015/01/31 01:30:01, haraken wrote: > > > > > > > > > > > > Does the new behavior align with the spec and other browsers? > > > > > > > > > > > > If so, we might want to update the test so that the result becomes > PASS. > > > > > > > > > > Also, the use of "FAIL" for these property-collector tests is a bit > > > > non-standard (at least that's how I think of the FAIL usage here) -- > they're > > in > > > > the sense of "not the same value as before a frame/window operation" > rather > > than > > > > "globally&inherently wrong". > > > > > > > > > > cf. expected applicationCache results for this particular variant. > > > > > > > > The use of FAIL here is intentional. abarth@, japhet@, myself, and > probably > > > > others, have wanted to make this behavior more consistent for detached > > windows- > > > > and in general, that means nulling, zeroing, or otherwise wiping the > fields > > of a > > > > detached window. > > > > > > Thanks for clarifying; in short, nothing to attend to here? > > > > Yes, I think this is fine. > > haraken@: is leaving the expected test output as-is acceptable to you also? Yes, LGTM
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881683003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/49047)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881683003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189443
Message was sent while issue was closed.
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes... ScreenOrientationBrowserTest.CrashTest_UseAfterDetach failing b/w blink 189443 -189439
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/896103002/ by noel@chromium.org. The reason for reverting is: ScreenOrientationBrowserTest.CrashTest_UseAfterDetach failing on ChromeOS b/w blink 189439 - 189443, suspect 189443, revert to see if ChromeOS greens. .
sigbjornf@opera.com changed reviewers: + japhet@chromium.org
Please take another look? I've updated this CL following the issues encountered via https://code.google.com/p/chromium/issues/detail?id=455161. While doing so, I experimented with taking this one step further and also clearing the LocalFrame's DOMWindow during detach. Too many edge cases had to be addressed to make that stable, which left me with little confidence that it could be made reliable, so I backed off from that. (See https://codereview.chromium.org/901903002/ for the approaches experimented with.)
I'm Blink gardening atm, but will try to take a look tonight.
https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... File Source/core/frame/FrameDestructionObserver.h (right): https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... Source/core/frame/FrameDestructionObserver.h:39: virtual void frameDestroyed(); Perhaps we should rename this if it's not sent at destruction time anymore?
https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... File Source/core/frame/FrameDestructionObserver.h (right): https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... Source/core/frame/FrameDestructionObserver.h:39: virtual void frameDestroyed(); On 2015/02/06 20:33:10, dcheng wrote: > Perhaps we should rename this if it's not sent at destruction time anymore? I don't mind, but I believe haraken wants to remove FrameDestructionObserver and have LifecycleObserver<LocalFrame> take its place, so replacing uses of "destruct" with "detach" here is perhaps not worth doing in that light?
On 2015/02/06 20:50:16, sof wrote: > https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... > File Source/core/frame/FrameDestructionObserver.h (right): > > https://codereview.chromium.org/881683003/diff/60001/Source/core/frame/FrameD... > Source/core/frame/FrameDestructionObserver.h:39: virtual void frameDestroyed(); > On 2015/02/06 20:33:10, dcheng wrote: > > Perhaps we should rename this if it's not sent at destruction time anymore? > > I don't mind, but I believe haraken wants to remove FrameDestructionObserver and > have LifecycleObserver<LocalFrame> take its place, so replacing uses of > "destruct" with "detach" here is perhaps not worth doing in that light? I suggest we keep the "Destroyed" naming convention; notifications in these final-lifetime-phases use that (cf. LifecycleNotifier<T>::notifyContextDestroyed() et al.)
lgtm, but let's update the description to include the changes for supplements that are FrameDestructionObservers too.
(friendly) monday ping :)
On 2015/02/09 18:46:11, dcheng wrote: > lgtm, but let's update the description to include the changes for supplements > that are FrameDestructionObservers too. thanks, updated.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881683003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189837
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/908043002/ by sigbjornf@opera.com. The reason for reverting is: Linux ChromiumOS Tests (dbg) bot is timing out on ScreenOrientationBrowserTest.CrashTest_UseAfterDetach, http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes... Can't reproduce locally, but will revert & look into it tomorrow..
On 2015/02/09 22:01:34, sof wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/908043002/ by mailto:sigbjornf@opera.com. > > The reason for reverting is: Linux ChromiumOS Tests (dbg) bot is timing out on > ScreenOrientationBrowserTest.CrashTest_UseAfterDetach, > > > http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes... > > Can't reproduce locally, but will revert & look into it tomorrow.. With the test now adjusted to handle the behavior that this CL brings (via https://codereview.chromium.org/909043002), landing it again.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881683003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189906 |
