1013744 - [e10s] Can't open Web Console using CMD+OPT+K and CMD+OPT+I keyboard shortcuts in e10s

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect, P1)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Install Greasemonkey addon: https://addons.mozilla.org/en-US/firefox/addon/greasemonkey
2. In a new e10s window, open a random web page.
3. Try to open the Web Console with the keyboard shortcuts CMD+OPT+K or CMD+OPT+I.

RESULT:
Nothing happens! The Web Console does not open, though the "Tools" menu flashes when pressing CMD+OPT+I.

I don't have any Greasemonkey user scripts installed, just the Greasemonkey addon itself.

Comment 1

[Chris Peterson [:cpeterson]]

I was mistaken: this bug does not require the Greasemonkey addon. I can repro this bug without any addons installed.

We should consider this bug for M2 or even M1 milestone.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I shouldn't be surprised anymore, but this is more complicated than it first looked, and would _not_ have made a good first bug. So, luckily, it's not marked as such anymore. :)

I think I'm narrowing down on this. In bug 862519, we added infrastructure for handling keyboard shortcuts with e10s. We do this by capturing keyboard input in the capture phase in the parent process, marking anything that needs to execute in the bubbling phase on the way down, serializing the event, letting the content process run the event, and then the content process echo's the event back up to the parent process where it gets handled. At least, I _think_ that's how it works.

This works well in most cases, _except_ on OS X when the user is modifying their key with Alt. On OS X, Alt is used to insert alternative characters. For example, Alt-j results in ∆.

Without e10s, we see through that alternative character with nsContentUtils::GetAccelKeyCandidates which returns an array of alternative keys that the key event might actually represent (so there might be a candidate for ∆, but there also might be a candidate for Alt-j, so it returns ∆, Alt-j).

The problem seems to be that when the event is deserialized from the content process, we either fail to get accel key candidates, or the function just doesn't work as it should, because we're only considering the alternative character (∆) and not the other one (Alt-j), and the latter is the one we're really interested in, since this is almost certainly what was set in the <key> handler.

So I need to figure out why nsContentUtils::GetAccelKeyCandidates is not working when alt-keying into e10s tabs.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I _think_ nsContentUtils::GetAccelKeyCandidates is not working because alternativeCharCodes on the WidgetKeyboardEvent isn't being properly serialized.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: [e10s] Can't open Web Console using CMD+OPT+K and CMD+OPT+I keyboard shortcuts in e10s

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8459852 [details] [diff] [review]
[e10s] Can't open Web Console using CMD+OPT+K and CMD+OPT+I keyboard shortcuts in e10s

Hey jimm, masayuki:

So in order to fix this bug, I want to try to serialize and deserialize WidgetKeyboardEvent.alternativeCharCodes over IPC.

I started by just attempting to send over the number of alternative character codes that are available, but in my debug build, this is resulting in a crash:

