Bug 14437 - CRASH: RTÉ video crashes Safari
Summary: CRASH: RTÉ video crashes Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Anders Carlsson
URL: http://www.rte.ie/news/
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-06-27 14:41 PDT by Nicholas Shanks
Modified: 2007-07-05 21:36 PDT (History)
3 users (show)

See Also:


Attachments
crash log number 1 (33.77 KB, text/plain)
2007-06-27 15:09 PDT, Nicholas Shanks
no flags Details
crash log number 2 (29.19 KB, text/plain)
2007-06-27 15:10 PDT, Nicholas Shanks
no flags Details
test case (DRT-only) (791 bytes, text/html)
2007-06-28 11:54 PDT, Alexey Proskuryakov
no flags Details
proposed fix (3.20 KB, patch)
2007-06-29 02:24 PDT, Maxime BRITTO
darin: review-
Details | Formatted Diff | Diff
alternate fix (4.39 KB, patch)
2007-07-02 03:13 PDT, Maxime BRITTO
andersca: review-
Details | Formatted Diff | Diff
Tweaked initial patch (4.95 KB, patch)
2007-07-03 11:52 PDT, Anders Carlsson
darin: review-
Details | Formatted Diff | Diff
Address comments (6.55 KB, patch)
2007-07-03 12:57 PDT, Anders Carlsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Shanks 2007-06-27 14:41:42 PDT
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.
Comment 1 Nicholas Shanks 2007-06-27 15:09:36 PDT
Created attachment 15279 [details]
crash log number 1
Comment 2 Nicholas Shanks 2007-06-27 15:10:31 PDT
Created attachment 15280 [details]
crash log number 2
Comment 3 Alexey Proskuryakov 2007-06-28 10:29:52 PDT
Interestingly, I still get this crash if I remove RealPlayer plugin, although Safari does complain about it being unavailable.
Comment 4 Alexey Proskuryakov 2007-06-28 11:54:50 PDT
Created attachment 15295 [details]
test case (DRT-only)
Comment 5 Alexey Proskuryakov 2007-06-28 12:08:44 PDT
Anders asked to assign this bug to him :)
Comment 6 Maxime BRITTO 2007-06-29 00:48:51 PDT
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.
Comment 7 Alexey Proskuryakov 2007-06-29 01:16:53 PDT
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.
Comment 8 Maxime BRITTO 2007-06-29 01:31:53 PDT
(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.
Comment 9 Maxime BRITTO 2007-06-29 02:24:31 PDT
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 10 mitz 2007-06-29 02:42:54 PDT
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?
Comment 11 Darin Adler 2007-06-30 08:09:43 PDT
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".
Comment 12 Maxime BRITTO 2007-07-02 03:13:17 PDT
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.
Comment 13 Alexey Proskuryakov 2007-07-02 12:04:35 PDT
(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).
Comment 14 Anders Carlsson 2007-07-03 11:17:21 PDT
(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. 

Comment 15 Anders Carlsson 2007-07-03 11:24:51 PDT
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.
Comment 16 Anders Carlsson 2007-07-03 11:52:00 PDT
Created attachment 15371 [details]
Tweaked initial patch

Here's the first patch with some added asserts.
Comment 17 Anders Carlsson 2007-07-03 12:01:17 PDT
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.
Comment 18 Darin Adler 2007-07-03 12:28:30 PDT
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.
Comment 19 Anders Carlsson 2007-07-03 12:57:26 PDT
Created attachment 15375 [details]
Address comments
Comment 20 Darin Adler 2007-07-03 13:08:14 PDT
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
Comment 21 Anders Carlsson 2007-07-03 13:17:31 PDT
Committed revision 23950.
Comment 22 David Kilzer (:ddkilzer) 2007-07-05 21:36:12 PDT
<rdar://problem/5316292>