Bug 14437 – CRASH: RTÉ video crashes Safari

• Component: Plug-ins
• Status: RESOLVED FIXED

[Nicholas Shanks]

Watch any piece of video or audio (RealPlayer plug‐in reqd.) related to an article on the site http://www.rte.ie/news/

After the advert plays (looks like a flash div overlayed upon the plug‐in) the browser will crash. Could be javascript‐related or a buggy plug‐in not being walled off correctly into it's own address space and taking down the whole app.

[Nicholas Shanks]

Created attachment 15279 [details]
crash log number 1

[Nicholas Shanks]

Created attachment 15280 [details]
crash log number 2

[Alexey Proskuryakov]

Interestingly, I still get this crash if I remove RealPlayer plugin, although Safari does complain about it being unavailable.

[Alexey Proskuryakov]

Created attachment 15295 [details]
test case (DRT-only)

[Alexey Proskuryakov]

Anders asked to assign this bug to him :)

[Maxime BRITTO]

So bad, I've been working on this bug yesterday and I have a fix. I havn't finished the layout test, that's why I havn't attached the patch yet.

[Alexey Proskuryakov]

I think it won't hurt if you both attach your fixes, if it's no additional work for any of you. The test I attached is a finished layout test AFAICT.

We normally use bug assignment to avoid work duplication.

[Maxime BRITTO]

(In reply to comment #7)
> I think it won't hurt if you both attach your fixes, if it's no additional work
> for any of you. The test I attached is a finished layout test AFAICT.
> 
> We normally use bug assignment to avoid work duplication.
> 
I've just tried your test case and you're right it's great. If you agree I would like to put it in my patch because the one I was working on is clearly not as good as yours.

[Maxime BRITTO]

Created attachment 15311 [details]
proposed fix

With this fix Safari won't crash anymore and it will play the video. I don't know if this is the more efficient way but it works and no other test case is affected by this fix.
The test case in the patch is the reduction provided by Alexey Proskuryakov in this page.

Comment on attachment 15311 [details]
proposed fix

It looks like -aeDescByEvaluatingJavaScriptFromString: is susceptible to the same bug.

Can't you just protect the frame and return the result instead of returning nil?

[Darin Adler]

Comment on attachment 15311 [details]
proposed fix

I think a better fix would be to put the frame pointer into a RefPtr rather than simply returning nil in this case. I believe that will fix the bug in a straightforward way. Just:

    RefPtr<Frame> frame = m_frame;

And then use frame instead of m_frame throughout.

Other small issues: ChangeLog is missing a name and email address, needs to be indented properly and misspells the word "function".

[Maxime BRITTO]

Created attachment 15346 [details]
alternate fix

We talk about this with Mitz (which prefers the "RefPtr" fix) and Andersca (which prefer the "return nil" fix) on friday on IRC and Andersca convinced us that returning nil what the best way to fix this.

Still, I tried  the RefPtr way and it doesn't work because we get a nil script proxy (KJSProxy) and it crashes when trying to interpret.

I've found a third way to fix this if you really want to return something in all cases : protecting the ExecState instead of the frame.This way it works and no test case is affected.

[Alexey Proskuryakov]

(In reply to comment #12)

> We talk about this with Mitz (which prefers the "RefPtr" fix) and Andersca
> (which prefer the "return nil" fix) on friday on IRC and Andersca convinced us
> that returning nil what the best way to fix this.

Unfortunately, I have missed this conversation - what were the reasons given for returning nil?

I think that a plugin may want to receive a response even after being stopped - it still can do something that doesn't require GUI interaction (e.g. write preferences or report back to origin site).

[Anders Carlsson]

(In reply to comment #13)
> (In reply to comment #12)
> 
> 
> I think that a plugin may want to receive a response even after being stopped -
> it still can do something that doesn't require GUI interaction (e.g. write
> preferences or report back to origin site).
> 

That's a separate bug then - There's already code in evaluateJavaScriptPluginRequest: that does nothing if the plug-in has been stopped:

// Don't continue if stringByEvaluatingJavaScriptFromString caused the plug-in to stop.
if (!isStarted) {
    return;
}

FWIW, we do the same thing on Windows. 

[Anders Carlsson]

Comment on attachment 15346 [details]
alternate fix

>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.

[Anders Carlsson]

Created attachment 15371 [details]
Tweaked initial patch

Here's the first patch with some added asserts.

[Anders Carlsson]

Comment on attachment 15371 [details]
Tweaked initial patch

Alexey is convinced but thinks Darin should review the patch since he r-:ed the first patch.

[Darin Adler]

Comment on attachment 15371 [details]
Tweaked initial patch

I just talked this over with Anders. It's clear that in cases where the JSValue is an object, we can't return the object, because we can't run its toString method in the right context -- the context is gone because the frame is destroyed.

But in the simple cases where the JSValue is not an object, it seems there's no reason we can't return the value. So I suggest we make a version of toString that works on non-object JSValue, and use that if m_frame is nil. That way the common cases of plain old strings and such will work fine.

That seems slightly better than always returning nil. review- until we at least consider that option.

[Anders Carlsson]

Created attachment 15375 [details]
Address comments

[Darin Adler]

Comment on attachment 15375 [details]
Address comments

I think it would be more elegant if there was a call in JSValue we could use rather than having to do both JSImmediate::toString and getString.

But this seems good. r=me

[Anders Carlsson]

Committed revision 23950.

[David Kilzer (:ddkilzer)]

<rdar://problem/5316292>