167228 - possible crash in nsEventStateManager::PreHandleEvent

Status: VERIFIED INVALID

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

Description

[Boris Zbarsky [:bzbarsky]]

If I click a link that opens a new window, Mozilla crashes.  This is a
regression from version 1.374 of nsEventStateManager.cpp.  The code that crashes
is in the NS_DEACTIVATE of nsEventStateManager::PreHandleEvent :

      nsCOMPtr<nsIFocusController> focusController =
getter_AddRefs(GetFocusControllerForDocument(mDocument));
        if (focusController) {
          // Suppress the command dispatcher.
          focusController->SetSuppressFocus(PR_TRUE, "Deactivate Suppression");
        }

      // Now fire blurs.  We have to fire a blur on the focused window
      // and on the focused element if there is one.
      if (gLastFocusedDocument && gLastFocusedDocument == mDocument) {
        if (gLastFocusedContent) {
          // Blur the element.
          nsCOMPtr<nsIPresShell> shell;
          
          nsCOMPtr<nsIDOMElement> focusedElement;
          focusController->GetFocusedElement(getter_AddRefs(focusedElement));

This last line crashes because focusController is null.  It looks like we need
to wrap some more of that code in |if (focusController)| checks, but I don't
know enough about this code to tell what events we need to fire even when we
have no focusController.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing all sorts of ways to trigger the crash

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: more complete testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

I also see this crash about 50% of the time when I close context menus.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: real testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  Looks like the problem was that I had only picked up the newer versions of
_some_ of the files in bryner's patch.  Once I update all of them, things are
happy.  Donwgrading accordingly.

If focusController is never null whenever gLastFocusedDocument is non-null, this
is just invalid.  Otherwise, the lack of a null-check still seems suspect.

Comment 6

[Brian Ryner (not reading)]

I don't think that's possible, off the top of my head.  We can get at the focus
controller whenever the document has a DocShell, and that's certainly necessary
for it to be focused, so...

Comment 7

[Boris Zbarsky [:bzbarsky]]

verified.  Sorry for the trouble.