nsCxPusher sometimes fails to trigger termination functions

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

In the first testcase in bug 336605, in my builds, when the event fires, the window doesn't actually close. Instead it closes the next time I click in the window.

The problem is in nsCxPusher:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsContentUtils.cpp#2353
If the incoming stack is non-empty, then we suppress firing ScriptEvaluated(PR_TRUE) in the Pop. But in this case, the stack contains a chrome JS context, while we're pushing a content JS context ... so in fact the script in the content JS context *has* terminated and we should call ScriptEvaluated(PR_TRUE).
Created attachment 220861 [details] [diff] [review]
fix

This fixes it by only setting mScriptIsRunning if the pushed context is already on the stack.
Attachment #220861 - Flags: superreview?(bugmail)
Attachment #220861 - Flags: review?(bugmail)
I think it'd be better if jst could review this, I don't feel I know the code well enough for a r+sr. I could still sr though if you want.
Attachment #220861 - Flags: superreview?(jst)
Attachment #220861 - Flags: superreview?(bugmail)
Attachment #220861 - Flags: review?(jst)
Attachment #220861 - Flags: review?(bugmail)
Comment on attachment 220861 [details] [diff] [review]
fix

r+sr=jst
Attachment #220861 - Flags: superreview?(jst)
Attachment #220861 - Flags: superreview+
Attachment #220861 - Flags: review?(jst)
Attachment #220861 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 5

12 years ago
weirdal crashed in this new function.

Updated

12 years ago
Blocks: 340651

Comment 6

12 years ago
I see crashes too with this test case
https://bugzilla.mozilla.org/attachment.cgi?id=224680&action=view
You may have to try it few times.
As far as I see, the problem is that IsContextOnStack doesn't handle
null contexts, which are pushed to stack for example in
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#5780

Comment 7

12 years ago
Created attachment 224689 [details] [diff] [review]
null check
Attachment #224689 - Flags: superreview?(jst)
Attachment #224689 - Flags: review?(jst)

Comment 8

12 years ago
smaug: mrbkap stuck the null check into bug 340602
Blocks: 340602

Updated

12 years ago
Attachment #224689 - Flags: superreview?(jst)
Attachment #224689 - Flags: review?(jst)
No longer blocks: 340602
Depends on: 340602
No longer blocks: 340651
Depends on: 340651
You need to log in before you can comment on or make changes to this bug.