[webkit-changes] [169781] trunk/Source/WebCore

Tue, 10 Jun 2014 16:31:28 -0700

Title: [169781] trunk/Source/WebCore
Revision
169781
Author
bfulg...@apple.com
Date
2014-06-10 16:30:43 -0700 (Tue, 10 Jun 2014)

Log Message

REGRESSION (r167962): Out of bounds read in JSC::StructureIDTable::get()
https://bugs.webkit.org/show_bug.cgi?id=133463
<rdar://problem/17098100>

Reviewed by Geoffrey Garen.

Revise MediaControllerHost implementation so that instead of holding its
own pointer to the JS Controller object, it uses new properties added to
the internal media controls DOM hierarchy. This allows the GC to see the
needed lifecycle of the various media control objects and avoids the
premature deallocation that caused this bug.
 
* Modules/mediacontrols/MediaControlsHost.h:
(WebCore::MediaControlsHost::controllerJSValue): Deleted.
(WebCore::MediaControlsHost::setControllerJSValue): Deleted.
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::controllerJSValue): Added convenience function
to share logic for retrieving the controller object.
(WebCore::HTMLMediaElement::updateCaptionContainer): Revise to use new method
for accessing the controller.
(WebCore::HTMLMediaElement::didAddUserAgentShadowRoot): Connect the media
elements JS wrapper object to the MediaControlsHost JS wrapper. Then connect
the MediaControlsHost JS wrapper to the Controller JS object.
(WebCore::HTMLMediaElement::pageScaleFactorChanged): Revise to use new method
for accessing the controller.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (169780 => 169781)


--- trunk/Source/WebCore/ChangeLog	2014-06-10 23:23:46 UTC (rev 169780)
+++ trunk/Source/WebCore/ChangeLog	2014-06-10 23:30:43 UTC (rev 169781)
@@ -1,3 +1,31 @@
+2014-06-09  Brent Fulgham  <bfulg...@apple.com>
+
+        REGRESSION (r167962): Out of bounds read in JSC::StructureIDTable::get()
+        https://bugs.webkit.org/show_bug.cgi?id=133463
+        <rdar://problem/17098100>
+
+        Reviewed by Geoffrey Garen.
+
+        Revise MediaControllerHost implementation so that instead of holding its
+        own pointer to the JS Controller object, it uses new properties added to
+        the internal media controls DOM hierarchy. This allows the GC to see the
+        needed lifecycle of the various media control objects and avoids the
+        premature deallocation that caused this bug.
+ 
+        * Modules/mediacontrols/MediaControlsHost.h:
+        (WebCore::MediaControlsHost::controllerJSValue): Deleted.
+        (WebCore::MediaControlsHost::setControllerJSValue): Deleted.
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::controllerJSValue): Added convenience function
+        to share logic for retrieving the controller object.
+        (WebCore::HTMLMediaElement::updateCaptionContainer): Revise to use new method
+        for accessing the controller.
+        (WebCore::HTMLMediaElement::didAddUserAgentShadowRoot): Connect the media
+        elements JS wrapper object to the MediaControlsHost JS wrapper. Then connect
+        the MediaControlsHost JS wrapper to the Controller JS object.
+        (WebCore::HTMLMediaElement::pageScaleFactorChanged): Revise to use new method
+        for accessing the controller.
+
 2014-06-10  Myles C. Maxfield  <mmaxfi...@apple.com>
 
         Japanese text in Google search is rendered too low and clipped

Modified: trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h (169780 => 169781)


--- trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h	2014-06-10 23:23:46 UTC (rev 169780)
+++ trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h	2014-06-10 23:30:43 UTC (rev 169781)
@@ -71,15 +71,11 @@
     bool controlsDependOnPageScaleFactor() const;
     void setControlsDependOnPageScaleFactor(bool v);
 
-    JSC::JSValue controllerJSValue() const { return m_controller; }
-    void setControllerJSValue(JSC::JSValue controller) { m_controller = controller; }
-
 private:
     MediaControlsHost(HTMLMediaElement*);
 
     HTMLMediaElement* m_mediaElement;
     RefPtr<MediaControlTextTrackContainerElement> m_textTrackContainer;
-    JSC::JSValue m_controller;
 };
 
 }

Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (169780 => 169781)


--- trunk/Source/WebCore/html/HTMLMediaElement.cpp	2014-06-10 23:23:46 UTC (rev 169780)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp	2014-06-10 23:30:43 UTC (rev 169781)
@@ -3649,6 +3649,32 @@
     m_processingPreferenceChange = false;
 }
 
