Changeset 135878 in webkit

Timestamp:

Nov 27, 2012, 10:36:16 AM (12 years ago)

Author:

Lucas Forschler

Message:

Merge 134495 & 134666 for <rdar://problem/12696290> & <rdar://problem/12704510>

Location:

branches/safari-536.28-branch/Source/WebCore

Files:

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

branches/safari-536.28-branch/Source/WebCore/ChangeLog

@@ +1 @@
+2012-11-27  Lucas Forschler  <lforschler@apple.com>
+
+        <rdar://problem/12704510>
+        Merge r134666
+
+    2012-11-14  Mark Lam  <mark.lam@apple.com>
+
+            Fixed regressions due to adding JSEventListener::m_wrapper null checks.
+            https://bugs.webkit.org/show_bug.cgi?id=102183.
+
+            Reviewed by Geoffrey Garen.
+
+            Fixed JSEventListener::operator==() to work within the contract that
+            when m_wrapper is 0, m_jsFunction is also expected to be 0. Also fixed
+            some typos in comments.
+
+            No new tests.
+
+            * bindings/js/JSEventListener.cpp:
+            (WebCore::JSEventListener::visitJSFunction):
+            (WebCore::JSEventListener::operator==):
+            * bindings/js/JSEventListener.h:
+            (WebCore::JSEventListener::jsFunction):
+
+2012-11-27  Lucas Forschler  <lforschler@apple.com>
+
+        <rdar://problem/12696290>
+        Merge r134495
+
+    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-26  Simon Fraser  <simon.fraser@apple.com>
 

branches/safari-536.28-branch/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 m_jsFunction is zombied, and should never be accessed.
+    if (!m_wrapper)
+        return;
+
     if (m_jsFunction)
         visitor.append(&m_jsFunction);
@@ -167 +170 @@
 bool JSEventListener::operator==(const EventListener& listener)
 {
-    if (const JSEventListener* jsEventListener = JSEventListener::cast(&listener))
-        return m_jsFunction == jsEventListener->m_jsFunction && m_isAttribute == jsEventListener->m_isAttribute;
+    if (const JSEventListener* jsEventListener = JSEventListener::cast(&listener)) {
+        // If m_wrapper is 0, then m_jsFunction is zombied, and should never be
+        // accessed. m_jsFunction should effectively be 0 in that case.
+        JSC::JSObject* jsFunction = m_wrapper ? m_jsFunction.get() : 0;
+        JSC::JSObject* otherJSFunction = jsEventListener->m_wrapper ?
+            jsEventListener->m_jsFunction.get() : 0;
+        return jsFunction == otherJSFunction && m_isAttribute == jsEventListener->m_isAttribute;
+    }
     return false;
 }

branches/safari-536.28-branch/Source/WebCore/bindings/js/JSEventListener.h

@@ -86 +86 @@
         }
 
+        // If m_wrapper is 0, then m_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