432275 - Regression - Java applets crashing browser [@ obj_eval]

Status: RESOLVED FIXED

Description

[cdrevland]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050504 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050504 Minefield/3.0pre

I noticed this on www.thaibg.com
When loading the java applets, the browser crashes instantly. This works in all other browsers including Fx2, so this should be considered a regression.

Reproducible: Always

Steps to Reproduce:
1. Go to www.thaibg.com
2. Click one of the board games, for example "Go" 
3. Wait for browser to die
Actual Results:  
The fox is brutally murdered.

Expected Results:  
The applet will load without problems.

I was using Mac OS X Leopard, and the latest nightly(see build information).
But I got confirmed that all platforms share the same error.

Comment 1

[Marcia Knous [:marcia - use ni]]

Confirming the crash using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050604 Minefield/3.0pre. I see the crash on Windows Vista as well. IE seems to render the game OK, but it takes some time to load.

Comment 2

[Marcia Knous [:marcia - use ni]]

http://crash-stats.mozilla.com/report/index/c3da3454-1b89-11dd-94f2-0013211cbf8a is my crash report on Mac. 

Comment 3

[Brian Crowder]

Created attachment 319599 [details] [diff] [review]
null-check

Igor and I audited all the other references to regs in the source, this seems to be the only obvious null-deref.  We got faked out here by liveconnect creating a "psuedo"-frame without setting up regs.

Comment 4

[Brian Crowder]

Comment on attachment 319599 [details] [diff] [review]
null-check

This is a bad bug with an easy/safe fix.

Comment 5

[Blake Kaplan (:mrbkap)]

Created attachment 319616 [details] [diff] [review]
Additional patch to make LC's pseudo-frame more valid again

I argued for this patch on IRC in order to restore honor to liveconnect's pseudo-frame. This patch makes liveconnect mirror js_set_watch in the appropriate codepath. crowder has tested to ensure that this patch alone *also* fixes this bug, and I would like both patches for belt-and-braces.

Comment 6

[Blake Kaplan (:mrbkap)]

Comment on attachment 319616 [details] [diff] [review]
Additional patch to make LC's pseudo-frame more valid again

This is defense-in-depth against liveconnect-related crashes.

Comment 7

[Igor Bukanov]

Comment on attachment 319616 [details] [diff] [review]
Additional patch to make LC's pseudo-frame more valid again

>Make liveconnect's pseudo-frame initialize regs. bug 432275
>
>diff --git a/js/src/liveconnect/nsCLiveconnect.cpp b/js/src/liveconnect/nsCLiveconnect.cpp
>--- a/js/src/liveconnect/nsCLiveconnect.cpp
>+++ b/js/src/liveconnect/nsCLiveconnect.cpp
>@@ -107,6 +107,7 @@ private:
>     nsCOMPtr<nsIJSContextStack> mContextStack;
>     JSContext*                  mContext;
>     JSStackFrame                mFrame;
>+    JSFrameRegs                 mRegs;
>     nsresult                    mPushResult;
> };
> 
>@@ -185,11 +186,16 @@ AutoPushJSContext::AutoPushJSContext(nsI
> 
>             if (fun)
>             {
>+                JSScript *script = JS_GetFunctionScript(cx, fun);
>                 mFrame.fun = fun;
>-                mFrame.script = JS_GetFunctionScript(cx, fun);
>+                mFrame.script = script;
>                 mFrame.callee = JS_GetFunctionObject(fun);
>                 mFrame.scopeChain = JS_GetParent(cx, mFrame.callee);
>                 mFrame.down = cx->fp;
>+                mRegs.pc = script->code + script->length
>+                           - JSOP_STOP_LENGTH;
>+                mRegs.sp = NULL;

In case if at some point in future JSFrameRegs will be extended I suggests to add here:

JS_STATIC_ASSERT(sizeof mRegs == sizeof mRegs.pc + sizeof mRegs.sp);

r+ with this extra check.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319599 [details] [diff] [review]
null-check

a1.9=beltzner

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319616 [details] [diff] [review]
Additional patch to make LC's pseudo-frame more valid again

a1.9=beltzner

Comment 10

[Brian Crowder]

jsobj.c: 3.472
nsCLiveConnect.cpp: 1.52 (hoping mrbkap won't mind my landing his patch)