[webkit-reviews] review denied: [Bug 14437] CRASH: RTÉ video crashes Safari : [Attachment 15346] alternate fix

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Maxime BRITTO
<mbritto at pleyo.com>'s request for review:
Bug 14437: CRASH: RTÉ video crashes Safari
http://bugs.webkit.org/show_bug.cgi?id=14437

Attachment 15346: alternate fix
http://bugs.webkit.org/attachment.cgi?id=15346&action=edit

------- Additional Comments from Anders Carlsson <andersca at apple.com>
>Index: WebCore/page/mac/WebCoreFrameBridge.mm
>===================================================================
>--- WebCore/page/mac/WebCoreFrameBridge.mm	(revision 23926)
>+++ WebCore/page/mac/WebCoreFrameBridge.mm	(working copy)
>@@ -682,20 +682,32 @@ static HTMLFormElement *formElementFromD
> - (NSString *)stringByEvaluatingJavaScriptFromString:(NSString *)string
forceUserGesture:(BOOL)forceUserGesture
> {
>     ASSERT(m_frame->document());
>+    
>+    //The main frame cannot be destroyed so we can backup the global exec
state
>+    ExecState *ex = m_frame->scriptProxy()->interpreter()->globalExec();

The above comment isn't really correct here. While it is true that a script
can't cause the main frame to be destroyed,
there's no guarantee that the main frame actually is passed in here.

>+    //the script may destroy the frame if it's not the main frame
>     JSValue* result = m_frame->loader()->executeScript(0, string,
forceUserGesture);

Which means that m_frame could point to garbage memory here

>     JSLock lock;
>-    return String(result ?
result->toString(m_frame->scriptProxy()->interpreter()->globalExec()) : "");
>+    return String(result ? result->toString(ex) : "");

And ex could point to garbage memory here too.

> - (NSAppleEventDescriptor *)aeDescByEvaluatingJavaScriptFromString:(NSString
*)
> {

>From code inspection we do however know that the only place this function is
called from is 

- (NSAppleEventDescriptor *)aeDescByEvaluatingJavaScriptFromString:(NSString
*)script
{
    return [[[self mainFrame] _bridge]
aeDescByEvaluatingJavaScriptFromString:script];
}

Which means that m_frame will be valid even after running the script.

>     ASSERT(m_frame->document());
>+    
>+    //The main frame cannot be destroyed so we can backup the global exec
state
>+    ExecState *ex = m_frame->scriptProxy()->interpreter()->globalExec();
>+    
>+    //the script may destroy the frame if it's not the main frame
>     JSValue* result = m_frame->loader()->executeScript(0, string, true);
>+    
>     if (!result) // FIXME: pass errors
>	  return 0;
>+    
>     JSLock lock;
>-    return
aeDescFromJSValue(m_frame->scriptProxy()->interpreter()->globalExec(), result);

>+    return aeDescFromJSValue(ex, result);

So this code does not need to change.