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

RESOLVED FIXED

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Robert Longson, Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, fixed1.8.0.2, fixed1.8.1, regression
Points:
---
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tcn-dl])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 212585 [details] [diff] [review]
patch
(Reporter)

Comment 2

11 years ago
Created attachment 212586 [details] [diff] [review]
diff -w
(Reporter)

Updated

11 years ago
Attachment #212585 - Flags: review?(dbradley)
(Reporter)

Updated

11 years ago
Summary: browser asserts in JS_EndRequest with garbage cx->requestDepth → browser asserts on startup in JS_EndRequest with garbage cx->requestDepth
(Reporter)

Comment 3

11 years ago
Created attachment 212591 [details] [diff] [review]
correction

The original did not have identical functionality if data == NULL.
Attachment #212585 - Attachment is obsolete: true
Attachment #212591 - Flags: review?(dbradley)
Attachment #212585 - Flags: review?(dbradley)
(Reporter)

Comment 4

11 years ago
Created attachment 212592 [details] [diff] [review]
diff -w
Attachment #212586 - Attachment is obsolete: true

Comment 5

11 years ago
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.
Blocks: 265740
Severity: normal → critical
Flags: blocking1.8.0.2?
Keywords: crash, regression
(Assignee)

Comment 6

11 years ago
The patch in bug 328161 fixes this in a _much_ cleaner way.
(Assignee)

Comment 7

11 years ago
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.
Assignee: dbradley → mrbkap
Attachment #212591 - Attachment is obsolete: true
Attachment #212592 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #212714 - Flags: superreview?(brendan)
Attachment #212714 - Flags: review?(brendan)
Attachment #212591 - Flags: review?(dbradley)
Is there a testcase QA could use to verify when this has been fixed?
(Assignee)

Comment 9

11 years ago
Fix checked into trunk. I'm still hoping that brendan will stamp the r+sr he gave me in person in the bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
Attachment #212714 - Flags: superreview?(brendan)
Attachment #212714 - Flags: superreview+
Attachment #212714 - Flags: review?(brendan)
Attachment #212714 - Flags: review+
(Assignee)

Comment 11

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

This is needed for bug 265740.
Attachment #212714 - Flags: approval1.8.0.2?
Comment on attachment 212714 [details] [diff] [review]
Clean fix, good for everywhere

approved for 1.8.0 branch, a=dveditz
Attachment #212714 - Flags: approval1.8.0.2? → approval1.8.0.2+
Flags: blocking1.8.0.2? → blocking1.8.0.2+
(Assignee)

Comment 13

11 years ago
Most of this fix (sans the request stuff, see my comment in bug 265740) was checked into the 1.8 branches.
Keywords: fixed1.8.0.2, fixed1.8.1
please provide testcase and/or testing guidance for this fix.
Whiteboard: [tcn-dl]
You need to log in before you can comment on or make changes to this bug.