Last Comment Bug 328044 - browser asserts on startup in JS_EndRequest with garbage cx->requestDepth
: browser asserts on startup in JS_EndRequest with garbage cx->requestDepth
Status: RESOLVED FIXED
[tcn-dl]
: crash, fixed1.8.0.2, fixed1.8.1, regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Phil Schwartau
Mentors:
Depends on:
Blocks: 265740
  Show dependency treegraph
 
Reported: 2006-02-21 07:10 PST by Robert Longson
Modified: 2006-03-02 11:45 PST (History)
3 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.74 KB, patch)
2006-02-21 07:15 PST, Robert Longson
no flags Details | Diff | Splinter Review
diff -w (2.28 KB, patch)
2006-02-21 07:15 PST, Robert Longson
no flags Details | Diff | Splinter Review
correction (5.60 KB, patch)
2006-02-21 07:52 PST, Robert Longson
no flags Details | Diff | Splinter Review
diff -w (2.29 KB, patch)
2006-02-21 07:53 PST, Robert Longson
no flags Details | Diff | Splinter Review
Clean fix, good for everywhere (5.14 KB, patch)
2006-02-22 01:25 PST, Blake Kaplan (:mrbkap)
brendan: review+
brendan: superreview+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description Robert Longson 2006-02-21 07:10:02 PST
When I start firefox from a recent trunk build it crashes on startup. 
A clean rebuild and new profile have not solved this.

Each time the browser starts JS_EndRequest asserts on cx->requestDepth.

It seems to me as if the problem is in nsXPCComponents_Utils::EvalInSandbox:
1. This function calls JS_NewContext to create a new context on the heap.
2. It then creates an AutoJSRequestWithNoCallContext object on the stack, passing this the previously creeated context.
3. At the end of this function the context is destroyed by calling JS_DestroyContextNoGC.
4. Finally the stack is cleaned up when the function exits which calls the destructor on the AutoJSRequestWithNoCallContext. The destructor does not have a valid context to work on as it was destroyed in step 3.

It looks like this could be fixed by ensuring the AutoJSRequestWithNoCallContext object destructor fires before the JS_DestroyContextNoGC call.
Comment 1 Robert Longson 2006-02-21 07:15:07 PST
Created attachment 212585 [details] [diff] [review]
patch
Comment 2 Robert Longson 2006-02-21 07:15:31 PST
Created attachment 212586 [details] [diff] [review]
diff -w
Comment 3 Robert Longson 2006-02-21 07:52:38 PST
Created attachment 212591 [details] [diff] [review]
correction

The original did not have identical functionality if data == NULL.
Comment 4 Robert Longson 2006-02-21 07:53:07 PST
Created attachment 212592 [details] [diff] [review]
diff -w
Comment 5 Jesse Ruderman 2006-02-22 00:25:00 PST
Timeless thinks this is a regression from bug 265740.  That bug is blocking Gecko 1.8.0.2, so this should too.  Nominating.

Adding crash keyword because I think this prevents starting a debug Firefox if you have any universal Greasemonkey scripts.  (Should JavaScript Engine assertions have the keyword "assertion" or the keyword "crash"?  They differ from other assertions in that they are fatal.)

This might be fixed by the patch in bug 328161.  This bug also has a patch, so I'm not just duping.
Comment 6 Blake Kaplan (:mrbkap) 2006-02-22 00:58:20 PST
The patch in bug 328161 fixes this in a _much_ cleaner way.
Comment 7 Blake Kaplan (:mrbkap) 2006-02-22 01:25:30 PST
Created attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

Okay, this gets the request stuff right and is good for the branch as well as the trunk (the other patch uses an added API that doesn't exist on the 1.8 branch). It also makes use of C++'s feature that destructors run in reverse order of creation.
Comment 8 Daniel Veditz [:dveditz] 2006-02-22 11:29:34 PST
Is there a testcase QA could use to verify when this has been fixed?
Comment 9 Blake Kaplan (:mrbkap) 2006-02-22 11:49:16 PST
Fix checked into trunk. I'm still hoping that brendan will stamp the r+sr he gave me in person in the bug.
Comment 10 Brendan Eich [:brendan] 2006-02-22 12:26:27 PST
Comment on attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

Blake won't mark my stamp for me based on my saying so in his cube, wahhhh.

/be
Comment 11 Blake Kaplan (:mrbkap) 2006-02-23 11:30:11 PST
Comment on attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

This is needed for bug 265740.
Comment 12 Daniel Veditz [:dveditz] 2006-02-23 15:33:09 PST
Comment on attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

approved for 1.8.0 branch, a=dveditz
Comment 13 Blake Kaplan (:mrbkap) 2006-02-23 18:13:04 PST
Most of this fix (sans the request stuff, see my comment in bug 265740) was checked into the 1.8 branches.
Comment 14 Dave Liebreich [:davel] 2006-03-02 11:45:50 PST
please provide testcase and/or testing guidance for this fix.

Note You need to log in before you can comment on or make changes to this bug.