Track event handlers on windows

Source/core/frame/DOMWindow.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 41 matching lines @@
 #include "core/events/DOMWindowEventQueue.h"
 #include "core/events/EventListener.h"
 #include "core/events/HashChangeEvent.h"
 #include "core/events/MessageEvent.h"
 #include "core/events/PageTransitionEvent.h"
 #include "core/events/PopStateEvent.h"
 #include "core/frame/BarProp.h"
 #include "core/frame/Console.h"
 #include "core/frame/DOMPoint.h"
 #include "core/frame/DOMWindowLifecycleNotifier.h"
+#include "core/frame/EventHandlerRegistry.h"
 #include "core/frame/FrameConsole.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/History.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Location.h"
 #include "core/frame/Navigator.h"
 #include "core/frame/Screen.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLFrameOwnerElement.h"
@@ skipping 250 matching lines @@
 }
 
 DOMWindow::DOMWindow(LocalFrame& frame)
     : FrameDestructionObserver(&frame)
     , m_shouldPrintWhenFinishedLoading(false)
 #if ASSERT_ENABLED
     , m_hasBeenReset(false)
 #endif
 {
     ScriptWrappable::init(this);
+    m_frame->host()->eventHandlerRegistry().didMoveIntoFrameHost(*this);

[Rick Byers, 2014/05/06 20:54:39]

Please add a comment describing why this is necessary.  Naively I don't see how
there can be any event handlers to attach to the registry at this point when the
window is just being created now.  Ah, I see now - it's to ensure the observer
is created.

I think separate windowCreated/windowDestroyed entrypoints into the registry
might be clearer.  Then you could ASSERT in in windowCreated that there are no
event handlers on the window (and maybe even assert in windowDestroyed that
there aren't any handlers left in the registry if we can get deterministic
cleanup).  WDYT?

Overall it seems like this might be a lot of extra bookkeeping in order to
robustly support the DOMWindowLifecycleObserver abstraction.  Since every
DOMWindow should always have exactly one FrameHost, perhaps this would get a lot
simpler (and more robust) if we just hard-coded calls into EventHandlerRegistry
in DOMWindow::*eventListener (as the touch event handlers are tracked today)?

[Sami, 2014/05/07 10:36:41]

Yeah, I was thinking along the same lines -- it would be much simpler just to
add the few hooks that we need to DOMWindow. I've now done that and I think the
result is much cleaner. WDYT?

[Rick Byers, 2014/05/07 13:28:21]

Yeah this seems a lot cleaner.  The observer abstraction is a nice idea, but in
this case I don't think it's worth the extra lifetime management complexity.

 }
 
 void DOMWindow::clearDocument()
 {
     if (!m_document)
         return;
 
     if (m_document->isActive()) {
         // FIXME: We don't call willRemove here. Why is that OK?
         // This detach() call is also mostly redundant. Most of the calls to
@@ skipping 135 matching lines @@
         enqueuePopstateEvent(stateObject);
     else
         m_pendingStateObject = stateObject;
 }
 
 DOMWindow::~DOMWindow()
 {
     ASSERT(m_hasBeenReset);
     reset();
 
+    if (m_frame && m_frame->host())
+        m_frame->host()->eventHandlerRegistry().didMoveOutOfFrameHost(*this);
+
     removeAllEventListeners();
 
     ASSERT(m_document->isStopped());
     clearDocument();
 }
 
 const AtomicString& DOMWindow::interfaceName() const
 {
     return EventTargetNames::DOMWindow;
 }
@@ skipping 17 matching lines @@
 {
     return frame() ? frame()->page() : 0;
 }
 
 void DOMWindow::frameDestroyed()
 {
     FrameDestructionObserver::frameDestroyed();
     reset();
 }
 
 void DOMWindow::willDetachFrameHost()

[Rick Byers, 2014/05/06 20:54:39]

Does this only happen when a window is being destroyed?  Is there any way we can
verify that event handlers aren't added after this point?

[Sami, 2014/05/07 10:36:41]

I don't think we need to worry about that. If handlers are attached after
FrameHost goes away, they shouldn't appear in the registry anyway. There's also
no need to import such handlers later, since as far as I can tell DOMWindow
can't attach to a different FrameHost.

The critical thing is to make sure we unregister all existing handlers before
the FrameHost goes away. The asserts in EventHandlerRegistry::checkConsistency()
check for that.

[Rick Byers, 2014/05/07 13:28:21]

Thanks, sounds reasonable.

 {
+    m_frame->host()->eventHandlerRegistry().didMoveOutOfFrameHost(*this);
     InspectorInstrumentation::frameWindowDiscarded(m_frame, this);
 }
 
 void DOMWindow::willDestroyDocumentInFrame()
 {
     // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
     // unregister themselves from the DOMWindow as a result of the call to willDestroyGlobalObjectInFrame.
     Vector<DOMWindowProperty*> properties;
     copyToVector(m_properties, properties);
     for (size_t i = 0; i < properties.size(); ++i)
@@ skipping 1329 matching lines @@
     visitor->trace(m_media);
     visitor->trace(m_sessionStorage);
     visitor->trace(m_localStorage);
     visitor->trace(m_applicationCache);
     visitor->trace(m_performance);
     visitor->trace(m_css);
     WillBeHeapSupplementable<DOMWindow>::trace(visitor);
 }
 
 } // namespace WebCore