320172 - Crash when evaluating scripts in content loaded with Prototype Ajax [@ Variables]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Evan DiBiase]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/416.12 (KHTML, like Gecko) Safari/416.13
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

When a DIV in a page is updated using the Prototype library's Ajax.Updater with evalScripts set to true, and the updated content contains inline JavaScript, Firefox crashes. We've tried this on Mac OS X on both 1.0.7 and 1.5, and on Windows XP 1.0.7 and 1.5, and on Linux 1.0.7.

Firefox does not crash if the response text from the Ajax call does not contain inline JavaScript. Additionally, the actual inline JavaScript call does not seem to matter; the crash will occur with a simple "var x = 5;" or more complex code.

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.peakstrategy.net/firefox_rules/ (also works if the files are local)
2. Wait for the Ajax call to complete.

Actual Results:  
Firefox crashes.

Expected Results:  
The DIV would be updated with the response text of the Ajax call, or, in the worst case, Firefox would indicate that there was a problem in the Prototype library or in our use of it, without crashing.

This crash has been filed as several Talkback incidents, including TB12925669Z.

Comment 1

[Evan DiBiase]

Attachment: Stack trace from OS X 10.4.3, Firefox 1.5

Comment 2

[timeless]

Incident ID: 12925669 
Stack Signature Variables() 8871b9bc 
Product ID Firefox15 
Build ID 2005111116 
Trigger Time 2005-12-13 14:54:46.0 
Platform MacOSX 
Operating System Darwin 8.3.0 
Module libmozjs.dylib.1.0.0 + (0004df30) 
URL visited  
User Comments  
Since Last Crash 363 sec 
Total Uptime 1120859 sec 
Trigger Reason SIGBUS: Bus Error: (signal 10) 
Source File, Line No. /builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsparse.c, line 2164 
Stack Trace  

Variables()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsparse.c, line 2164]
Variables()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsparse.c, line 2143]
Statements()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsparse.c, line 1054]
js_CompileTokenStream()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsparse.c, line 469]
CompileTokenStream()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsapi.c, line 3581]
JS_CompileUCScriptForPrincipals()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsapi.c, line 3681]
obj_eval()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsobj.c, line 1236]
js_Invoke()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 1177]
js_Interpret()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 3526]
js_Invoke()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 1197]
js_Interpret()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 3526]
js_Invoke()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 1197]
js_Interpret()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 3526]
js_Invoke()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 1197]
js_InternalInvoke()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsinterp.c, line 1275]
JS_CallFunctionValue()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/js/src/jsapi.c, line 4159]
nsJSContext::CallEventHandler()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1413]
nsGlobalWindow::RunTimeout()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6298]
nsGlobalWindow::TimerCallback()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6656]
nsTimerImpl::Fire()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/xpcom/threads/nsTimerImpl.cpp, line 395]
handleTimerEvent()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/xpcom/threads/nsTimerImpl.cpp, line 462]
PL_HandleEvent()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/xpcom/threads/plevent.c, line 689]
PL_ProcessPendingEvents()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/xpcom/threads/plevent.c, line 623]
__CFRunLoopDoSources0()
__CFRunLoopRun()
CFRunLoopRunSpecific()
RunCurrentEventLoopInMode()
GetNextEventMatchingMask()
WNEInternal()
WaitNextEvent()
nsMacMessagePump::GetEvent()   nsMacMessagePump::DoMessagePump()   nsAppShell::Run()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsAppShell.cpp, line 114]
nsAppStartup::Run()   XRE_main()  [/builds/tinderbox/Fx-Mozilla1.8/Darwin_7.9.0_Depend/mozilla/toolkit/xre/nsAppRunner.cpp, line 2315]
_start()   start()

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

We're crashing because cx->fp has a null varobj.

Comment 4

[shutdown]

Here is the minimized testcase.

javascript: (function xxx(){ ["var x"].forEach(eval); })();

The eval'ed script is compiled with the scripted caller's
JSStackFrame, which is |xxx|'s one and has no varobj.

Comment 5

[Brendan Eich [:brendan]]

Regression from bug 285219 which exposed a latent bug: obj_eval needs to js_GetCallObject if !fp->varobj.  mrbkap is on the case.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the crash

I actually had basically this patch sitting in my tree for several hours today while I debugged it to see what was "wrong" (see my discussion below). This patch fixes the crash by providing a varobj for the compiler to use when parsing the variable declaration.

