Remove all event listeners during window's frame destruction step.

Remove all event listeners during window's frame destruction step.

Perform the removal of LocalDOMWindow event listeners as part of it being
informed of LocalFrame's destruction (via frameDestroyed()). This is preferable
to waiting until LocalDOMWindow's destructor is run, as it makes the timing of it
independent of when that destructor actually gets to run.

With Oilpan, it is possible that a frame and its window will be swept out without
an explicit frame destruction notification. In that event, a prefinalizing
action is run to take care of the removal.

Additionally, this CL removes an explicit call to reset() from the LocalDOMWindow
destructor. We have since r182337 been asserting that the reset() has already
occurred, without it ever triggering. Time to remove the redundant reset() call.

R=haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192149

[sof, 2015-03-18 22:12:30]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[sof, 2015-03-18 22:12:31]

please take a look.

[haraken, 2015-03-18 23:38:45]

Nice cleanup. LGTM.

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:506: #if ENABLE(OILPAN)

Not related to your CL, I'm wondering if we could remove this ENABLE(OILPAN)
branch. Now that we have a pre-finalizer in LocalDOMWindow, I guess it would be
possible to refactor the code so that oilpan and non-oilpan use the same code.

Ignore this comment if I'm not making sense. I don't quite remember why we have
different ASSERTs between oilpan and non-oilpan here. Either way, a separate CL
is fine.

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:522: // (Non-Oilpan,
LocalDOMWindow::reset() will always be invoked which takes care of this step.)

Help me understand: The LocalDOMWindow::reset() is called by
LocalFrame::setDOMWindow(), and it seems there is no difference in timing when
the setDOMWindow() is called in oilpan and non-oilpan. If that's the case, why
do you argue that reset() is always called in non-oilpan but not in oilpan?

[sof, 2015-03-19 06:27:36]

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:506: #if ENABLE(OILPAN)
On 2015/03/18 23:38:45, haraken wrote:
> 
> Not related to your CL, I'm wondering if we could remove this ENABLE(OILPAN)
> branch. Now that we have a pre-finalizer in LocalDOMWindow, I guess it would
be
> possible to refactor the code so that oilpan and non-oilpan use the same code.
> 
> Ignore this comment if I'm not making sense. I don't quite remember why we
have
> different ASSERTs between oilpan and non-oilpan here. Either way, a separate
CL
> is fine.
> 

It would be nice to empty this dtor entirely of anything but asserts, but I
haven't investigated what parts of clearDocument() can be moved up/out.

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:522: // (Non-Oilpan,
LocalDOMWindow::reset() will always be invoked which takes care of this step.)
On 2015/03/18 23:38:45, haraken wrote:
> 
> Help me understand: The LocalDOMWindow::reset() is called by
> LocalFrame::setDOMWindow(), and it seems there is no difference in timing when
> the setDOMWindow() is called in oilpan and non-oilpan. If that's the case, why
> do you argue that reset() is always called in non-oilpan but not in oilpan?

Yes, if the frame is detached, reset() will be called at the same time. Should
that not happen & when end up finalizing LocalFrame, ~LocalFrame() will only
call setDOMWindow() with Oilpan disabled. That's where the difference comes
from.

(A comment with a quiet word of caution about not adding too much code to this
dispose() method might be worth it here. We don't want to be doing too much work
in these actions.)

[haraken, 2015-03-19 06:54:45]

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1017043002/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:522: // (Non-Oilpan,
LocalDOMWindow::reset() will always be invoked which takes care of this step.)
On 2015/03/19 06:27:36, sof wrote:
> On 2015/03/18 23:38:45, haraken wrote:
> > 
> > Help me understand: The LocalDOMWindow::reset() is called by
> > LocalFrame::setDOMWindow(), and it seems there is no difference in timing
when
> > the setDOMWindow() is called in oilpan and non-oilpan. If that's the case,
why
> > do you argue that reset() is always called in non-oilpan but not in oilpan?
> 
> Yes, if the frame is detached, reset() will be called at the same time. Should
> that not happen & when end up finalizing LocalFrame, ~LocalFrame() will only
> call setDOMWindow() with Oilpan disabled. That's where the difference comes
> from.
> 
> (A comment with a quiet word of caution about not adding too much code to this
> dispose() method might be worth it here. We don't want to be doing too much
work
> in these actions.)

Thanks for the clarification; makes sense!

[sof, 2015-03-19 08:21:20]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-03-19 08:21:21]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1017043002/#ps60001
(title: "Improve LocalDOMWindow::dispose() comment a bit")

[commit-bot: I haz the power, 2015-03-19 08:21:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017043002/60001

[commit-bot: I haz the power, 2015-03-19 10:17:01]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192149