IPDL protocol error: unknown union type
[Child 66281] ###!!! ABORT: IPDL error [PBrowserChild]: "unknown union type". abort()ing as a result.: file /Users/mikeconley/Projects/mozilla-central/ipc/glue/ProtocolUtils.cpp, line 201
mozilla::ipc::FatalError(char const*, char const*, int, bool)+0x0000017D [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0078988D]
mozilla::dom::PBrowserChild::FatalError(char const*) const+0x0000002A [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0082672A]
mozilla::dom::PBrowserChild::Read(mozilla::dom::MaybeNativeKeyBinding*, IPC::Message const*, void**)+0x0000012C [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0082DABC]
mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&)+0x00002B1B [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008322DB]
mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)+0x00000092 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008C2262]
mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)+0x00000117 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00785817]
mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&)+0x000000CF [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00784B6F]
mozilla::ipc::MessageChannel::OnMaybeDequeueOne()+0x000001FE [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00780DBE]
void DispatchToMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), Tuple0 const&)+0x00000083 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0079C1F3]
RunnableMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run()+0x0000004E [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0079C0EE]
mozilla::ipc::MessageChannel::RefCountedTask::Run()+0x00000028 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0079E598]
mozilla::ipc::MessageChannel::DequeueTask::Run()+0x00000024 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0079E564]
MessageLoop::RunTask(Task*)+0x00000060 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00709CE0]
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&)+0x0000004F [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0070A25F]
MessageLoop::DoWork()+0x00000124 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0070A494]
mozilla::ipc::DoWorkRunnable::Run()+0x00000095 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00788585]
nsThread::ProcessNextEvent(bool, bool*)+0x00000622 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00156DA2]
NS_ProcessPendingEvents(nsIThread*, unsigned int)+0x000000AB [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0003E95B]
nsBaseAppShell::NativeEventCallback()+0x000000C9 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01FDD3E9]
nsAppShell::ProcessGeckoEvents(void*)+0x000001B1 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F677B1]
__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__+0x00000011 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00012B31]
__CFRunLoopDoSources0+0x000000F5 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00012455]
__CFRunLoopRun+0x00000315 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x000357F5]
CFRunLoopRunSpecific+0x00000122 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x000350E2]
RunCurrentEventLoopInMode+0x000000D1 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0005FEB4]
ReceiveNextEventCommon+0x00000164 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0005FC52]
BlockUntilNextEventMatchingListInMode+0x0000003E [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0005FAE3]
_DPSNextEvent+0x000002AD [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x00155533]
-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x00000080 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x00154DF2]
-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x00000077 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F66427]
-[NSApplication run]+0x00000205 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x0014C1A3]
nsAppShell::Run()+0x000000A7 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F68127]
XRE_RunAppShell+0x00000159 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x03C856A9]
mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)+0x000000C9 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00788809]
MessageLoop::RunInternal()+0x00000076 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00709BC6]
MessageLoop::RunHandler()+0x00000015 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00709AD5]
MessageLoop::Run()+0x0000002D [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00709A7D]
XRE_InitChildProcess+0x00000A81 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x03C84EF1]
main+0x000000C6 [/Users/mikeconley/Projects/mozilla-central/obj-debug/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container +0x00000D76]
[Child 66281] ###!!! ABORT: IPDL error [PBrowserChild]: "unknown union type". abort()ing as a result.: file /Users/mikeconley/Projects/mozilla-central/ipc/glue/ProtocolUtils.cpp, line 201
Hit MOZ_CRASH() at /Users/mikeconley/Projects/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30

STR:

1) Apply patch, and make a debug build
2) Run Firefox, and open an e10s window
3) When the e10s window is done loading, hit Accel+Alt+k (although any keyboard input will probably do).


Any idea why I'm hitting this crash? What have I missed?

Comment 6

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

Comment on attachment 8459852 [details] [diff] [review]
[e10s] Can't open Web Console using CMD+OPT+K and CMD+OPT+I keyboard shortcuts in e10s

I guess that it's same as bug 1035595. nsTArray<T>::Length() is size_t, not uint32_t.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Serialize alternative character codes to the content process to fix Alt-key shortcuts. r=?

The alternative character codes weren't being serialized, so keyboard events that were
echoed back from the content process would fail to match XUL <key> event handlers if
they were mapped to characters with the "alt" modifier.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8459976 [details] [diff] [review]
Serialize alternative character codes to the content process to fix Alt-key shortcuts. r=?

Ah, yes, that was the problem! Thank you so much, masayuki!

Comment 9

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

Comment on attachment 8459976 [details] [diff] [review]
Serialize alternative character codes to the content process to fix Alt-key shortcuts. r=?

>+    uint32_t altCharCodesLen =
>+      static_cast<uint32_t>(aParam.alternativeCharCodes.Length());

Now this should work but I don't like this.

Could you change TextEvents.h to define AlternativeCharCodeArray and make WidgetKeyboardEvent and nsGUIEventsIPC.h use it? I.e.,

* Define: #typedef nsTArray<AlternativeCharCode> AlternativeCharCodeArray;
* Change the definition of WidgetKeyboardEvent::alternativeCharCodes with the new type name.
* Use AlternativeCharCodeArray::size_type in nsGUIEventsIPC.h