+static JSC::JSValue controllerJSValue(JSC::ExecState& exec, JSDOMGlobalObject& globalObject, HTMLMediaElement& media)
+{
+    auto mediaJSWrapper = toJS(&exec, &globalObject, &media);
+    
+    // Retrieve the controller through the JS object graph
+    JSC::JSObject* mediaJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaJSWrapper);
+    if (!mediaJSWrapperObject)
+        return JSC::jsNull();
+    
+    JSC::Identifier controlsHost(&exec.vm(), "controlsHost");
+    JSC::JSValue controlsHostJSWrapper = mediaJSWrapperObject->get(&exec, controlsHost);
+    if (exec.hadException())
+        return JSC::jsNull();
+
+    JSC::JSObject* controlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(controlsHostJSWrapper);
+    if (!controlsHostJSWrapperObject)
+        return JSC::jsNull();
+
+    JSC::Identifier controllerID(&exec.vm(), "controller");
+    JSC::JSValue controllerJSWrapper = controlsHostJSWrapperObject->get(&exec, controllerID);
+    if (exec.hadException())
+        return JSC::jsNull();
+
+    return controllerJSWrapper;
+}
+    
 void HTMLMediaElement::updateCaptionContainer()
 {
     LOG(Media, "HTMLMediaElement::updateCaptionContainer");
@@ -3672,12 +3698,11 @@
     JSC::ExecState* exec = globalObject->globalExec();
     JSC::JSLockHolder lock(exec);
 
-    JSC::JSValue controllerValue = m_mediaControlsHost->controllerJSValue();
-    if (controllerValue.isUndefinedOrNull() || !controllerValue.isObject())
+    JSC::JSValue controllerValue = controllerJSValue(*exec, *globalObject, *this);
+    JSC::JSObject* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
+    if (!controllerObject)
         return;
 
-    JSC::JSObject* controllerObject = controllerValue.toObject(exec);
-
     // The media controls script must provide a method on the Controller object with the following details.
     // Name: updateCaptionContainer
     // Parameters:
@@ -3685,11 +3710,10 @@
     // Return value:
     //     None
     JSC::JSValue methodValue = controllerObject->get(exec, JSC::Identifier(exec, "updateCaptionContainer"));
-    if (methodValue.isUndefinedOrNull() || !methodValue.isObject())
+    JSC::JSObject* methodObject = JSC::jsDynamicCast<JSC::JSObject*>(methodValue);
+    if (!methodObject)
         return;
 
-    JSC::JSObject* methodObject = methodValue.toObject(exec);
-
     JSC::CallData callData;
     JSC::CallType callType = methodObject->methodTable()->getCallData(methodObject, callData);
     if (callType == JSC::CallTypeNone)
@@ -5795,10 +5819,13 @@
     if (!m_mediaControlsHost)
         m_mediaControlsHost = MediaControlsHost::create(this);
 
+    auto mediaJSWrapper = toJS(exec, globalObject, this);
+    auto mediaControlsHostJSWrapper = toJS(exec, globalObject, m_mediaControlsHost.get());
+    
     JSC::MarkedArgumentBuffer argList;
     argList.append(toJS(exec, globalObject, root));
-    argList.append(toJS(exec, globalObject, this));
-    argList.append(toJS(exec, globalObject, m_mediaControlsHost.get()));
+    argList.append(mediaJSWrapper);
+    argList.append(mediaControlsHostJSWrapper);
 
     JSC::JSObject* function = functionValue.toObject(exec);
     JSC::CallData callData;
@@ -5807,8 +5834,28 @@
         return;
 
     JSC::JSValue controllerValue = JSC::call(exec, function, callType, callData, globalObject, argList);
-    m_mediaControlsHost->setControllerJSValue(controllerValue);
+    JSC::JSObject* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
+    if (!controllerObject)
+        return;
 
+    // Connect the Media, MediaControllerHost, and Controller so the GC knows about their relationship
+    JSC::JSObject* mediaJSWrapperObject = mediaJSWrapper.toObject(exec);
+    JSC::Identifier controlsHost(&exec->vm(), "controlsHost");
+    
+    ASSERT(!mediaJSWrapperObject->hasProperty(exec, controlsHost));
+
+    mediaJSWrapperObject->putDirect(exec->vm(), controlsHost, mediaControlsHostJSWrapper, JSC::DontDelete | JSC::DontEnum | JSC::ReadOnly);
+
+    JSC::JSObject* mediaControlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaControlsHostJSWrapper);
+    if (!mediaControlsHostJSWrapperObject)
+        return;
+    
+    JSC::Identifier controller(&exec->vm(), "controller");
+
+    ASSERT(!controllerObject->hasProperty(exec, controller));
+
+    mediaControlsHostJSWrapperObject->putDirect(exec->vm(), controller, controllerValue, JSC::DontDelete | JSC::DontEnum | JSC::ReadOnly);
+
     setPageScaleFactorProperty(exec, controllerValue, page->pageScaleFactor());
 
     if (exec->hadException())
@@ -5842,7 +5889,7 @@
     JSC::ExecState* exec = globalObject->globalExec();
     JSC::JSLockHolder lock(exec);
 
-    JSC::JSValue controllerValue = m_mediaControlsHost->controllerJSValue();
+    JSC::JSValue controllerValue = controllerJSValue(*exec, *globalObject, *this);
 
     setPageScaleFactorProperty(exec, controllerValue, page->pageScaleFactor());
 }

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to