815383 - pressing command key over vnc results in "Assertion failure: !(flag & [aNativeEvent modifierFlags]), at .../widget/cocoa/TextInputHandler.mm:1676"

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Dana Keeler (she/her) [:keeler]]

STR:
1. connect to an OS X machine over vnc
2. build or download a debug version of firefox
3. run it
4. press (or hold?) the command (apple) key (which is mapped to the "windows" key on the keyboard I'm using)
5. firefox halts with the following:

Assertion failure: !(flag & [aNativeEvent modifierFlags]), at /Users/keeler/mozilla-central/widget/cocoa/TextInputHandler.mm:1676

Comment 1

[Dana Keeler (she/her) [:keeler]]

I'm pretty sure this was introduced in bug 786956 (before that, I remember seeing output indicating failures of some kind, but it never halted the process, so I assumed it didn't matter).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> 1. connect to an OS X machine over vnc

Using screen sharing?  Using ssh?

Comment 3

[Dana Keeler (she/her) [:keeler]]

It's ssh+vnc. The "Sharing" services I have on are "Remote Login" and "Remote Management". Also, it looks like it's not just the command key - shift and control do it too.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

It's *never* been possible to run Firefox (or any GUI app) over an ssh connection and have it work properly.  Why are you doing it? :-)

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Do you make your ssh connection by doing "ssh [host]"?

And then do you run FF from the command line?

Comment 6

[Dana Keeler (she/her) [:keeler]]

The ssh is just a tunnel for privacy. The important part is I'm on one computer (a linux box, but that shouldn't matter), and I'm using vnc to remotely access another. This gives me a nice graphical environment as if I were sitting at the remote Mac. From there, I can either run my build from the command line or graphically launch a downloaded debug build in /Applications.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

OK, I don't understand what you're doing to connect to the remote Mac.

Please explain in more detail.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Sure:
On the remote Mac (OSX 10.7.5)
1. In System Preferences -> Sharing, both "Remote Login" (ssh) and "Remote Management" (basically vnc, if I understand correctly) are on.
2. For "Remote Management" -> "Computer Settings...", "VNC viewers may control screen with password" is checked, and a password is set.

On the local machine, either:
1. 'ssh -L 5901:localhost:5901 remoteMac'
2. (in another shell on the local machine) 'vncviewer localhost'
(I'm not sure about the port numbers - this is the old way of doing it)

or:
1. 'ssvncviewer -via remoteMac localhost' (this ssh tunnels to the remote machine before connecting over vnc)

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Which "VNC Viewer" are you using on the local machine?  Is there one I can compile on OS X?  (I currently don't have a Linux box to test with.)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Keeler from comment #1)
> I'm pretty sure this was introduced in bug 786956 (before that, I remember
> seeing output indicating failures of some kind, but it never halted the
> process, so I assumed it didn't matter).

That means VNC synthesizes strange NSFlagsChanged event whose keyCode is not a modifier key. And it may not set device dependent flags to the modifierFlags.

Comment 11

[Dana Keeler (she/her) [:keeler]]

(In reply to Steven Michaud from comment #9)
> Which "VNC Viewer" are you using on the local machine?  Is there one I can
> compile on OS X?  (I currently don't have a Linux box to test with.)

Here's what I'm using: http://www.karlrunge.com/x11vnc/ssvnc.html
(http://www.karlrunge.com/x11vnc/ssvnc.html#download for actual downloads)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> And it may not set device dependent flags to the modifierFlags.

Er, this is wrong. It has device dependent flags but we're not sure it's same as the native event.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Hmm, even with this patch, VNC always generates left modifier key events. So, KeyboardEvent.location never becomes DOM_KEY_LOCATION_RIGHT.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

I think that this is the safest fix.

VNC server in MacOS is pretty tricky. When I type modifier keys which can be distinguished left or right, VNC always sets left modifier key's device dependent flag with 0 keyCode. But other similar applications may send NSFlagsChanged events without device dependent flags and with 0 keyCode. So, it's the safest way to handle with device independent flags.

However, when I type a key via VNC on Windows, VNC tries inactivating NumLock. Then, we dispatch unnecessary VK_CLEAR key events for every key press. So, we should ignore the NumLock state change at the default case since current Mac doesn't have NumLock key and if it's a real key event, the keyCode should be set.

I think that the patch for bug 786956 should be backed out from Aurora or land this patch on Aurora too.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

OOps, function key case's new comment isn't necessary. I'll remove it, please ignore it.

Comment 16

[Alex Keybl [:akeybl]]

Let's go with the lowest risk solution and back bug 786956 out of Aurora.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Alex Keybl [:akeybl] from comment #16)
> Let's go with the lowest risk solution and back bug 786956 out of Aurora.

Thank you. Do I need to post the backout patch and need to request the approval? Or can I just backout the patch from Aurora with tracking-firefox19+ flag?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ugh, sorry I reset the flag unexpectedly...

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> (In reply to Alex Keybl [:akeybl] from comment #16)
> > Let's go with the lowest risk solution and back bug 786956 out of Aurora.
> 
> Thank you. Do I need to post the backout patch and need to request the
> approval? Or can I just backout the patch from Aurora with
> tracking-firefox19+ flag?

If it's a simple backout and requires no other code changes, a=akeybl. Otherwise we need to get r+ and post an approval request before backing out. Please make sure to comment in the appropriate bugs when performing the backout.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Alex. I just backed out the patch without any changes.

I have a question, I change tracking flag's value to "fixed", is that correct? "disabled" or something is better?

https://hg.mozilla.org/releases/mozilla-aurora/rev/15af27ede830

Comment 21

[Alex Keybl [:akeybl]]

Fixed is correct. Thanks Masayuki!

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Steven: ping

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Sorry, Masayuki.  I should be able to get to this later this week.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 686918 [details] [diff] [review]
Patch

TextInputHandler::HandleFlagsChanged() keeps getting harder to
understand :-)  But I believe I understand your changes, and they look
OK to me.

I do have some suggested changes to your comments, though:

+      // XXX Although, some applications might send the event with wrong device
+      //     devendent flags.

"XXX Some applications might send the event with incorrect
device-dependent flags."

If some applications actually do send incorrect device-dependant flags
(it's hard to tell from your comments in the bug), this should be as
follows:

"XXX Some applications send the event with incorrect device-dependent
flags."

+        // Typically, flag change here must be a deactivation except some
+        // lockable modifiers such as CapsLock.  However, some applications
+        // like VNC could send activating event without proper keyCode.  So,
+        // we need to decide which key event should be fired.

"Given correct information from the application, a flag change here
will normally be a deactivation (except for some lockable modifiers
such as CapsLock).  But some applications (like VNC) can send an
activating event with a zero keyCode.  So we need to check for that
here."

+              // NSNumericPadKeyMask change may be fired by VNC a lot of times.
+              // On the other hand, the Clear key event should be fired with
+              // proper keyCode.  So, we should just ignore this change.

"NSNumericPadKeyMask is fired by VNC a lot.  But not all of these
events can really be Clear key events, so we just ignore them."

+              // Typically, NSFunctionKeyMask change here must be a
+              // deactivation.  However, some applications like VNC could send
+              // activating event without proper keyCode.

"An NSFunctionKeyMask change here will normally be a deactivation.
But sometimes it will be an activation sent (by VNC for example) with
a zero keyCode."

+              // dispatchKeyDown = ((flag & [aNativeEvent modifierFlags]) != 0);

Just get rid of this line?

+            // The these cases (NSShiftKeyMask, NSControlKeyMask,
+            // NSAlternateKeyMask and NSCommandKeyMask) should be handled by the
+            // other branch of the if statement, below (which handles device
+            // dependent flags).  However, some applications like VNC may
+            // send key events without device dependent flags.

"These cases (NSShiftKeyMask, NSControlKeyMask, NSAlternateKeyMask and
NSCommandKeyMask) should be handled by the other branch of the if
statement, below (which handles device dependent flags).  However some
applications (like VNC) can send key events without any device
dependent flags, so we handle them here instead."

+            // VNC doesn't set proper keyCode value but sets device dependent
+            // flags.  However, we cannot distinguish the key location since
+            // VNC always sets left key's device dependent flags.
+            // Let's handle the keys with device independent modifier flags
+            // since it's safer.

"See the note above (in the other branch of the if statement) about
the NSShiftKeyMask, NSControlKeyMask, NSAlternateKeyMask and
NSCommandKeyMask cases."

Or maybe just drop this comment altogether.

Comment 25

[Chris Peterson [:cpeterson]]

btw, I see this assertion failure pretty often when I'm switching rapidly between applications using CMD+TAB. I'm running Mac OS X 10.8.2. I'm not using VNC.

In this particular case:
  [aNativeEvent modifierFlags] == 0x20102
  flag == 0x2
  TextInputHandler::sLastModifierFlags == 0x100108

The assertion fails because the &'d result is 0x2, not 0x0. The modifierFlag 0x2 is one of the device-dependent flags.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Steven. Your understanding must be correct.

(In reply to Chris Peterson (:cpeterson) from comment #25)
> In this particular case:
>   [aNativeEvent modifierFlags] == 0x20102

NSShiftKeyMask (0x20000) |
  NX_NONCOALSESCEDMASK (0x100) | NX_NEXTLSHIFTKEYMASK (0x2)

>   flag == 0x2
>   TextInputHandler::sLastModifierFlags == 0x100108

NSCommandKeyMask (0x100000) |
  NX_NONCOALSESCEDMASK (0x100) | NX_NEXTLCMDKEYMASK (0x8)

> The assertion fails because the &'d result is 0x2, not 0x0. The modifierFlag
> 0x2 is one of the device-dependent flags.

Yeah. Looks like left-command key was pressed on Gecko, and switches to another application with left-shift key??

Anyway, I think that the patch also fixes your case. Please check it with m-c tomorrow.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

device-independent flags:

NSAlphaShiftKeyMask     0x00010000
NSShiftKeyMask          0x00020000
NSControlKeyMask        0x00040000
NSAlternateKeyMask      0x00080000
NSCommandKeyMask        0x00100000
NSNumericPadKeyMask     0x00200000
NSHelpKeyMask           0x00400000
NSFunctionKeyMask       0x00800000

device-dependent flags (found in unofficial NeXT's header files in the web):

NX_NEXTCTLKEYMASK       0x00000001
NX_NEXTLSHIFTKEYMASK    0x00000002
NX_NEXTRSHIFTKEYMASK    0x00000004
NX_NEXTLCMDKEYMASK      0x00000008
NX_NEXTRCMDKEYMASK      0x00000010
NX_NEXTLALTKEYMASK      0x00000020
NX_NEXTRALTKEYMASK      0x00000040
NX_STYLUSPROXIMITYMASK  0x00000080
NX_NONCOALSESCEDMASK    0x00000100

Found with actual hardware on 10.7:

right-control key:      0x00002000

Comment 28

[Chris Peterson [:cpeterson]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Yeah. Looks like left-command key was pressed on Gecko, and switches to
> another application with left-shift key??

Masayuki, I was indeed using left-shift + left-command + tab key combination to switch applications. The shift key reverses the direction of the application switcher.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53accd955c2b

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/53accd955c2b

Comment 31

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0

I used a machine with Windows 7 x64 to connect remotely to a machine with MacOSX 10.8, and I can say that in Firefox 19 beta 5 (buildID: 20130206083616) this issue is verified as fixed.

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0

Verified as fixed using Firefox 20.0 (buildID: 20130326150557).