Closed Bug 333669 Opened 14 years ago Closed 10 years ago
Dereferencing null disp
Data in ns Event Listener Manager::Handle Event
If we have a NS_USER_DEFINED_EVENT we can end up dereferencing null pointer dispData.
Bryner, do you know what this was supposed to do in this case?
I think DispatchToInterface isn't ever called when message == NS_USER_DEFINED_EVENT. HandleEventSubType is used for those events.
If that's the case, we should assert so at the beginning of the method.
Marking wanted-1.9 for someone to figure out if it's a false positive.
Flags: blocking1.9a1? → blocking1.9-
It is a false positive because typeData is also null so DispatchToInterface won't be called. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventListenerManager.cpp&rev=1.229&mark=1682,1683,1685,1727,1729#1682
From what I see, the lines linked by smaug above are now http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventListenerManager.cpp&rev=1.277&mark=1142,1207#1142 and there's no check for what he was talking about, so all the rework done there might have exactly this crash come to the surface now. I have a stacktrace for crashing exactly in that function call, I'll attach it shortly.
Here's my stacktrace for this.
Status: NEW → ASSIGNED
The patch checks both |typeData| and |dispData|. Even though currently either they're both null or they're both non-null, checking for both makes sure no regressions would occur in future.
Comment on attachment 273223 [details] [diff] [review] Simple fix I'm not the right reviewer for this.
Attachment #273223 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 273223 [details] [diff] [review] Simple fix So what would this fix? As you said, "they're both null or they're both non-null" The crash KaiRo saw could be something else. Badly implemented eventhandler?
Comment on attachment 273223 [details] [diff] [review] Simple fix And if they both are not null, that is something we'd might want to assert but not have checks in opt-builds, imo, because that means the bug is somewhere else.
(In reply to comment #11) > (From update of attachment 273223 [details] [diff] [review]) > So what would this fix? > As you said, "they're both null or they're both non-null" When they're both null, the |DispatchToInterface| call tries to dereference both through |dispData->method| and |typeData->iid|, which could lead to a crash.
EVENT_TYPE_DATA_EQUALS has null checks, so useTypeInterface should be false whenever typeData is null.
Comment on attachment 273223 [details] [diff] [review] Simple fix (In reply to comment #14) > EVENT_TYPE_DATA_EQUALS has null checks, so useTypeInterface should > be false whenever typeData is null. > Hmmm that's right. So, should we close this bug as INVALID?
Attachment #273223 - Attachment is obsolete: true
So the |if (typeData)| referenced in comment #5 in the old code was useless and I did "correctly" crash? It's OK if I did lose 50 lines of typed test in an HTML form when a keypress event crashed, just because you tell me I can't crash there? Sounds somehow strange to me. I thought it's better to not crash in any case, even if a function gets called with "wrong" parameters...
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
QA Contact: ian → events
Yes, sometimes when callers give you bad arguments, crashing is the right thing to do. You can fix the caller, right?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.