Changeset 134495 in webkit

Timestamp:

Nov 13, 2012 3:21:38 PM (11 years ago)

Author:

mark.lam@apple.com

Message:

JSEventListener should not access m_jsFunction when its wrapper is gone.
​https://bugs.webkit.org/show_bug.cgi?id=101985.

Reviewed by Geoffrey Garen.

Added a few null checks for m_wrapper before we do anything with m_jsFunction.

No new tests.

• bindings/js/JSEventListener.cpp:

(WebCore::JSEventListener::initializeJSFunction):

• Removed a now invalid assertion. m_wrapper is expected to have a valid non-zero value when jsFunction is valid. However, in the case of JSLazyEventListener (which extends JSEventListener), m_wrapper is initially 0 when m_jsFunction has not been realized yet. When JSLazyEventListener::initializeJSFunction() realizes m_jsFunction, it will set m_wrapper to an appropriate wrapper object.

For this reason, JSEventListener::jsFunction() cannot do the null
check on m_wrapper until after the call to initializeJSFunction.

This, in turns, means that in the case of the non-lazy
JSEventListener, initializeJSFunction() will also be called, and
if the GC has collected the m_wrapper but the JSEventListener has
not been removed yet, it is possible to see a null m_wrapper while
m_jsFunction contains a non-zero stale value.

Hence, this assertion of (m_wrapper

!m_jsFunction) in

JSEventListener::initializeJSFunction() is not always true and
should be removed.

(WebCore::JSEventListener::visitJSFunction):
(WebCore::JSEventListener::operator==):

• bindings/js/JSEventListener.h:

(WebCore::JSEventListener::jsFunction):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/js/JSEventListener.cpp (modified) (3 diffs)
• bindings/js/JSEventListener.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-11-13  Mark Lam  <mark.lam@apple.com>
+
+        JSEventListener should not access m_jsFunction when its wrapper is gone.
+        https://bugs.webkit.org/show_bug.cgi?id=101985.
+
+        Reviewed by Geoffrey Garen.
+
+        Added a few null checks for m_wrapper before we do anything with m_jsFunction.
+
+        No new tests.
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::JSEventListener::initializeJSFunction):
+        - Removed a now invalid assertion. m_wrapper is expected to have a
+          valid non-zero value when jsFunction is valid. However, in the case
+          of JSLazyEventListener (which extends JSEventListener), m_wrapper is
+          initially 0 when m_jsFunction has not been realized yet. When
+          JSLazyEventListener::initializeJSFunction() realizes m_jsFunction,
+          it will set m_wrapper to an appropriate wrapper object.
+
+          For this reason, JSEventListener::jsFunction() cannot do the null
+          check on m_wrapper until after the call to initializeJSFunction.
+
+          This, in turns, means that in the case of the non-lazy
+          JSEventListener, initializeJSFunction() will also be called, and
+          if the GC has collected the m_wrapper but the JSEventListener has
+          not been removed yet, it is possible to see a null m_wrapper while
+          m_jsFunction contains a non-zero stale value.
+
+          Hence, this assertion of (m_wrapper || !m_jsFunction) in
+          JSEventListener::initializeJSFunction() is not always true and
+          should be removed.
+
+        (WebCore::JSEventListener::visitJSFunction):
+        (WebCore::JSEventListener::operator==):
+        * bindings/js/JSEventListener.h:
+        (WebCore::JSEventListener::jsFunction):
+
 2012-11-13  Adam Barth  <abarth@webkit.org>
 

trunk/Source/WebCore/bindings/js/JSEventListener.cpp

@@ -60 +60 @@
 JSObject* JSEventListener::initializeJSFunction(ScriptExecutionContext*) const
 {
-    ASSERT_NOT_REACHED();
     return 0;
 }
@@ -66 +65 @@
 void JSEventListener::visitJSFunction(SlotVisitor& visitor)
 {
+    // If m_wrapper is 0, then jsFunction is zombied, and should never be accessed.
+    if (!m_wrapper)
+        return;
+
     if (m_jsFunction)
         visitor.append(&m_jsFunction);
@@ -161 +164 @@
 bool JSEventListener::operator==(const EventListener& listener)
 {
+    // If m_wrapper is 0, then jsFunction is zombied, and should never be accessed.
+    if (!m_wrapper)
+        return false;
+
     if (const JSEventListener* jsEventListener = JSEventListener::cast(&listener))
         return m_jsFunction == jsEventListener->m_jsFunction && m_isAttribute == jsEventListener->m_isAttribute;

trunk/Source/WebCore/bindings/js/JSEventListener.h

@@ -86 +86 @@
         }
 
+        // If m_wrapper is 0, then jsFunction is zombied, and should never be accessed.
+        if (!m_wrapper)
+            return 0;
+
         // Verify that we have a valid wrapper protecting our function from
         // garbage collection.
         ASSERT(m_wrapper || !m_jsFunction);
-        if (!m_wrapper)
-            return 0;
 
         // Try to verify that m_jsFunction wasn't recycled. (Not exact, since an