433432 - Crashes in [@ -[ChildView processPluginKeyEvent:] ]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mats Palmgren (inactive)]

It's #10 or so on MacOSX.

Example crash report:
bp-dc44666b-2044-11dd-b6c8-001cc45a2ce4

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I haven't tried to reproduce the crash myself, but looking at the
code I'm guessing the problem is the same as in bug 402505.
Retaining 'self' and null-checking 'mGeckoChild' should fix the
crash.  Also, break out of the loop to reach ::ReleaseEvent(cloneEvent)
before returning.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Sorry, this was a careless mistake on my part :-(

Thanks for catching it, Mats.

I suspect it's too late to block RC1 for this, but it should be
included in RC2 (if there is one).

This patch is (basically) no-risk, and fixes a fairly serious problem.

Comment 3

[Mike Schroepfer]

(In reply to comment #2)
> Sorry, this was a careless mistake on my part :-(
> 
> Thanks for catching it, Mats.
> 
> I suspect it's too late to block RC1 for this, but it should be
> included in RC2 (if there is one).
> 

Does it require us to spin an RC2?

> This patch is (basically) no-risk, and fixes a fairly serious problem.

(basicially) No-risk can only mean automated or manual test coverage for all possible code paths, plus data showing this doesn't introduce any leaks (kungfudeathgrip in there) or change perf.  Do we have any of those?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> Does it require us to spin an RC2?

No, I don't think so.

> Do we have any of those?

nsAutoRetainCocoaObject kungFuDeathGrip() is used in quite a few other places, has been used for some time (a couple of months if I remember right), and has introduced no problems that I'm aware of (leaks or perf problems).

But no, there's no Cocoa-widget-specific test coverage.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

By the way, I found 22 instances of this crash in the last two weeks:

http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=signature&query_type=exact&platform=mac&branch=1.9&signature=-%5BChildView+processPluginKeyEvent%3A%5D&query=-%5BChildView+processPluginKeyEvent%3A%5D&range_value=14

We'll have to see what happens when RC1 is released ... but so far this probably isn't a topcrasher.

Comment 6

[Mike Schroepfer]

(In reply to comment #5)
> By the way, I found 22 instances of this crash in the last two weeks:
> 
> http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=signature&query_type=exact&platform=mac&branch=1.9&signature=-%5BChildView+processPluginKeyEvent%3A%5D&query=-%5BChildView+processPluginKeyEvent%3A%5D&range_value=14
> 
> We'll have to see what happens when RC1 is released ... but so far this
> probably isn't a topcrasher.
> 

Ok thanks - really helpful.  I'm going to take this off the nomination list.  Has [RC2?] on the whiteboard so we'll catch it if we need to do an RC2.  

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, this crash accounts for roughly half of Camino's trunk Talkback (the other half is bug 433644), so we'll need it fixed before we do a Camino release off of 1.9.

Comment 8

[Mats Palmgren (inactive)]

roc, sr please? (in case there is a RC2)

Comment 9

[Mats Palmgren (inactive)]

FWIW, it's currenty at #12 on the "Firefox 3.0" top crash list
for the last 2 weeks, with 389 reported crashes in that time period.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 320627 [details] [diff] [review]
Patch rev. 1

Low-risk crash fix.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 320627 [details] [diff] [review]
Patch rev. 1

a=beltzner, please land on the cvs root asap

Comment 12

[Mats Palmgren (inactive)]

Landed on CVS trunk for RC2 / 3.0.1:
mozilla/widget/src/cocoa/nsChildView.mm 	1.357 

Keeping bug open until mozilla-central opens for checkins.

Comment 13

[Josh Aas]

we are going to auto-merge before mozilla-central opens, no need to leave this bug open for a landing on mozilla-central

Comment 14

[Mats Palmgren (inactive)]

Ok, thanks.

-> FIXED