Revert 181758 "Simplify DOMWindowProperty and its unregistration."

This CL caused use-after-free in :LocalDOMWindow::willDetachDocumentFromFrame.
https://code.google.com/p/chromium/issues/detail?id=413316

> Simplify DOMWindowProperty and its unregistration.
> 
> As LocalDOMWindow controls the registration lifetime of its
> DOMWindowProperties, there's no need to offer a separate unregistration
> mechanism. Remove it, simplify notification code + retire now unused
> LocalDOMWindow back reference on DOMWindowProperty.
> 
> R=haraken,ager
> BUG=
> 
> Review URL: https://codereview.chromium.org/558213002

TBR=sigbjornf@opera.com

Review URL: https://codereview.chromium.org/564013002

git-svn-id: svn://svn.chromium.org/blink/trunk@181867 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/core/frame/DOMWindowProperty.cpp

@@ -34,24 +34,39 @@ namespace blink {
 
 DOMWindowProperty::DOMWindowProperty(LocalFrame* frame)
     : m_frame(frame)
+    , m_associatedDOMWindow(nullptr)
 {
     // FIXME: For now it *is* acceptable for a DOMWindowProperty to be created with a null frame.
     // See fast/dom/navigator-detached-no-crash.html for the recipe.
     // We should fix that.  <rdar://problem/11567132>
-    if (m_frame)
-        m_frame->domWindow()->registerProperty(this);
+    if (m_frame) {
+        m_associatedDOMWindow = m_frame->domWindow();
+        m_associatedDOMWindow->registerProperty(this);
+    }
 }
 
-DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(DOMWindowProperty);
+#if !ENABLE(OILPAN)
+DOMWindowProperty::~DOMWindowProperty()
+{
+    if (m_associatedDOMWindow)
+        m_associatedDOMWindow->unregisterProperty(this);
+
+    m_associatedDOMWindow = nullptr;
+    m_frame = nullptr;
+}
+#endif
 
 void DOMWindowProperty::willDestroyGlobalObjectInFrame()
 {
     // If the property is getting this callback it must have been created with a LocalFrame/LocalDOMWindow and it should still have them.
     ASSERT(m_frame);
+    ASSERT(m_associatedDOMWindow);
 
-    // DOMWindowProperty's registered lifetime is controlled by LocalDOMWindow. After
-    // the willDestroyGlobalObjectInFrame() notifications, LocalDOMWindow clears out
-    // all registrations. Hence no need to unregister.
+    // DOMWindowProperty lifetime isn't tied directly to the LocalDOMWindow itself so it is important that it unregister
+    // itself from any LocalDOMWindow it is associated with if that LocalDOMWindow is going away.
+    if (m_associatedDOMWindow)
+        m_associatedDOMWindow->unregisterProperty(this);
+    m_associatedDOMWindow = nullptr;
     m_frame = nullptr;
 }
 
@@ -59,6 +74,12 @@ void DOMWindowProperty::willDetachGlobalObjectFromFrame()
 {
     // If the property is getting this callback it must have been created with a LocalFrame/LocalDOMWindow and it should still have them.
     ASSERT(m_frame);
+    ASSERT(m_associatedDOMWindow);
 }
 
-} // namespace blink
+void DOMWindowProperty::trace(Visitor* visitor)
+{
+    visitor->trace(m_associatedDOMWindow);
+}
+
+}

third_party/WebKit/Source/core/frame/DOMWindowProperty.h

@@ -30,10 +30,10 @@
 
 namespace blink {
 
+class LocalDOMWindow;
 class LocalFrame;
 
 class DOMWindowProperty : public WillBeGarbageCollectedMixin {
-    DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED(DOMWindowProperty);
 public:
     explicit DOMWindowProperty(LocalFrame*);
 
@@ -42,10 +42,15 @@ public:
 
     LocalFrame* frame() const { return m_frame; }
 
-    virtual void trace(Visitor*) { }
+    virtual void trace(Visitor*);
 
 protected:
+#if !ENABLE(OILPAN)
+    virtual ~DOMWindowProperty();
+#endif
+
     LocalFrame* m_frame;
+    RawPtrWillBeMember<LocalDOMWindow> m_associatedDOMWindow;
 };
 
 }

third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp

@@ -556,14 +556,22 @@ void LocalDOMWindow::willDetachFrameHost()
 
 void LocalDOMWindow::willDestroyDocumentInFrame()
 {
-    for (WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> >::const_iterator it = m_properties.begin(); it != m_properties.end(); ++it)
-        (*it)->willDestroyGlobalObjectInFrame();
+    // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
+    // unregister themselves from the LocalDOMWindow as a result of the call to willDestroyGlobalObjectInFrame.
+    WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties;
+    copyToVector(m_properties, properties);
+    for (size_t i = 0; i < properties.size(); ++i)
+        properties[i]->willDestroyGlobalObjectInFrame();
 }
 
 void LocalDOMWindow::willDetachDocumentFromFrame()
 {
-    for (WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> >::const_iterator it = m_properties.begin(); it != m_properties.end(); ++it)
-        (*it)->willDetachGlobalObjectFromFrame();
+    // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
+    // unregister themselves from the LocalDOMWindow as a result of the call to willDetachGlobalObjectFromFrame.
+    WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties;
+    copyToVector(m_properties, properties);
+    for (size_t i = 0; i < properties.size(); ++i)
+        properties[i]->willDetachGlobalObjectFromFrame();
 }
 
 void LocalDOMWindow::registerProperty(DOMWindowProperty* property)
@@ -571,6 +579,11 @@ void LocalDOMWindow::registerProperty(DOMWindowProperty* property)
     m_properties.add(property);
 }
 
+void LocalDOMWindow::unregisterProperty(DOMWindowProperty* property)
+{
+    m_properties.remove(property);
+}
+
 void LocalDOMWindow::reset()
 {
     willDestroyDocumentInFrame();

third_party/WebKit/Source/core/frame/LocalDOMWindow.h

@@ -112,6 +112,7 @@ public:
     virtual LocalDOMWindow* toDOMWindow() OVERRIDE;
 
     void registerProperty(DOMWindowProperty*);
+    void unregisterProperty(DOMWindowProperty*);
 
     void reset();