336674 - nsCxPusher sometimes fails to trigger termination functions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it by only setting mScriptIsRunning if the pushed context is already on the stack.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 220861 [details] [diff] [review]
fix

r+sr=jst

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 5

[timeless]

weirdal crashed in this new function.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

Attachment: null check

Comment 8

[timeless]

smaug: mrbkap stuck the null check into bug 340602