320746 - [mac] Use GetCurrentEventKeyModifiers() instead of GetCurrentKeyModifiers()

Status: RESOLVED FIXED

(Core :: Widget, defect, P2)

Description

[Reed Loden [:reed]]

Spawning from bug 311466 comment #10 (from Jonas Wallden):
According to
http://developer.apple.com/documentation/Carbon/Conceptual/Carbon_Event_Manager/
Tasks/chapter_3_section_7.html GetCurrentKeyModifiers() will read modifier keys
even if the application is in the background. This means a user switching to another app during launch may inadvertently trigger safe mode. Apple recommends calling GetCurrentEventKeyModifiers() instead.

Comment 1

[Reed Loden [:reed]]

Attachment: Use GetCurrentEventKeyModifiers() - v1 (1.8)

This patch just changes GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers(). I can't test this as I do not have a Mac OS X box. From a grep of entire tree, there are other places where this might need to be done.

Comment 2

[Reed Loden [:reed]]

Making this bug for all of the tree. Patch coming up soon.

Comment 3

[Reed Loden [:reed]]

Attachment: Use GetCurrentEventKeyModifiers() - v1 (trunk)

Change GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers() everywhere except for plugin/oji/MRJCarbon/plugin/Source/MRJContext.cp.

Comment 4

[Mark Mentovai]

Comment on attachment 206279 [details] [diff] [review]
Use GetCurrentEventKeyModifiers() - v1 (trunk)

I'm not sure about this.  Calling GetCurrentEvent with no CE handler on the stack seems risky.  Calling it implicitly through GetCurrentEventKeyModifiers is no better.  We don't want to get old cached events, or worse, garbage.  This type of thing might work now because more system internals are implemented as CEs, and because we're still dependent on a bunch of Carbon ourselves, but I'm afraid that this will break in a bad way when Apple changes AppKit.

