640110 - ChildView uses outdated pointer of plugin event for nsGUIEvent

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Patch v1.0

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#5849

5849   nsKeyEvent geckoEvent(PR_TRUE, message, nsnull);
5850   [self convertCocoaKeyEvent:theEvent toGeckoEvent:&geckoEvent];
5851 
5852   // create event for use by plugins
5853   if (mIsPluginView) {
5854 #ifndef NP_NO_CARBON
5855     EventRecord carbonEvent;
5856     if (mPluginEventModel == NPEventModelCarbon) {
5857       ConvertCocoaKeyEventToCarbonEvent(theEvent, carbonEvent, message);
5858       geckoEvent.pluginEvent = &carbonEvent;
5859     }
5860 #endif
5861     NPCocoaEvent cocoaEvent;
5862     if (mPluginEventModel == NPEventModelCocoa) {
5863       ConvertCocoaKeyEventToNPCocoaEvent(theEvent, cocoaEvent, message);
5864       geckoEvent.pluginEvent = &cocoaEvent;
5865     }
5866   }

The cocoaEvent and carbonEvent are defined in |if (mIsPluginView)| block, but sets the pointer to geckoEvent which is outside of the block.

I'm not sure this causes actual crash because I don't find the crash reports.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 518003 [details] [diff] [review]
Patch v1.0

Josh made this issue in bug 634387, so, let's switch the reviewer to Josh.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

There are three other places in nsChildView.mm with the same problem:

http://hg.mozilla.org/mozilla-central/annotate/475ae5b49540/widget/src/cocoa/nsChildView.mm#l3453
http://hg.mozilla.org/mozilla-central/annotate/475ae5b49540/widget/src/cocoa/nsChildView.mm#l3643
http://hg.mozilla.org/mozilla-central/annotate/475ae5b49540/widget/src/cocoa/nsChildView.mm#l3748

Your patch should fix them, too.

I'm responsible for all the other cases :-(  Which is (I suppose) why
your report touched a nerve, and caused me to look for them.

I suspect none of these problems actually causes crashes -- possibly
due to quirks in the compiler's behavior.  But they're all easy to
fix, and really should be fixed.

By the way, Josh is on vacation for several more days.  So I'd be
happy to review your next patch.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Patch v2.0

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 518263 [details] [diff] [review]
Patch v2.0

Looks fine to me.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

(In reply to comment #2)
> I suspect none of these problems actually causes crashes -- possibly
> due to quirks in the compiler's behavior.  But they're all easy to
> fix, and really should be fixed.

I think that the patch should be landed after Fx4.0 but on both trunk and 2.0 branch because I think they actually don't cause crash with current compiler. How about you?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

That makes sense.

Future versions of the compiler might behave differently.  Or it's
behavior might change with different optimizations.

So it'd be safest to have this on both the trunk and the 2.0 branch.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Patch v2.0 (mq)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Ehsan, I think we should land this on cedar.

Comment 9

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/b3ae48f82202

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c22f220cb426