The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Evan DiBiase, Assigned: mrbkap)

Tracking

({crash, verified1.8.0.1, verified1.8.1})

1.8 Branch
mozilla1.9alpha1
crash, verified1.8.0.1, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.0.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 205765 [details]
Stack trace from OS X 10.4.3, Firefox 1.5
Keywords: crash

Comment 2

11 years ago
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()
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Summary: Crash when evaluating scripts in content loaded with Prototype Ajax → Crash when evaluating scripts in content loaded with Prototype Ajax [@ Variables]
Version: unspecified → 1.8 Branch
(Assignee)

Comment 3

11 years ago
We're crashing because cx->fp has a null varobj.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

11 years ago
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.

Updated

11 years ago
Flags: blocking1.8.0.1?
Regression from bug 285219 which exposed a latent bug: obj_eval needs to js_GetCallObject if !fp->varobj.  mrbkap is on the case.

/be
(Assignee)

Updated

11 years ago
Assignee: general → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Created attachment 205901 [details] [diff] [review]
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)}) })();
Attachment #205901 - Flags: review?(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
Attachment #205901 - Flags: review?(brendan)
Attachment #205901 - Flags: review+
Attachment #205901 - Flags: approval1.8.0.1?
(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
(Assignee)

Comment 9

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
(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.
could this have hurt Tp a tiny bit?

Comment 13

11 years ago
(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
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)
Status: RESOLVED → VERIFIED
This is biting AJAX hackers hard enough that I want it fixed in 1.5.0.1.

/be
Flags: blocking1.8.0.1? → blocking1.8.0.1+
*** Bug 320846 has been marked as a duplicate of this bug. ***
See http://wrath.rubyonrails.org/pipermail/rails-spinoffs/2005-December/001616.html.

/be

Comment 18

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

Comment 19

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

Comment 20

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

Comment 21

11 years ago
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-320172.js,v  <--  regress-320172.js
initial revision: 1.1
Flags: testcase+
Comment on attachment 205901 [details] [diff] [review]
Fix the crash

a=dveditz
Attachment #205901 - Flags: approval1.8.0.1? → approval1.8.0.1+
Comment on attachment 205901 [details] [diff] [review]
Fix the crash

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

b/e
Attachment #205901 - Flags: approval1.8.1+
(Assignee)

Comment 24

11 years ago
Checked into the branches.
Keywords: fixed1.8.0.1, fixed1.8.1
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.
Keywords: fixed1.8.0.1 → verified1.8.0.1

Comment 26

11 years ago
v 2006-01-11 1.8.0.1, 1.8.1, trunk windows/linux/mac
Keywords: fixed1.8.1 → verified1.8.1
*** Bug 325205 has been marked as a duplicate of this bug. ***

Comment 28

11 years ago
Basically taking out "var" keyword does it, but the bug is frustrating.
Crash Signature: [@ Variables]
You need to log in before you can comment on or make changes to this bug.