Closed Bug 640110 Opened 9 years ago Closed 9 years ago

ChildView uses outdated pointer of plugin event for nsGUIEvent

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- .x+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #518003 - Flags: review?(smichaud)
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.
Attachment #518003 - Flags: review?(smichaud) → review?(joshmoz)
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 on attachment 518263 [details] [diff] [review]
Patch v2.0

Looks fine to me.
Attachment #518263 - Flags: review?(smichaud) → review+
(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?
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.
Summary: [ChildView fireKeyEventForFlagsChanged] uses invalid pointer for plugins → ChildView uses outdated pointer of plugin event for nsGUIEvent
blocking2.0: --- → .x+
http://hg.mozilla.org/mozilla-central/rev/c22f220cb426
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.