328044 - browser asserts on startup in JS_EndRequest with garbage cx->requestDepth

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Robert Longson [:longsonr]]

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 [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Attachment: diff -w

Comment 3

[Robert Longson [:longsonr]]

Attachment: correction

The original did not have identical functionality if data == NULL.

Comment 4

[Robert Longson [:longsonr]]

Attachment: diff -w

Comment 5

[Jesse Ruderman]

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) (inactive)]

The patch in bug 328161 fixes this in a _much_ cleaner way.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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]]

Is there a testcase QA could use to verify when this has been fixed?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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]]

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) (inactive)]

Comment on attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

This is needed for bug 265740.

Comment 12

[Daniel Veditz [:dveditz]]

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) (inactive)]

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]]

please provide testcase and/or testing guidance for this fix.