641057 - [Mac] Firefox 4.0 crash [@ -[ChildView updatePluginFocusStatus:] ]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

There are a bunch of these at https://crash-stats.mozilla.com/.
There've been 24 in the last week, which puts this crash at #17 in the
list of Mac topcrashers.

https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A4.0&platform=mac&query_search=signature&query_type=exact&query=&date=03%2F11%2F2011%2010%3A36%3A53&range_value=1&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=-%5BChildView%20updatePluginFocusStatus%3A%5D

They're all null-dereferences at the following line:

http://hg.mozilla.org/releases/mozilla-2.0/annotate/5f8f494a4c29/widget/src/cocoa/nsChildView.mm#l5944

Which shows that the NS_QUERY_DOM_WIDGET_HITTEST event sent by
[ChildView currentEventShouldFocusPlugin] can cause our widget to be
deleted, nulling out mGeckoChild.

I'll post a patch in my next comment.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Comment 2

[Mats Palmgren (inactive)]

It's been a couple of years since I looked at this code, but don't you need
  nsAutoRetainCocoaObject kungFuDeathGrip(self);
before that call to ensure it's safe to peek at the mGeckoChild member?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Yes, that's probably a good idea.  And I see there are already several places in nsChildView.mm where "nsAutoRetainCocoaObject kungFuDeathGrip(self);" is added before dispatching a Gecko event, if mGeckoChild is to be checked afterwards.

New patch coming up.

Thanks :-)

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (follow Mats' suggestion)

Comment 5

[Marcia Knous [:marcia]]

Two comments in crash stats indicate the following:

I was working on my farm on Facebook, I toggled the full screen tab and crashed.

Refreshing FB page that had flash content

Comment 6

[Scoobidiver (away)]

It is #7 top crasher on Mac OS X in 4.0.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Unbitrot my rev1 patch

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Unbitrotted rev1 patch that's easier for someone else to land

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Ehsan landed this for me on cedar:
http://hg.mozilla.org/projects/cedar/rev/4c7d52ed5ccd

It should get merged to mozilla-central later today.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

My rev1 patch just landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/4c7d52ed5ccd

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I'm tentatively marking this status2.0.1-fixed+

But (with our new arrangements) I don't know whether this is enough by itself to get my patch into the next release on the 2.0 branch.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> But (with our new arrangements) I don't know whether this is enough
> by itself to get my patch into the next release on the 2.0 branch.

Actually it probably isn't, since we're still using the old system for
Firefox 4.0.X.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 523354 [details] [diff] [review]
Unbitrotted rev1 patch that's easier for someone else to land

This is a trivial patch (a NULL check) that fixes a Mac topcrasher.
It has zero risk.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4c7d52ed5ccd

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 523354 [details] [diff] [review]
Unbitrotted rev1 patch that's easier for someone else to land

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 523354 [details] [diff] [review]
Unbitrotted rev1 patch that's easier for someone else to land

Landed on the 2.0 branch:
http://hg.mozilla.org/releases/mozilla-2.0/rev/12107407598c