Closed Bug 347480 Opened 18 years ago Closed 17 years ago

Potentially incorrect script context used for event handlers

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: markh, Assigned: smaug)

Details

Attachments

(1 file)

nsJSEventListener::HandleEvent potentially is using an incorrect script context when executing an event handler.

A comment to this effect was first introduced in http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/dom/src/events&command=DIFF_FRAMESET&file=nsJSEventListener.cpp&rev2=1.17&rev1=1.16

[Look for the comment starting '// XXX This doesn't seem like the correct context']

The DOM_AGNOSTIC landing has moved some of this code - look for a FIXME comment referencing this bug in nsJSEventListener.cpp.

See also bug 339649 for another place where the context may be incorrect.
Flags: blocking1.9?
Seems like we should figure out what's going on here for 1.9. Assigning to smaug for now.
Assignee: events → Olli.Pettay
Flags: blocking1.9? → blocking1.9+
Smaug - have you had a chance to look into this?
Haven't thought about this too much yet, but currently I think
using mContext is the right thing to do.
Can anyone think of reason why the context could be wrong?
When nsJSEventListener::HandleEvent is called there is a context pushed to stack. 
That context is taken from the scriptglobalobject of the ownerdocument of the 
currentTarget.
Similarly when nsEventListenerManager::AddScriptEventListener creates 
nsIJSEventListener, it gets the context from the object to which the listener
is registered. So the context should be the same.
The only case when context taken from the stack and mContext might differ seems to
be the case that bug 339649 describes (and hopefully it will be fixed there).

So I'd just remove the comment in nsJSEventListener.
Attachment #271992 - Flags: superreview?(jst)
Attachment #271992 - Flags: review?(jst)
Comment on attachment 271992 [details] [diff] [review]
Remove XXX but add an assertion

Yeah, makes sense to me.
Attachment #271992 - Flags: superreview?(jst)
Attachment #271992 - Flags: superreview+
Attachment #271992 - Flags: review?(jst)
Attachment #271992 - Flags: review+
Attachment #271992 - Flags: approval1.9?
Attachment #271992 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: