332204 - Ensure we don't crash if aEvent is null for nsEventDispatcher::Dispatch

Status: RESOLVED WONTFIX

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jonathan Watt [:jwatt]]

nsEventDispatcher::Dispatch has an assertion to check that aEvent is not null, but no check of the argument for release builds. It seems to me that assertions are useful when a program may continue to run without a serious error being detected. In this case though, we'll just crash straight away so there's not much risk of that. However, I don't think we should crash, we should just return an nsresult failure code if somehow a null argument slips through on a rare occasion. Certainly in release builds. If we do that then the assertion is useful.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Return before we crash in the NS_ENSURE_TRUE macro following the assertion.

Comment 2

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

Comment on attachment 216732 [details] [diff] [review]
patch

I don't see any reason for this. If someone is dispatching an event without nsEvent, something is *badly* wrong and it is "ok" to crash.
nsEventDispatcher::Dispatch
is anyway only for Gecko internal usage, and it is the caller which should be fixed.

Comment 3

[Jonathan Watt [:jwatt]]

I'm aware that nsEventDispatcher is for gecko internal use only, but bugs are present in gecko and of course future changes can introduce new ones. I absolutely agree that if there's a bug the gecko caller should be fixed. That's why the assertion is there - to flag the error up in a debug build so we fix it. However, we can't guarantee that all present and future code paths will be exercised before each release build goes out. It is possible that a bug could slip through into a release build, and I don't agree that in a *release* build it is okay to crash on this. I see no reason to loose all open windows, tabs, history, form data I'm filling out, chatzilla session etc. just because some event couldn't be dispatched because a rare code path was exercised and aEvent turned out to be null. Please reconsider. :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think I agree with smaug here.  The fact that the event is not allowed to be null is _very_ clearly documented.  Doing safety null-checks for entry points into content or for cases where the codepath is not clear is ok, but the codepath here is very clear -- one creates an event and then dispatches it.  If event creation failed, there's nothing to dispatch, and all callers should be checking that.  So the check here would be redudant, imo.

Did you actually run into a case where a null nsEvent came in?

Comment 5

[Jonathan Watt [:jwatt]]

No, I didn't. I guess it was just a reaction from looking at the talkback server and thinking "crap! are we really at the 17 million mark? we need to try harder to cover all the bases!".

I'm out voted though, so I won't push this any further. :-)