>   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>   {
>-    uint32_t keyNameIndex = 0, codeNameIndex = 0;
>+    uint32_t keyNameIndex = 0, codeNameIndex = 0, numAlternativeCharCodes = 0;
>     if (ReadParam(aMsg, aIter,
>                   static_cast<mozilla::WidgetInputEvent*>(aResult)) &&
>         ReadParam(aMsg, aIter, &keyNameIndex) &&
>         ReadParam(aMsg, aIter, &codeNameIndex) &&
>         ReadParam(aMsg, aIter, &aResult->mKeyValue) &&
>         ReadParam(aMsg, aIter, &aResult->mCodeValue) &&
>         ReadParam(aMsg, aIter, &aResult->keyCode) &&
>         ReadParam(aMsg, aIter, &aResult->charCode) &&
>         ReadParam(aMsg, aIter, &aResult->isChar) &&
>         ReadParam(aMsg, aIter, &aResult->mIsRepeat) &&
>         ReadParam(aMsg, aIter, &aResult->location) &&
>-        ReadParam(aMsg, aIter, &aResult->mUniqueId))
>+        ReadParam(aMsg, aIter, &aResult->mUniqueId) &&
>+        ReadParam(aMsg, aIter, &numAlternativeCharCodes))
>     {
>+      for (uint32_t i = 0; i < numAlternativeCharCodes; ++i) {
>+        uint32_t unshiftedCharCode;
>+        uint32_t shiftedCharCode;
>+        if (!ReadParam(aMsg, aIter, &unshiftedCharCode) ||
>+            !ReadParam(aMsg, aIter, &shiftedCharCode)) {
>+          return false;
>+        }
>+        mozilla::AlternativeCharCode altCharCode(unshiftedCharCode, shiftedCharCode);

This should be safer:

>+        mozilla::AlternativeCharCode altCharCode;
>+        if (!ReadParam(aMsg, aIter, &altCharCode.mUnshifteCharCode) ||
>+            !ReadParam(aMsg, aIter, &altCharCode.mShiftedCharCode)) {

Comment 10

[Tom S [:evilpie]]

Attachment: Masayuki's approach

I first tried to get away with just serializing mozilla::AlternativeCharCode, but that doesn't work, because the serializer for for nsTArray needs an empty constructor.

Comment 11

[Tom S [:evilpie]]

This doesn't crash, but I am not sure what I have to do for this code to make a difference.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8460157 [details] [diff] [review]
Masayuki's approach

I tested this patch and it seems to do the job - thanks evilpie. :)

Is this closer to what you had in mind, masayuki?

Comment 14

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

Comment on attachment 8460157 [details] [diff] [review]
Masayuki's approach

>+    for (uint32_t index = 0; index < length; index++) {
>+      uint32_t unshiftedChar;
>+      uint32_t shiftedChar;
>+      if (!ReadParam(aMsg, aIter, &unshiftedChar) ||
>+          !ReadParam(aMsg, aIter, &shiftedChar)) {
>+        aResult->Clear();
>+        return false;
>+      }
>+      mozilla::AlternativeCharCode altCharCodes(unshiftedChar, shiftedChar);
>+      aResult->AppendElement(altCharCodes);
>+    }

Cannot write as:

>+    for (uint32_t index = 0; index < length; index++) {
>+      mozilla::AlternativeCharCode altCharCodes;
>+      if (!ReadParam(aMsg, aIter, &altCharCodes.mUnshiftedCharCode) ||
>+          !ReadParam(aMsg, aIter, &altCharCodes.mShiftedCharCode)) {
>+        aResult->Clear();
>+        return false;
>+      }
>+      aResult->AppendElement(altCharCodes);
>+    }

? This style won't broken by the change of type of AlternativeCharCode, although, it won't occurs. If it's impossible, landing without this change is okay.

Comment 15

[Tom S [:evilpie]]

>? This style won't broken by the change of type of AlternativeCharCode, although, it won't occurs. If >it's impossible, landing without this change is okay.
Yeah this isn't possible right now, because of how the constructor of AlternativeCharCode is defined.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tom Schuster [:evilpie] from comment #15)
> >? This style won't broken by the change of type of AlternativeCharCode, although, it won't occurs. If >it's impossible, landing without this change is okay.
> Yeah this isn't possible right now, because of how the constructor of
> AlternativeCharCode is defined.

Over IRC, masayuki and I agreed we should just add an empty constructor to AlternativeCharCode. I'll do that now.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Ok, last bit of redirection - evilpie has offered to write the empty constructor, as well as create a new AlternativeCharCode serializer, which will clean a bunch of this up.

Unfortunately, this will require another new review, but it should result in cleaner code.

Thanks evilpie!

Comment 18

[Tom S [:evilpie]]

Attachment: v2

A lot simpler code when you have an empty constructor.

Comment 19

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

Comment on attachment 8460367 [details] [diff] [review]
v2

What's this? WidgetKeyboardEvent.alternativeCharCode isn't AlternativeCharCode. It is nsTArray<AlternativeCharCode>. So, the previous patch's change must be necessary.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Er, see above, I guess. :)

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8460367 [details] [diff] [review]
v2

I just tested this patch, and it seemed to work alright - but is that only because the key combination I tested only had a single alternate? Is this code busted for multiple alternative character codes?

Comment 22

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

(In reply to Mike Conley (:mconley) from comment #21)
> Comment on attachment 8460367 [details] [diff] [review]
> v2
> 
> I just tested this patch, and it seemed to work alright - but is that only
> because the key combination I tested only had a single alternate? Is this
> code busted for multiple alternative character codes?

I'm not sure. Does it work with this?
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#463

If so, I'll give r+. Could you confirm it with debugger or printf()?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> (In reply to Mike Conley (:mconley) from comment #21)
> > Comment on attachment 8460367 [details] [diff] [review]
> > v2
> > 
> > I just tested this patch, and it seemed to work alright - but is that only
> > because the key combination I tested only had a single alternate? Is this
> > code busted for multiple alternative character codes?
> 
> I'm not sure. Does it work with this?
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#463
> 
> If so, I'll give r+. Could you confirm it with debugger or printf()?

I'm sorry - I'm not sure what you're asking me to do. I'm afraid I'm not too familiar with this code - are you asking me to change the alternativeCharCode from an nsTArray to a FallibleTArray?

Comment 24

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

(In reply to Mike Conley (:mconley) from comment #23)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> > (In reply to Mike Conley (:mconley) from comment #21)
> > > Comment on attachment 8460367 [details] [diff] [review]
> > > v2
> > > 
> > > I just tested this patch, and it seemed to work alright - but is that only
> > > because the key combination I tested only had a single alternate? Is this
> > > code busted for multiple alternative character codes?
> > 
> > I'm not sure. Does it work with this?
> > http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#463
> > 
> > If so, I'll give r+. Could you confirm it with debugger or printf()?
> 
> I'm sorry - I'm not sure what you're asking me to do. I'm afraid I'm not too
> familiar with this code - are you asking me to change the
> alternativeCharCode from an nsTArray to a FallibleTArray?

No, if the patch works fine, here
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#472
should run. So, I'd like you to confirm the line is running at copying a keyboard event.

Comment 25

[Tom S [:evilpie]]

Yes that is how this works. All of the serialization templates automatically nest. This means nsTArray<T> works for every T that also has ParamTraits<T> (i.e. is serializable). I believe nsTArray is actually an InfallibleTArray and so it uses this: http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#506, but that just uses the code you linked.

Comment 26

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

Comment on attachment 8460367 [details] [diff] [review]
v2

Thank you for the information. Then, r=masayuki.

Comment 27

[Tom S [:evilpie]]

\o/ Thanks for reacting so quickly. Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b61475872224 after try looked good enough https://tbpl.mozilla.org/?tree=Try&rev=200d9a07a26a

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b61475872224

Comment 29

[Mike Conley (:mconley) (:⚙️)]

My notes on this bug:

http://people.mozilla.org/~mconley2/bugnotes/bug-1013744.html

Comment 30

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using old Nightly (2014-07-04), verified that using latest Nightly (2014-09-01) the issue is not reproducible anymore, the Web Console can be opened using CMD+OPT+K and CMD+OPT+I in e10s.