Notice that shutdown's reduced testcase throws a reference error with this patch, however. This is because forEach passes two arguments to its function argument, the current element and the index. This means that in the testcase, the call to eval actually looks like: |eval("var x", 0);|, where the 0 gets boxed and is used as the scope object, but the scope object doesn't have any relation to the scopeobj used by the function. To fix things the testcase could be modified to do:
(function xxx() { ["var x"].forEach(function(e, i){eval(e)}) })();

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 205901 [details] [diff] [review]
Fix the crash

r=me, this is a good null ptr deref crash fix to consider for 1.8.0.1, after baking on trunk for a little while.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> To fix things the testcase could be modified to do:
> (function xxx() { ["var x"].forEach(function(e, i){eval(e)}) })();

That will put the var x in the inner function (the anonymous lambda (e, i)).  Not what is wanted, perhaps -- or does it matter in the non-reduced testcase?

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Evan DiBiase]

Wow, everyone -- thanks so much for your quick response! I am quite impressed with the speed and ability of the Mozilla team.

Is there any sense of what Firefox releases this will make it in to? Given that it's been checked in to trunk, I'm assuming that the next release of 1.5 will have it, but I don't know if these kinds of things tend to get backported for people still on the 1.0 track.

If it's not likely to be backported, is there a workaround we can employ for users still on 1.0.x and 1.5?

Thank you again; I've always believed that open source development produced fast, quality fixes, and it's really cool to see that belief confirmed.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> Is there any sense of what Firefox releases this will make it in to?

We've nominated this fix to be included in the next "security and stability" release of Firefox (1.5.0.1, or Gecko 1.8.0.1) which we are planning to release in late January. If it doesn't meet the requirements for 1.8.0.1, it'll definitely be in Firefox 2.0, but that's going to be at least a few months off.

Since this is a non-exploitable crash, it is highly unlikely (unfortunately) for this fix to be backported to the 1.0.x branch.


> If it's not likely to be backported, is there a workaround we can employ for
> users still on 1.0.x and 1.5?

In prototype.js, I see:
  evalScripts: function() {
    return this.extractScripts().map(eval);
  },

Making this be:
  evalScripts: function() {
    return this.extractScripts().map(function(e) { eval(e) });
  }

should fix the crash.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

could this have hurt Tp a tiny bit?

Comment 13

[shutdown]

(In reply to comment #6)
> Notice that shutdown's reduced testcase throws a reference error with this
> patch, however. This is because forEach passes two arguments to its function
> argument, the current element and the index. This means that in the testcase,
> the call to eval actually looks like: |eval("var x", 0);|, where the 0 gets
> boxed and is used as the scope object, but the scope object doesn't have any
> relation to the scopeobj used by the function.

According to the spec (ECMA-262 3rd section 12.2), a variable declaration
without an initializer never references the declared variable. Hence,
|eval("var x", 0)| should not throw a ReferenceError. Right?
http://bclary.com/2004/11/07/ecma-262.html#a-12.2

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

No crash with SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051216 Mozilla/1.0

and/or Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051216 Firefox/1.6a1, both trunk.

Verified FIXED (crash only; comment 12 and comment 13 I leave to others)

Comment 15

[Brendan Eich [:brendan]]

This is biting AJAX hackers hard enough that I want it fixed in 1.5.0.1.

/be

Comment 16

[Brendan Eich [:brendan]]

*** Bug 320846 has been marked as a duplicate of this bug. ***

Comment 17

[Brendan Eich [:brendan]]

See http://wrath.rubyonrails.org/pipermail/rails-spinoffs/2005-December/001616.html.

/be

Comment 18

[roborahul]

see Prototype bug ticket if interested: http://dev.rubyonrails.org/ticket/3288

Comment 19

[Mike Schroepfer]

Please bake a few more days on trunk we'll approve.

Comment 20

[Mark Stosberg]

*** Bug 320870 has been marked as a duplicate of this bug. ***

Comment 21

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_6/Regress/regress-320172.js,v  <--  regress-320172.js
initial revision: 1.1

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 205901 [details] [diff] [review]
Fix the crash

a=dveditz

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 205901 [details] [diff] [review]
Fix the crash

mrbkap will get this into the branches, I have faith.

b/e

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the branches.

Comment 25

[Marcia Knous [:marcia]]

verified fixed on the branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. Using the STR listed in the bug I was not able to crash.

Comment 26

[Bob Clary [:bc] (inactive)]

v 2006-01-11 1.8.0.1, 1.8.1, trunk windows/linux/mac

Comment 27

[Daniel Veditz [:dveditz]]

*** Bug 325205 has been marked as a duplicate of this bug. ***

Comment 28

[Mark Sergienko]

Basically taking out "var" keyword does it, but the bug is frustrating.