(If Apple's explicitly said something about maintaining GetCurrentEvent compatibility in Cocoa apps, lemme know.)

Comment 5

[Mark Mentovai]

(In reply to comment #4)
> Calling it implicitly through GetCurrentEventKeyModifiers is no better.

It's not an implicit call, it's actually a separate cache that's maintained in response to other events, but I'm still concerned about missing those other events in the future.  I'll think about this some more.

Comment 6

[Håkan Waara]

Mark and I discussed this:  we can probably use NSApplication's currentEvent message, to solve this problem.

Comment 7

[Chris Lawson (gone)]

I have a patch for the Camino-only portion of the tree. Should I put that in this bug, or should I open a new bug just for our fixes?

I'm not sure how the Carbon vs. Cocoa issue is going to resolve itself for the rest of the code base, which is why I haven't patched anything outside Camino.

cl

Comment 8

[Mark Mentovai]

You can separate the patch out if you want to.

As for the stuff in the core, looking at Reed's patch (by the way, Reed, did you enjoy New York?), the only thing that we'd want to convert to Cocoa for sure is the stuff in widget/src/cocoa.  In those cases, there's no need to use [NSApp currentEvent], because every place GetCurrentKeyModifiers() is called, there's a perfectly good NSEvent already available.

The rest of the stuff in the core should be left Carbon, at least for now, changing GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers().  But hold off on that until you're sure it does the right thing, for example, when you command-tab to put the app in the background.

Comment 9

[Chris Lawson (gone)]

(In reply to comment #8)
> You can separate the patch out if you want to.

Coming shortly, as an attachment to this bug.

> As for the stuff in the core, looking at Reed's patch (by the way, Reed, did
> you enjoy New York?), the only thing that we'd want to convert to Cocoa for
> sure is the stuff in widget/src/cocoa.  In those cases, there's no need to use
> [NSApp currentEvent], because every place GetCurrentKeyModifiers() is called,
> there's a perfectly good NSEvent already available.
> 
> The rest of the stuff in the core should be left Carbon, at least for now,
> changing GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers().  But hold
> off on that until you're sure it does the right thing, for example, when you
> command-tab to put the app in the background.

I'll let Håkan handle this, then, since he has a far better handle on what's going on in Core than I do. Assigning to him for now, and if he can't get to it, you can take care of it when you get around to it, Mark.

cl

Comment 10

[Chris Lawson (gone)]

Attachment: Camino-only patch

Here's the Camino portion of the patch.

cl

Comment 11

[Håkan Waara]

Chris, please do a quick sanity check that the places you changed work as expected before this gets checked in.

Comment 12

[Mark Mentovai]

Comment on attachment 216608 [details] [diff] [review]
Camino-only patch

As discussed:

When you're sticking this stuff into a BOOL, you do need the explicit |!= 0| check.  BOOLs are 8 bits wide, and the modifiers correspond to more significant bits than that, so they'll get lost and everything will evaluate to 0 when you try to copy the wider 32-bit value into the BOOL.

This change is OK because there's no BOOL involved, and if can see all 32 bits:

-      if (GetCurrentKeyModifiers() & cmdKey)
+      if ([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask)

Comment 13

[Chris Lawson (gone)]

Attachment: fixes Mark's comments (checked in, trunk and 1.8)

BTW, the reason I marked the other two patches as obsolete was because there's been a lot of work going on in the Cocoa widget stuff lately and they're very likely bitrotted.

Håkan, if you want to bring them up to snuff once the Camino-only stuff lands, be my guest.

cl

Comment 14

[Mark Mentovai]

Comment on attachment 216698 [details] [diff] [review]
fixes Mark's comments (checked in, trunk and 1.8)

I still think alternates are generally better where we can use 'em, but this ought to work.  Death to GetCurrentKeyModifiers.

Comment 15

[Mark Mentovai]

Comment on attachment 216698 [details] [diff] [review]
fixes Mark's comments (checked in, trunk and 1.8)

This patch has been checked in on the trunk and 1.8 branch.

Comment 16

[Colin Barrett [:cbarrett]]

I think we should really change those other call sites -- this is causing a pretty annoying behavior with Safe Mode, where if you are holding down option in another app Firefox will start in Safe Mode, which requires you to quit and restart to get back to regular (unsafe?) mode.

Now that we are all Cocoa widgets, is it safe to call GetCurrentEvent with [NSApplication run] on the stack? We'd need to make sure that's the case, it might not be during startup.

Perhaps we can "fake" the behavior by also checking if we are the foreground application?

Nominating for blocking because of the really annoying behavior of Safe Mode activating while Firefox is starting up in the background.

Comment 17

[Damon Sicore (:damons)]

So, this exists in Fx2, right?  Doesn't sound like this should block, but re-nom if you are dead set on blocking.

Comment 18

[Colin Barrett [:cbarrett]]

The behavior of holding down option key changed in 1.9. Before it used to bring up the profile manager, from which it was easy to continue on to your task. Not a big deal if it came up accidentally.

Now the Safe Mode dialog opens when you hold down option, and it's impossible to get to a normal browsing session from that dialog without quitting the browser and relaunching.

I was going to file a separate bug for changing that behavior, but gavin pointed me at this one. Maybe we want a separate bug afterall?

Comment 19

[Damon Sicore (:damons)]

OK.  We should fix this as it's a generally bad user experience.  +'ing with a P2.  Josh, can you re-prioritize accordingly?

Comment 20

[Josh Aas]

Attachment: convert all, v1.0

I did testing, looks like we can go ahead and make all of the conversions.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Josh, Apple's current doc at the URL from comment #0 says the
following:

> Note that GetCurrentEventButtonState and GetCurrentEventKeyModifiers
> do not return useful values if your application is not active. If
> your application wants to determine the current mouse or keyboard
> modifier state while in the background, it must query the hardware
> using GetCurrentButtonState or GetCurrentKeyModifiers.

Is this something we need to worry about?

Comment 22

[Josh Aas]

Not that I know of. We read correctly on startup and whenever we send events to plugins, so I think we're fine. Those are the two cases that we're changing in my patch.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

What I'm worried about is pathological cases like an event being sent
to a plugin just after the browser has switched to the background.
Apple's doc implies that GetCurrentKeyModifiers() will return garbage
when called in the background.

Comment 24

[Josh Aas]

How would that happen? If the browser is in the background and the user takes action, the action shouldn't be fed to the plugin. If the browser is not in the background with the user takes action, the flow would be like this:

- action
- event generated for browser
- app backgrounds at some point
- event processed in queue, modifier used when it was generated is used

This seems correct to me. You either generated the event while the browser was active or not, either way it is correct.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

OK.  As long as you're sure GetCurrentKeyModifiers() will never be
called while the browser is backgrounded, then I have no problems with
this patch.

Comment 26

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

Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

Let's do this after beta 4.

Comment 27

[Reed Loden [:reed]]

Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

This is a blocker. No approval1.9 needed.

Comment 28

[Josh Aas]

landed on trunk, resolving, we don't need to do more on the 1.8 branch

Comment 29

[Jesse Ruderman]

This patch didn't fix the bug.  Holding Alt while Firefox is starting in the background (e.g. from a python script that keeps restarting Firefox) can still make Firefox go into safe mode.  I'm using Tiger.

Comment 30

[Jesse Ruderman]

Filed bug 507782.