432556 - Null pointer de-reference in [ToolbarWindow _toolbarPillButtonClicked:]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

The [ToolbarWindow _toolbarPillButtonClicked:] method (in
nsCocoaWindow.mm) sets its geckoWindow local variable as follows:

  nsCocoaWindow *geckoWindow = [[self delegate] geckoWidget];

But if the current ToolbarWindow's widget has been destroyed, [self
delegate] will return nil, as will [[self delegate] geckoWidget].  So
geckoWindow will be nil, and a crash will happen when it's
de-referenced at the bottom of the method.

Breakpad has recorded a couple of these crashes in the last week:

http://crash-stats.mozilla.com/report/list?range_unit=weeks&query_search=signature&query_type=exact&platform=mac&branch=1.9&signature=-%5BToolbarWindow+_toolbarPillButtonClicked%3A%5D&query=-%5BToolbarWindow+_toolbarPillButtonClicked%3A%5D&range_value=1

The following two lines should be added after the line that sets
geckoWindow:

  if (!geckoWindow)
    return;

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Josh and Stuart:  This fix is a no-brainer, and will need to be done
at some point anyway.  It's also possible that it could resolve bug
432005.  Do you think it's worth trying to get it landed quickly, so
that the guy who reported bug 432005 can try it in a nightly?

Comment 2

[Stuart Morgan]

(In reply to comment #1)
> It's also possible that it could resolve bug 432005.

I don't see how, since it should never be possible to create a ToolbarWindow (or any of the other custom window classes used in XUL apps) in Camino.

Comment 3

[Josh Aas]

Can we please get an actual patch on this bug with review flags set instead of a description of the patch?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

>> It's also possible that it could resolve bug 432005.
>
> I don't see how, since it should never be possible to create a
> ToolbarWindow (or any of the other custom window classes used in XUL
> apps) in Camino.

Oops, you're right.  Sorry for the confusion.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here it is.

Comment 6

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

Comment on attachment 319815 [details] [diff] [review]
Fix

trivial fix, no risk

Comment 7

[Josh Aas]

landed on trunk