496582 - nsCocoaWindow_NSWindow_sendEvent should not assume |self| will remain valid

Status: RESOLVED WORKSFORME

(Core :: Widget: Cocoa, defect)

Description

[Stuart Morgan]

An early crash from Camino 2.0b3:
http://crash-stats.mozilla.com/report/index/c67985f4-f77e-4c83-bb33-b08d82090604
shows a crash under _freedHandler in
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsCocoaWindow.mm&rev=1.151&mark=2155#2155

Given that, it looks like it's not safe to assume that the window will still be valid after [self nsCocoaWindow_NSWindow_sendEvent:anEvent];

If nothing else there should be a [self retain]; just before calling through, and a [self release] at the end of the method (I'd rather it not be an autorelease, since there should be as little impact on object lifetimes as possible).

Since this is a regression caused by the swizzling, I'd really like to see it fixed in 1.9.0.x

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> there should be a [self retain]; just before calling through, and a
> [self release] at the end of the method

In light of your stack, I think this is a reasonable suggestion.  I'll
post a patch that does this.

This bug isn't "caused by method swizzling" -- it's caused by
the assumption that an NSWindow will always be valid after a call to
[NSWindow sendEvent:], which your stack shows to be false.

Comment 2

[Stuart Morgan]

(In reply to comment #1)
It helps if you don't misquote me. I did not say "caused by method swizzling", I said "caused by *the* swizzling", as in "caused by the swizzling I just mentioned two sentences ago, so I assume the reference is painfully clear".

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Trunk patch

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: 1.9.0-branch patch

Comment 5

[Josh Aas]

Comment on attachment 381830 [details] [diff] [review]
Trunk patch

Maybe use nsAutoRetainCocoaObject?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> Maybe use nsAutoRetainCocoaObject?

Fine with me.  New patch coming up shortly.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Trunk patch v1.1 (follow Josh's suggestion)

Comment 8

[Josh Aas]

Comment on attachment 381831 [details] [diff] [review]
1.9.0-branch patch

We should probably do autoretain on 1.9.0 too, adding all necessary code.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: 1.9.0-branch patch v1.1 (follow Josh's suggestion)

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 381855 [details] [diff] [review]
Trunk patch v1.1 (follow Josh's suggestion)

Oops.  This has been made obsolete by the patch for bug 178324, which
got rid of nsCocoaWindow_NSWindow_sendEvent.

Comment 11

[Samuel Sidler (old account; do not CC)]

Is there anywhere else this patch should land, like 1.9.1?

Comment 12

[Steven Michaud [:smichaud] (Retired)]

It should probably also land on the 1.9.1 branch.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 382323 [details] [diff] [review]
1.9.0-branch patch v1.1 (follow Josh's suggestion)

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 382323 [details] [diff] [review]
1.9.0-branch patch v1.1 (follow Josh's suggestion)

Landed on the 1.9.0 branch:
Checking in widget/src/cocoa/nsCocoaWindow.mm;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v  <--  nsCocoaWindow.mm
new revision: 1.152; previous revision: 1.151
done

Comment 15

[Samuel Sidler (old account; do not CC)]

This isn't really RESOLVED FIXED since it needs to land on 1.9.1 as well and Bugzilla doesn't really have a way to say "this bug is for two branches but not for trunk". So I tend to prefer such bugs staying open until they're fixed in both places.

Comment 16

[Samuel Sidler (old account; do not CC)]

Fixed this in 1.9.0.12, so we should fix it in 1.9.1.x, maybe 1.9.1.1?

Comment 17

[Samuel Sidler (old account; do not CC)]

Not going to block 1.9.1.1 on this, but Steven if you can work up a 1.9.1 patch, we'll look at it for approval.