1051842 - [e10s] crash in -[ChildView keyDown:]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

This bug was filed from the Socorro interface and is 
report bp-e548287f-e0f7-436f-b224-2d7602140808.
=============================================================
My last 3 crashes have this signature. (2 on Friday, one this morning). And it it's probably half of my last weeks worth of crashes. This is with e10s turned on, usually happens which switching from one text box to another (i.e. from a username field to a password field)

Comment 1

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

This bug should be fixed before enabling e10s in Nightly builds in default settings because this crash might mean that key logger can steal password from us or some a11y tools are not available.

I guess that this is caused by call order of nsIWidget::SetInputContext() and nsIWidget::NotifyIME() are broken by e10s.

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#144

> 144     sync NotifyIMEFocus(bool focus)
> 145       returns (nsIMEUpdatePreference preference, uint32_t seqno);

Only NotifyIMEFocus() is synchronous call...

I think that at least SetInputContext() should be synchronous too.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#213

Comment 2

[David Parks [:handyman]]

Attachment: 1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

I thought so too but the real culprit is more subtle.  This patch fixes the crash -- the other patches are simply related issues.

First, the repro.

Steps:
* Open an e10s window
* Go to a page with a password text field (I used bugzilla.mozilla.com)
* Select the password text field.
* Command-Tab away from the browser, then Command-tab back to it.
* Type something

Expected Behavior: 
* The password field shows bullets for the typed characters.

Actual Behavior:
* Crash!

-------

> I guess that this is caused by call order of nsIWidget::SetInputContext() and nsIWidget::NotifyIME() are broken by e10s.

One of these patches makes SetInputContext synchronous because I agree with the concern, although it was not the cause of the crash.  OTOH, I don't think that the NotifyIME*() messages actually need to be made synchronous.  Since these messages are always sent between the same actor-instances, they are ordered by the message queue.  The weaker condition of 'proper call order' is all we need here -- we don't need actual synchronization with the other threads. 

I think SetInputContext may be different but thats really just a feeling.  I can't design a case where not being synchronous causes an issue... but it makes me nervous asynchronously setting things like password mode.  I've made it synchronous in a separate patch... it doesn't hurt anything.

-----------

The actual problem was that OSX has separate concepts of the 'main' and the 'key' window.  We conflate these two OSX messages into one Focus event that therefore fails.  Specifically, we see the following steps:

* When we Cmd-Tab back to the browser, OSX says that the window has become the Main window.
* We attempt to set password focus on the correct view in the new main window.  We fail because our window is not the key window.
* OSX tells us that the window has become the Key window.

We need to set the focus after the window becomes the key window.  I set focus when OSX sends the key window event.

Comment 3

[David Parks [:handyman]]

Attachment: 2. Normalize secure input requests.

Normalize secure input requests.  We have been counting the number of requests for secure input with the intent of requiring the equal amount of disable requests, but this behavior would not gain anything and the implementation actually never disabled requests - it always zeros out the count.  The original idea seems to have been simply to mimic OSX behavior.  This patch makes the zeroing-out behavior more consistent.  DisableSecureEventInput should probably be removed.

Comment 4

[David Parks [:handyman]]

Attachment: 3. Make the PBrowser::SetInputContext message synchronous.

As suggested in comment 1, making SetInputContext synchronous.

Comment 5

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

Comment on attachment 8475483 [details] [diff] [review]
1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

Looks great! Thank you for your work.

Although, I'm not a peer of these modules, I find some nits which violate our coding rules.

>+NS_IMETHODIMP
>+nsFocusManager::TakeKeyFocus(nsIDOMWindow* aWindow)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
>+  NS_ENSURE_TRUE(window && window->IsOuterWindow(), NS_ERROR_INVALID_ARG);

Now, we should use

if (NS_WARN_IF(!window || !window->IsOuterWindow()) {
  return NS_ERROR_INVALID_ARG;
}

>+  NS_ASSERTION(currentWindow, "keyboard focus set with no window current");
>+  if (!currentWindow)
>+    return NS_OK;

Wrap if block with {}. I.e.,

if (!currentWindow) {
  return NS_OK;
}

>+- (void)takeKeyFocus
>+{
>+  if (mGeckoWindow) {
>+    nsIWidgetListener* listener = mGeckoWindow->GetWidgetListener();
>+    if (listener)
>+      listener->TakeKeyFocus();

ditto.

>+- (void)releaseKeyFocus
>+{
>+  if (mGeckoWindow) {
>+    nsIWidgetListener* listener = mGeckoWindow->GetWidgetListener();
>+    if (listener)
>+      listener->ReleaseKeyFocus();

ditto.

>++ (void)takeKeyFocus:(NSWindow*)aWindow
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  WindowDelegate* delegate = (WindowDelegate*) [aWindow delegate];
>+  if (!delegate || ![delegate isKindOfClass:[WindowDelegate class]])
>+    return;

ditto.

>++ (void)releaseKeyFocus:(NSWindow*)aWindow
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  WindowDelegate* delegate = (WindowDelegate*) [aWindow delegate];
>+  if (!delegate || ![delegate isKindOfClass:[WindowDelegate class]])
>+    return;

ditto.

>+void
>+nsWebShellWindow::TakeKeyFocus()
>+{
>+  nsCOMPtr<nsIXULWindow> xulWindow(this);
>+
>+  nsCOMPtr<nsPIDOMWindow> window = mDocShell ? mDocShell->GetWindow() : nullptr;
>+  nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);
>+  if (fm && window)
>+    fm->TakeKeyFocus(window);

ditto.

Cannot you to use nsFocusManager here? nsFocusManager::GetFocusManager() is better an do_GetService() if it's available.

>+void
>+nsWebShellWindow::ReleaseKeyFocus()
>+{
>+  nsCOMPtr<nsIXULWindow> xulWindow(this);
>+
>+  nsCOMPtr<nsPIDOMWindow> window = mDocShell ? mDocShell->GetWindow() : nullptr;
>+  nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);
>+  if (fm && window)
>+    fm->ReleaseKeyFocus(window);

ditto.

Comment 6

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

Comment on attachment 8475487 [details] [diff] [review]
2. Normalize secure input requests.

LGTM. If you request review to me, I'll give r+ simply.

Comment 7

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

Comment on attachment 8475491 [details] [diff] [review]
3. Make the PBrowser::SetInputContext message synchronous.

>-    SetInputContext(int32_t IMEEnabled,
>+    sync SetInputContext(int32_t IMEEnabled,
>                     int32_t IMEOpen,
>                     nsString type,
>                     nsString inputmode,
>                     nsString actionHint,
>                     int32_t cause,
>                     int32_t focusChange);

Should be adjust the indent of following lines for arguments.

Comment 8

[David Parks [:handyman]]

Hi Masayuki.  Thanks for the help.  The review requests are on hold, due to this:

https://tbpl.mozilla.org/?tree=Try&rev=2c12f082516d

There are some orange mochitests related to focus.  So far, I'm thinking that the reason is that sendMouseEvent, which the tests use to change focus, does not tell the OS about the faked mouse event... and the OS uses mouse events to establish its own concept of focus, which is what this bug is all about.

This may be a bigger problem than it initially sounds.  Mochitests use DOMWindowUtils::sendMouseEvent to fake mouse presses.  Many other modules use the routine as well, for fake, as well as real, mouse events.  However, in the fake event case, we need that fake mouse event to trigger the same OSX keyWindowFocus event that occurs naturally with real mouse events.  We absolutely *do not* want to fake keyWindowFocus in the case of real mouse events -- we count on the OS to do it.  This may be doable as sendMouseEvent has a parameter "aIsSynthesized" that is intended to indicate whether a mouse event is fake.

OSX will only set key window focus on its UI event thread.  The call would therefore have to happen by synchronizing with the UI thread.  This would be a lot of effort and seems brittle and wrong.  I'm looking for a better way...

When I've got something working, I will take you up on your review offer :)

Most of the changes you asked for will be easy.  Some of it, like the nsWebShellWindow code, follows a pattern used by pretty much all functions in the file.  So did, for example, the nsWindowMap::takeKeyFocus code, although in that case I agree that the missing braces are useful enough to break symmetry with the rest of the file.  In each case, I can see both sides.

Comment 9

[David Parks [:handyman]]

Attachment: 1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

This patch fixes the coding guidelines mistakes and the orange mochitests.  The difference with the previous patch is that an OSX key-focus change is now only altered if it comes after an OSX main-focus change.  Usually, the OS sets main focus before key focus but that is not guaranteed and mochitest conditions are one of those weird cases.  Otherwise, FocusManager::Focus() does some funky OS-agnostic checks that make it fail when called twice.

Before I made the cosmetic changes, I got green tests here:
https://tbpl.mozilla.org/?tree=Try&rev=8a2caf82cc6c

Tests on these patches (with the cosmetic changes) are still coming in here:
https://tbpl.mozilla.org/?tree=Try&rev=2aea09376c7f

Comment 10

[David Parks [:handyman]]

Attachment: 2. Normalize secure input requests.

Comment 11

[David Parks [:handyman]]

Attachment: 3. Make the PBrowser::SetInputContext message synchronous.

Comment 12

[David Parks [:handyman]]

Attachment: 3. Make the PBrowser::SetInputContext message synchronous.

D'oh... Forgot to fix the indentation.

Comment 13

[Neil Deakin]

Comment on attachment 8476476 [details] [diff] [review]
1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

>exporting patch:
># HG changeset patch
># User David Parks <[e-mail]>
># Date 1408602141 25200
>#      Wed Aug 20 23:22:21 2014 -0700
># Node ID 6209bbaa275ce794c8fc6df99b2920fbd20f30c7
># Parent  e7806c9c83f3edc29787133cfaef14ab3bd635c9
>Bug 1051842 - crash in -[ChildView keyDown:].
>Distinguish between MacOS DidBecomeMain and DidBecomeKey events.  Focus
>was failing to be set correctly because the window was not key when
>focus was tested.  This patch tests focus on both events.
>
>diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
>--- a/dom/base/nsFocusManager.cpp
>+++ b/dom/base/nsFocusManager.cpp
>@@ -768,16 +768,44 @@ nsFocusManager::WindowLowered(nsIDOMWind
>   if (mFocusedWindow)
>     Blur(nullptr, nullptr, true, true);
> 
>   mWindowBeingLowered = nullptr;
> 
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsFocusManager::TakeKeyFocus(nsIDOMWindow* aWindow)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
>+  if (NS_WARN_IF(!window || !window->IsOuterWindow())) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  // retrieve the last focused element within the window that was raised
>+  nsCOMPtr<nsPIDOMWindow> currentWindow;
>+  nsCOMPtr<nsIContent> currentFocus =
>+    GetFocusedDescendant(window, true, getter_AddRefs(currentWindow));
>+
>+  NS_ASSERTION(currentWindow, "keyboard focus set with no window current");
>+  if (!currentWindow) {
>+    return NS_OK;
>+  }
>+
>+  Focus(currentWindow, currentFocus, 0, true, false, true, true);
>+  return NS_OK;
>+}
>+

This is certainly not correct. This will cause the same content to be focused twice and the window to receive the focus event twice, once when the window becomes main and then again when it becomes key. (We may not have an explict test for this)


> * We attempt to set password focus on the correct view in the new main window.  We fail because our window is not the key window.

In what way does this fail? Is a different window being set main than the key window?

Comment 15

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

Comment on attachment 8476481 [details] [diff] [review]
3. Make the PBrowser::SetInputContext message synchronous.

Although, I'm not a peer of this file, but this should be synchronous API.

Comment 16

[David Parks [:handyman]]

Attachment: 1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

The previous review was right about a number of events being erroneously sent on key/main messages.  This version uses nsFocusManager::EnsureCurrentWidgetFocused when a key focus event is received.  It also re-establishes the 'secure' field setting that was causing this bug.  By doing these two things at the right time, we avoid a complex configuration where OSX has taken key-focus from another window but has not yet taken it's main focus - that could end up with the wrong window having focus.

Comment 17

[David Parks [:handyman]]

Try server results:
https://tbpl.mozilla.org/?tree=Try&rev=84bf98e735e0

Comment 18

[Neil Deakin]

Comment on attachment 8477863 [details] [diff] [review]
1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

>+  if (mInputContext.IsPasswordEditor()) {
>+    TextInputHandler::EnableSecureEventInput();
>+  } else {
>+    TextInputHandler::EnsureSecureEventInputDisabled();
>+  }
>+

Why don't you just do this directly in nsWindowMap::windowBecameKey and not need to change the rest of the code?

Comment 19

[David Parks [:handyman]]

(In reply to Neil Deakin from comment #18)
> Comment on attachment 8477863 [details] [diff] [review]
> 1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.
> 
> >+  if (mInputContext.IsPasswordEditor()) {
> >+    TextInputHandler::EnableSecureEventInput();
> >+  } else {
> >+    TextInputHandler::EnsureSecureEventInputDisabled();
> >+  }
> >+
> 
> Why don't you just do this directly in nsWindowMap::windowBecameKey and not
> need to change the rest of the code?


The main issue there is mInputContext, which is found in the nsChildView that is active, according to the viewManager of the Window that has focus.  The other callbacks go through the chain of objects I did:

nsWindowMap -> nsCocoaWindow -> nsWebShellWindow -> nsFocusManager -> nsChildView 

If there is a simpler way, Im missing it so just let me know.  I think the original implementations were designed to reduce class inter-dependencies but have gotten 'wordy'.

Comment 20

[David Parks [:handyman]]

NI'ed to get your thoughts on comment 19.

Comment 21

[Neil Deakin]

Isn't the focused child view when windowBecameKey is called the same as [window firstResponder]?

So really the problem here is that nsIWidgetListener::WindowActivated should be called when the window becomes the key window, not when it becomes the main window as it appears to do now (with the method renamed to WindowsFocused probably). Then a separate WindowActivated method is added to update the active state on the window and fire the activate event. That's a bit more work though.

Comment 22

[David Parks [:handyman]]

Indeed, it seems [window firstResponder] is the right object.  That's easy and will reduce some clutter.

On your bigger point, I think you are suggesting that becomeMainWindow should do very little and becomeKeyWindow should basically do all of the work (ie becomeKeyWindow calls the existing WindowActivated and becomeMainWindow calls some new routine that 'updates the active state on the window and fires the activate event').  Something like that was initially the plan but the design of the key and main window handlers looks to have been established to cover a bunch of cases I don't understand (sheets and non-embedders?) so I'm loathe to rip it all out without more clarity.  Much worse, the focus code is very order-dependent and platform-generic -- it would take a root canal to break the concepts (the big job is nsFocusManager, where WindowActivated means WindowRaised, which is obviously wrong on mac and would need to be broken up to support the idea of key focus).  I'd like to see the module 'fixed' wrt this but I think the potential for mass regressions is unwarranted when the bug is so contained.  But my concerns may be because I'm new to this code.  Can you give me a better idea of how you would divide the two implementations (your hypothetical "WindowActivated" and "WindowFocused")?

Comment 23

[David Parks [:handyman]]

Attachment: 1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

This is a version of the patch without all of the platform-independent stuff.  It still fixes this bug.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=d2b20c9509a6

Comment 24

[Neil Deakin]

I wasn't suggesting to split activating and focusing in this bug. I filed bug 1059910 for that, which is a larger task.
 
This isn't specifically a Mac issue. All three platforms distinguish between active windows, frontmost windows and focused windows to different degrees. However, apart from some terminology changes, we should be able to leave the other platforms mostly as is.

Generally, we would need to move out the calls to window->ActivateOrDeactivate and to fire the activate events from nsFocusManager::WindowRaised. However, there could be some confusion over which window to report via the focus manager properties. I think doing this would then allow us to remove the manual activation notifications in nsCocoaWindow::Show to handle sheets.

Comment 27

[David Parks [:handyman]]

I need a bit of clarification - I think I may have misunderstood what you were saying.  Are you asking for more work on this patch or were your comments intended as notes for bug 1059910?  I'd interpreted your comments in comment 21 and comment 24 as snippets related to approving this patch but they don't seem to be functional outside of a full solution to bug 1059910.  If they are supposed to be part of this patch, can you give me a few more hints about how to go about implementing them?  I've been trying but every thread I pull immediately ends up encompassing the broader bug.

Comment 28

[Neil Deakin]

I wrote that comment before filing the bug. It would indeed be more relevant to be in the other bug, 1059910.

I was a bit hesitant about this patch as I'm not entirely sure if [window firstResponder] is always the same as the focused window in some special case with embedding or plugins or something, but a Mac reviewer would have more insight to this.

Comment 29

[David Parks [:handyman]]

Thanks, that's basically what I'd figured.  I am also not 100% certain on the firstResponder setting -- that concern was the origin of the first ("wordy") patch (attachment 8477863 [details] [diff] [review] -- it doesn't have the ambiguity as it follows the pattern of the existing main-focus handlers).  But someone will know if the shortcut is safe.  I'll ask around.  (firstResponder is used in the key-focus case already so I'm pretty confident it'll get blessed.)

If you think its important to resolve bug 1059910 quickly for this issue, let me know and I'll work on developing what I've got already.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8480347 [details] [diff] [review]
1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

This looks fine to me.  And I think all these patches together should get rid of at least the non-plugin crashes in -[ChildView keyDown:].

But things are still not right:  In step 3 of the STR from comment #2, InputContext::IsPasswordEditor() returns TRUE, as it should.  But by the end of step 4 (after you've Cmd+Tab-ed away and then back again), IsPasswordEditor() is no longer true, even though the I-beam cursor is still in the password field.

This problem doesn't happen in current trunk code, in either kind of window (e10s or "normal").  So we'll presumably need to deal with this in a followup bug to this one.

David, do you want to take care of this? :-)

Comment 32

[Steven Michaud [:smichaud] (Retired)]

> But things are still not right:  In step 3 of the STR from comment
> #2, InputContext::IsPasswordEditor() returns TRUE, as it should.
> But by the end of step 4 (after you've Cmd+Tab-ed away and then back
> again), IsPasswordEditor() is no longer true, even though the I-beam
> cursor is still in the password field.

This is only true for an e10s window.  IsPasswordEditor() is set
correctly for a non-e10s window.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

A note on -[NSWindow firstResponder]:

This can be any object belonging to (or inheriting from) the NSResponder class.  Or it can be NULL.  If I remember right, the "default" value for [NSWindow firstResponder] is a pointer to the window itself, which is more or less equivalent to setting it to NULL.  But once you click anywhere in the window, its first responder becomes frontmost NSView you clicked on.  In Mozilla's case this is usually a ChildView object.  Or since Markus Stange's changes to allow Gecko code to draw in the titlebar, it may always be a ChildView object.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

To distinguish this bug from bug 893973.

Comment 35

[David Parks [:handyman]]

Comment on attachment 8480347 [details] [diff] [review]
1. Distinguish between MacOS DidBecomeMain and DidBecomeKey events.

Thanks Steven, that turned out to be pretty enlightening.

The comments in the review caused me to revisit some of the behavior in this patch... and now I'm invalidating it.  There is a broader issue to resolve and it exists on all platforms.  I'm finishing up a new patch that addresses the core issue.

Comment 36

[David Parks [:handyman]]

Attachment: 1. Handle InputContexts for separate processes' content trees separately

This patch resolves an issue where all trees in the forest of content trees shared an InputContext (on the OS's widget object).  That created a situation where someone (say, the nsFocusManager) would ask the widget for the current InputContext for content it is adjusting focus of (say, chrome), expecting it to be aligned with the content in question, but the InputContext would have values from some other content (say, tab content).  This confuses the InputContext, IMEStateManager, nsFocusManager and a bunch of other stuff.  It fails on Mac because of this special MOZ_CRASH test that we trigger but I've confirmed Windows gets into a confused IMEState by the same STR.

My solution is to maintain *two* input states in the widget, one for the main/chrome content and one for any 'active' tab content (inactive tabs are implicitly DISABLED).  Clearly, at least one of these should be disabled (chrome and tabs can't *both* have focus) but this is violated, at least on mac, very rarely (I'm not sure of the cause but I've actually only seen it around init time on non-remote 'about' pages).  I have not seen this cause an actual issue.

With this patch, nsFocusManager/IMEStateManager/etc behavior talks to the widget through PuppetWidget, which now maintains a copy of the InputContext that is used for its content (the other copy is in the widget on the main process).

The widget disambiguates the contexts in SetInputContext.  Only a context that is/will-be active gets to update system IME state.  (When only one context is active, there is no conflict.)  The test that sparks this crash is changed to test only the active IME state, if any.

Finally, the EventStateManager was swallowing events that don't target tab content.  This means that content never got a chance to DISABLE itself when chrome took focus.  I've changed it to send messages when tab content has focus which fixes that issue.

This patch works for Mac and Windows but, looking at the code, it seems pretty certain that Android and Linux have the same bug.  This patch will not actually affect their behavior.  If tryserver tests bless this patch then I'll probably try to commit it quickly and create a new bug for those two.

Comment 37

[David Parks [:handyman]]

Tryserver results:
https://tbpl.mozilla.org/?tree=Try&rev=f53d8b8e3629

Comment 38

[David Parks [:handyman]]

Attachment: 1. Handle InputContexts for separate processes' content trees separately

Tryserver tests look good.  I'm refreshing the patch to remove extraneous comments.

Steven, would you mind reviewing this new patch?  Comment 36 has notes on what it does.

Comment 39

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

Comment on attachment 8491094 [details] [diff] [review]
1. Handle InputContexts for separate processes' content trees separately

> In e10s, each nsFocusManger (main/chrome and content/tab) operates on
> its own content tree with its own InputContext.  They also communicate
> their InputContexts with the OS widget (nsChildView on mac, nsWindow
> on Windows).  Prior to this patch, the OS widgets didn't distinguish
> between main and content InputContexts.  When GetInputContext was
> called on the OS widget, it would return whatever InputContext it
> had set last.  This patch distinguishes between InputContexts in the
> widget.

Hmm, I don't understand the your new design yet. The InputContext is synchronized with IME state of its related native IME context. So, managing two input context per one native IME context doesn't make sense to me...

>@@ -1204,16 +1204,25 @@ EventStateManager::HandleCrossProcessEve
>       aEvent->message == NS_TOUCH_START) {
>     // If this event only has one target, and it's remote, add it to
>     // the array.
>     nsIFrame* frame = GetEventTarget();
>     nsIContent* target = frame ? frame->GetContent() : nullptr;
>     if (IsRemoteTarget(target)) {
>       targets.AppendElement(target);
>     }
>+    else {

nit: Use |} else {| style.

>+  /**
>+   * Returns the currently active input context (ie main or remote-content).
>+   * Returns nullptr if no context is active or if both are active (a rare case
>+   * that seems to happen briefly due to OS window event order).
>+   */
>+  InputContext *GetActiveInputContext();

So, one of two input context is active, I feel storing only the active input context does make sense...

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -473,16 +473,19 @@ nsChildView::nsChildView() : nsBaseWidge
> , mDrawing(false)
> , mPluginDrawing(false)
> , mIsDispatchPaint(false)
> , mPluginInstanceOwner(nullptr)
> {
>   EnsureLogInitialized();
> 
>   memset(&mPluginCGContext, 0, sizeof(mPluginCGContext));
>+
>+  mMainInputContext.mIMEState.mEnabled = IMEState::DISABLED;
>+  mContentInputContext.mIMEState.mEnabled = IMEState::DISABLED;

For the safer, we're using ENABLED for its initial value because IME is available unexpectedly is better than IME is not available unexpectedly.

>@@ -1977,72 +1976,104 @@ nsChildView::NotifyIME(const IMENotifica
> }
> 
> NS_IMETHODIMP_(void)
> nsChildView::SetInputContext(const InputContext& aContext,
>                              const InputContextAction& aAction)
> {
>   NS_ENSURE_TRUE_VOID(mTextInputHandler);
> 
>-  if (mTextInputHandler->IsFocused()) {
>+  InputContext& inputContext = 
>+    aAction.HasRemoteOrigin() ? mContentInputContext : mMainInputContext;
>+  IMEState::Enabled oldContextEnabled = inputContext.mIMEState.mEnabled;
>+
>+  // If either the old or new context is not DISABLED and this view has focus
>+  // then we properly update the OS secure-input state.
>+  if (mTextInputHandler->IsFocusedOnKeyFocus() &&
>+    (aContext.mIMEState.mEnabled != IMEState::DISABLED ||
>+      oldContextEnabled != IMEState::DISABLED)) {

I don't understand why we need to check if one of them are not disabled.

>     if (aContext.IsPasswordEditor()) {
>       TextInputHandler::EnableSecureEventInput();
>     } else {
>       TextInputHandler::EnsureSecureEventInputDisabled();
>     }
>   }
> 
>-  mInputContext = aContext;
>+  inputContext = aContext;
>+
>+  // Update TextInputHandler to new state unless state is
>+  // transitioning from DISABLED to DISABLED.
>   switch (aContext.mIMEState.mEnabled) {

So, the native IME event handler state is set for the last set input context which may be either chrome's or content's.

>     case IMEState::ENABLED:
>     case IMEState::PLUGIN:
>       mTextInputHandler->SetASCIICapableOnly(false);
>       mTextInputHandler->EnableIME(true);
>-      if (mInputContext.mIMEState.mOpen != IMEState::DONT_CHANGE_OPEN_STATE) {
>+      if (inputContext.mIMEState.mOpen != IMEState::DONT_CHANGE_OPEN_STATE) {
>         mTextInputHandler->SetIMEOpenState(
>-          mInputContext.mIMEState.mOpen == IMEState::OPEN);
>+          inputContext.mIMEState.mOpen == IMEState::OPEN);
>       }
>       break;
>     case IMEState::DISABLED:
>-      mTextInputHandler->SetASCIICapableOnly(false);
>-      mTextInputHandler->EnableIME(false);
>+      if (oldContextEnabled != IMEState::DISABLED) {
>+        mTextInputHandler->SetASCIICapableOnly(false);
>+        mTextInputHandler->EnableIME(false);
>+      }
>       break;
>     case IMEState::PASSWORD:
>       mTextInputHandler->SetASCIICapableOnly(true);
>       mTextInputHandler->EnableIME(false);
>       break;
>     default:
>       NS_ERROR("not implemented!");
>   }
> }
> 
> NS_IMETHODIMP_(InputContext)
> nsChildView::GetInputContext()
> {
>-  switch (mInputContext.mIMEState.mEnabled) {
>+  // Always use the input context for the main proc.  Content proc's
>+  // input context is obtained from the PuppetWidget in the content proc.
>+  switch (mMainInputContext.mIMEState.mEnabled) {
>     case IMEState::ENABLED:
>     case IMEState::PLUGIN:
>       if (mTextInputHandler) {
>-        mInputContext.mIMEState.mOpen =
>+        mMainInputContext.mIMEState.mOpen =
>           mTextInputHandler->IsIMEOpened() ? IMEState::OPEN : IMEState::CLOSED;
>-        break;
>       }
>-      // If mTextInputHandler is null, set CLOSED instead...
>+      else {
>+        mMainInputContext.mIMEState.mOpen = IMEState::CLOSED;
>+      }
>+      break;
>     default:
>-      mInputContext.mIMEState.mOpen = IMEState::CLOSED;
>+      mMainInputContext.mIMEState.mOpen = IMEState::CLOSED;
>       break;
>   }
>-  mInputContext.mNativeIMEContext = [mView inputContext];
>+  mMainInputContext.mNativeIMEContext = [mView inputContext];
>   // If input context isn't available on this widget, we should set |this|
>   // instead of nullptr since nullptr means that the platform has only one
>   // context per process.
>-  if (!mInputContext.mNativeIMEContext) {
>-    mInputContext.mNativeIMEContext = this;
>-  }
>-  return mInputContext;
>+  if (!mMainInputContext.mNativeIMEContext) {
>+    mMainInputContext.mNativeIMEContext = this;
>+  }
>+  return mMainInputContext;

However, you always return chrome's input context. This may mismatch with native IME state if content's input context was set. This means that if their states are different, chrome's IMEStateManager may fail change IME state.

>+InputContext *
>+nsChildView::GetActiveInputContext()
>+{
>+  if (mContentInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
>+    if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
>+      return nullptr;
>+    }
>+    return &mContentInputContext;
>+  }
>+  if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
>+    return &mMainInputContext;
>+  }
>+  return nullptr;
> }

I don't understand this method what does... InputContext has other information of focused editor if its IMEState is editable. Therefore, this may return wrong information to the caller...

>@@ -5694,17 +5725,18 @@ static int32_t RoundUp(double aDouble)
>   // otherwise why is the OS sending us a key down event?  But it's just
>   // possible we're in Gecko's hidden window, so we check first.
>   NSWindow *viewWindow = [self window];
>   if (viewWindow && [viewWindow isKeyWindow]) {
>     [viewWindow orderWindow:NSWindowAbove relativeTo:0];
>   }
> 
> #if !defined(RELEASE_BUILD) || defined(DEBUG)
>-  if (mGeckoChild && mTextInputHandler && mTextInputHandler->IsFocused()) {
>+  InputContext *activeIC = mGeckoChild ? mGeckoChild->GetActiveInputContext() : nullptr;

nit: * should be the after of the type name. I.e., |InputContext* activeIC...|

>+  if (activeIC && mTextInputHandler && mTextInputHandler->IsFocused()) {

We won't log the state when there is no focused editor?

> #ifdef MOZ_CRASHREPORTER
>     NSWindow* window = [self window];
>     NSString* info = [NSString stringWithFormat:@"\nview [%@], window [%@], key event [%@], window is key %i, is fullscreen %i, app is active %i",
>                       self, window, theEvent, [window isKeyWindow], ([window styleMask] & (1 << 14)) != 0,
>                       [NSApp isActive]];
>     nsAutoCString additionalInfo([info UTF8String]);
> #endif
>     if (mIsPluginView) {
>@@ -5713,27 +5745,27 @@ static int32_t RoundUp(double aDouble)
> #ifdef MOZ_CRASHREPORTER
>         CrashReporter::AppendAppNotesToCrashReport(NS_LITERAL_CSTRING("\nBug 893973: ") +
>                                                    NS_LITERAL_CSTRING(CRASH_MESSAGE));
>         CrashReporter::AppendAppNotesToCrashReport(additionalInfo);
> #endif
>         MOZ_CRASH(CRASH_MESSAGE);
>         #undef CRASH_MESSAGE
>       }
>-    } else if (mGeckoChild->GetInputContext().IsPasswordEditor() &&
>+    } else if (activeIC->IsPasswordEditor() &&

I don't understand why this means that the focused editor is password field because active input context is just editable input context of them.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -448,34 +448,50 @@ struct InputContextAction {
>     // keyboard events will be handled by menu.
>     MENU_GOT_PSEUDO_FOCUS,
>     // Menu lost pseudo focus that means focused content will handle keyboard
>     // events.
>     MENU_LOST_PSEUDO_FOCUS
>   };
>   FocusChange mFocusChange;
> 
>+  /**
>+   * mOrigin indicates whether this focus event refers to local or remote content.
>+   */
>+  enum Origin {

nit:

enum Origin
{

>+    // Adjusting focus of content local to this process
>+    ORIGIN_LOCAL,
>+    // Adjusting focus of content in a remote process
>+    ORIGIN_REMOTE
>+  };
>+  Origin mOrigin;
>+
>   bool ContentGotFocusByTrustedCause() const {
>     return (mFocusChange == GOT_FOCUS &&
>             mCause != CAUSE_UNKNOWN);
>   }
> 
>   bool UserMightRequestOpenVKB() const {
>     return (mFocusChange == FOCUS_NOT_CHANGED &&
>             mCause == CAUSE_MOUSE);
>   }
> 
>+  bool HasRemoteOrigin() const {
>+    return mOrigin == ORIGIN_REMOTE;
>+  }
>+
>   InputContextAction() :
>-    mCause(CAUSE_UNKNOWN), mFocusChange(FOCUS_NOT_CHANGED)
>+    mCause(CAUSE_UNKNOWN), mFocusChange(FOCUS_NOT_CHANGED), mOrigin(ORIGIN_LOCAL)
>   {
>   }

Could you change the coding style to the latest?

I.e.,

InputContextAction()
  : mCause(CAUSE_UNKNONW)
  , mFocusChange(FOCUS_NOT_CHANGED)
  , mOrigin(ORIGIN_LOCAL)
{
}

> 
>   explicit InputContextAction(Cause aCause,
>-                              FocusChange aFocusChange = FOCUS_NOT_CHANGED) :
>-    mCause(aCause), mFocusChange(aFocusChange)
>+                              FocusChange aFocusChange = FOCUS_NOT_CHANGED,
>+                              Origin aOrigin = ORIGIN_LOCAL) :
>+    mCause(aCause), mFocusChange(aFocusChange), mOrigin(aOrigin)

Same here.

>@@ -6822,31 +6823,42 @@ nsWindow::NotifyIME(const IMENotificatio
> {
>   return IMEHandler::NotifyIME(this, aIMENotification);
> }
> 
> NS_IMETHODIMP_(void)
> nsWindow::SetInputContext(const InputContext& aContext,
>                           const InputContextAction& aAction)
> {
>-  InputContext newInputContext = aContext;
>-  IMEHandler::SetInputContext(this, newInputContext, aAction);
>-  mInputContext = newInputContext;
>+  InputContext &inputContext =
>+    (aAction.HasRemoteOrigin()) ? mContentInputContext : mMainInputContext;
>+
>+  // Update IMEHandler state with new state unless we are transitioning
>+  // from DISABLED to DISABLED.
>+  if ((inputContext.mIMEState.mEnabled != IMEState::DISABLED) ||
>+    (aContext.mIMEState.mEnabled != IMEState::DISABLED))  {

I don't understand why you need to limit the case to notify our native IME event handlers of IME state change. They may assume that it's notified always.

>+    InputContext newInputContext = aContext;
>+    IMEHandler::SetInputContext(this, newInputContext, aAction);
>+    inputContext = newInputContext;
>+  }
>+  else {
>+    inputContext = aContext;
>+  }
> }
> 
> NS_IMETHODIMP_(InputContext)
> nsWindow::GetInputContext()
> {
>-  mInputContext.mIMEState.mOpen = IMEState::CLOSED;
>-  if (WinUtils::IsIMEEnabled(mInputContext) && IMEHandler::GetOpenState(this)) {
>-    mInputContext.mIMEState.mOpen = IMEState::OPEN;
>+  if (WinUtils::IsIMEEnabled(mMainInputContext) &&
>+      IMEHandler::GetOpenState(this)) {

IMEHandler may have been notified content's state. Why this always checks chrome's input context but returns actual IME open state?

>+nsWindowBase::nsWindowBase()
>+{
>+  mMainInputContext.mIMEState.mEnabled = IMEState::DISABLED;
>+  mContentInputContext.mIMEState.mEnabled = IMEState::DISABLED;
>+}

On Windows, IME is enabled in default state. So, initializing with disabled state is wrong.

And you don't update MetroWidget... You should do it.

Additionally, you don't update GTK, Qt, Android and Gonk too.


I guess that your patch works well at least with *current* IMEStateManager. But at reading the widget's input context management, the code does not make sense because doubled input context is hard to understand why they are necessary even though zero or one input context can be active.

I believe that we shouldn't change widget level input context management.

Comment 40

[David Parks [:handyman]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #39)
> Comment on attachment 8491094 [details] [diff] [review]
> 1. Handle InputContexts for separate processes' content trees separately
> 
> > In e10s, each nsFocusManger (main/chrome and content/tab) operates on
> > its own content tree with its own InputContext.  They also communicate
> > their InputContexts with the OS widget (nsChildView on mac, nsWindow
> > on Windows).  Prior to this patch, the OS widgets didn't distinguish
> > between main and content InputContexts.  When GetInputContext was
> > called on the OS widget, it would return whatever InputContext it
> > had set last.  This patch distinguishes between InputContexts in the
> > widget.
> 
> Hmm, I don't understand the your new design yet. The InputContext is
> synchronized with IME state of its related native IME context. So, managing
> two input context per one native IME context doesn't make sense to me...

You are right that the content InputContext is extraneous -- it could be replaced by a boolean that indicates whether the input context in the widget is from an ORIGIN_REMOTE or ORIGIN_LOCAL action.  The other can be assumed to be DISABLED (ignoring the init-time case I mentioned earlier).  The PuppetWidget change made all of this unnecessary.  I'll make the change.


> >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
> >--- a/widget/cocoa/nsChildView.mm
> >+++ b/widget/cocoa/nsChildView.mm
> >@@ -473,16 +473,19 @@ nsChildView::nsChildView() : nsBaseWidge
> > , mDrawing(false)
> > , mPluginDrawing(false)
> > , mIsDispatchPaint(false)
> > , mPluginInstanceOwner(nullptr)
> > {
> >   EnsureLogInitialized();
> > 
> >   memset(&mPluginCGContext, 0, sizeof(mPluginCGContext));
> >+
> >+  mMainInputContext.mIMEState.mEnabled = IMEState::DISABLED;
> >+  mContentInputContext.mIMEState.mEnabled = IMEState::DISABLED;
> 
> For the safer, we're using ENABLED for its initial value because IME is
> available unexpectedly is better than IME is not available unexpectedly.

This seems to get set anyway but I couldn't have both ENABLED.  With the change you suggest above, that becomes irrelevant (I'll change it to one InputContext that is ENABLED).


> >@@ -1977,72 +1976,104 @@ nsChildView::NotifyIME(const IMENotifica
> > }
> > 
> > NS_IMETHODIMP_(void)
> > nsChildView::SetInputContext(const InputContext& aContext,
> >                              const InputContextAction& aAction)
> > {
> >   NS_ENSURE_TRUE_VOID(mTextInputHandler);
> > 
> >-  if (mTextInputHandler->IsFocused()) {
> >+  InputContext& inputContext = 
> >+    aAction.HasRemoteOrigin() ? mContentInputContext : mMainInputContext;
> >+  IMEState::Enabled oldContextEnabled = inputContext.mIMEState.mEnabled;
> >+
> >+  // If either the old or new context is not DISABLED and this view has focus
> >+  // then we properly update the OS secure-input state.
> >+  if (mTextInputHandler->IsFocusedOnKeyFocus() &&
> >+    (aContext.mIMEState.mEnabled != IMEState::DISABLED ||
> >+      oldContextEnabled != IMEState::DISABLED)) {
> 
> I don't understand why we need to check if one of them are not disabled.

Ah, this is the core cause of the bug.  Only an enabled context can (and should) change system state.  There is nothing that DISABLED windows should have to say to the system -- if the other side is active then it expects to have total control over those calls (and it 'sort of' counts them and crashes when someone else screws up its count).  This transition (DISABLED->DISABLED) is the only one to ignore.  If some content is entering OR exiting a state other than DISABLED then it must 'have' or 'be taking' control of the system IME state.


> 
> >     if (aContext.IsPasswordEditor()) {
> >       TextInputHandler::EnableSecureEventInput();
> >     } else {
> >       TextInputHandler::EnsureSecureEventInputDisabled();
> >     }
> >   }
> > 
> >-  mInputContext = aContext;
> >+  inputContext = aContext;
> >+
> >+  // Update TextInputHandler to new state unless state is
> >+  // transitioning from DISABLED to DISABLED.
> >   switch (aContext.mIMEState.mEnabled) {
> 
> So, the native IME event handler state is set for the last set input context
> which may be either chrome's or content's.
<snip>
> However, you always return chrome's input context. This may mismatch with
> native IME state if content's input context was set. This means that if
> their states are different, chrome's IMEStateManager may fail change IME
> state.

nsChildView::GetInputContext is used as the widget implementation of GetInputContext on the main process and PuppetWidget::GetInputContext is used on the child process.  So when each process' nsFocusManager, for example, calls GetInputContext on the 'widget', it gets the one for the process it's using.  Part of this crash was the IMEStateManager checking to see if a WindowActivated call was changing IMEState... by comparing it's active window's desired state (which is DISABLED in the STR since the chrome doesnt have focus) against it's current state (PASSWORD since the widget returns the IMEState for the wrong content tree).  It then changes (DISABLEs) the state when the tab content has set it to PASSWORD, causing everything to go haywire.  Now it *can't* get out of sync because they don't even have access to one another.


> 
> >+InputContext *
> >+nsChildView::GetActiveInputContext()
> >+{
> >+  if (mContentInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> >+    if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> >+      return nullptr;
> >+    }
> >+    return &mContentInputContext;
> >+  }
> >+  if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> >+    return &mMainInputContext;
> >+  }
> >+  return nullptr;
> > }
> 
> I don't understand this method what does... InputContext has other
> information of focused editor if its IMEState is editable. Therefore, this
> may return wrong information to the caller...

I'm not sure I get what you mean.  This function is minor support to the MOZ_CRASH test in keyDown but it's supposed to help keyDown to know which context to test against system state (ie which context is not DISABLED).  You are right that the DISABLED case should be checked - this will be trivial when I replace the second InputContext with a boolean as suggested.

> >+  if (activeIC && mTextInputHandler && mTextInputHandler->IsFocused()) {
> 
> We won't log the state when there is no focused editor?

I take it you are referring to the "mTextInputHandler->IsFocused()" part but... that wasn't me :)

> 
> > #ifdef MOZ_CRASHREPORTER
> >     NSWindow* window = [self window];
> >     NSString* info = [NSString stringWithFormat:@"\nview [%@], window [%@], key event [%@], window is key %i, is fullscreen %i, app is active %i",
> >                       self, window, theEvent, [window isKeyWindow], ([window styleMask] & (1 << 14)) != 0,
> >                       [NSApp isActive]];
> >     nsAutoCString additionalInfo([info UTF8String]);
> > #endif
> >     if (mIsPluginView) {
> >@@ -5713,27 +5745,27 @@ static int32_t RoundUp(double aDouble)
> > #ifdef MOZ_CRASHREPORTER
> >         CrashReporter::AppendAppNotesToCrashReport(NS_LITERAL_CSTRING("\nBug 893973: ") +
> >                                                    NS_LITERAL_CSTRING(CRASH_MESSAGE));
> >         CrashReporter::AppendAppNotesToCrashReport(additionalInfo);
> > #endif
> >         MOZ_CRASH(CRASH_MESSAGE);
> >         #undef CRASH_MESSAGE
> >       }
> >-    } else if (mGeckoChild->GetInputContext().IsPasswordEditor() &&
> >+    } else if (activeIC->IsPasswordEditor() &&
> 
> I don't understand why this means that the focused editor is password field
> because active input context is just editable input context of them.

This is still just the ASSERT code but I think it should only compare the system IME state with the active content (or either if both are DISABLED) since that's the (only) one that should be synchronized with the system state.


> >@@ -6822,31 +6823,42 @@ nsWindow::NotifyIME(const IMENotificatio
> > {
> >   return IMEHandler::NotifyIME(this, aIMENotification);
> > }
> > 
> > NS_IMETHODIMP_(void)
> > nsWindow::SetInputContext(const InputContext& aContext,
> >                           const InputContextAction& aAction)
> > {
> >-  InputContext newInputContext = aContext;
> >-  IMEHandler::SetInputContext(this, newInputContext, aAction);
> >-  mInputContext = newInputContext;
> >+  InputContext &inputContext =
> >+    (aAction.HasRemoteOrigin()) ? mContentInputContext : mMainInputContext;
> >+
> >+  // Update IMEHandler state with new state unless we are transitioning
> >+  // from DISABLED to DISABLED.
> >+  if ((inputContext.mIMEState.mEnabled != IMEState::DISABLED) ||
> >+    (aContext.mIMEState.mEnabled != IMEState::DISABLED))  {
> 
> I don't understand why you need to limit the case to notify our native IME
> event handlers of IME state change. They may assume that it's notified
> always.

Same as the Mac version, this is based on trying to get system state to only synchronize with one content tree at a time.

> >+    InputContext newInputContext = aContext;
> >+    IMEHandler::SetInputContext(this, newInputContext, aAction);
> >+    inputContext = newInputContext;
> >+  }
> >+  else {
> >+    inputContext = aContext;
> >+  }
> > }
> > 
> > NS_IMETHODIMP_(InputContext)
> > nsWindow::GetInputContext()
> > {
> >-  mInputContext.mIMEState.mOpen = IMEState::CLOSED;
> >-  if (WinUtils::IsIMEEnabled(mInputContext) && IMEHandler::GetOpenState(this)) {
> >-    mInputContext.mIMEState.mOpen = IMEState::OPEN;
> >+  if (WinUtils::IsIMEEnabled(mMainInputContext) &&
> >+      IMEHandler::GetOpenState(this)) {
> 
> IMEHandler may have been notified content's state. Why this always checks
> chrome's input context but returns actual IME open state?

Oops.  Clearly that's how the code worked before but this isn't useful anymore.  Of course, the only case where this is an issue is when the InputContext is DISABLED... so Open-ness wouldn't exactly be important... but its wrong.  I'll fix that too.  I'd still suggest setting mOpen in the non-DISABLED cases tho as I think that state gets messed with in other parts of the Windows implementation.

> 
> >+nsWindowBase::nsWindowBase()
> >+{
> >+  mMainInputContext.mIMEState.mEnabled = IMEState::DISABLED;
> >+  mContentInputContext.mIMEState.mEnabled = IMEState::DISABLED;
> >+}
> 
> On Windows, IME is enabled in default state. So, initializing with disabled
> state is wrong.

Same as Mac.  I'll take it out also when I drop the second InputContext.

> And you don't update MetroWidget... You should do it.
> Additionally, you don't update GTK, Qt, Android and Gonk too.

OK.

> I guess that your patch works well at least with *current* IMEStateManager.
> But at reading the widget's input context management, the code does not make
> sense because doubled input context is hard to understand why they are
> necessary even though zero or one input context can be active.
> 
> I believe that we shouldn't change widget level input context management.

I'm not sure if I've clarified that but the overall gist is this:

The services involved here (the nsFocusManager, IMEStateHandler, etc) expect to have control over their widget.  They synchronize its state with the relevant content.  But they can't synchronize it with *two* contents -- fortunately, at least one should always be DISABLED.  By not distinguishing the two in the widget, and then returning the information for the wrong state (via GetInputHandler), we confuse the services.

Thoughts?  Let me know if this still sounds wrong.  In the mean time, I'll implement the changes.

Comment 41

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

(In reply to David Parks [:handyman] from comment #40)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23,
> JST) from comment #39)
> > Comment on attachment 8491094 [details] [diff] [review]
> > 1. Handle InputContexts for separate processes' content trees separately
> > 
> > > In e10s, each nsFocusManger (main/chrome and content/tab) operates on
> > > its own content tree with its own InputContext.  They also communicate
> > > their InputContexts with the OS widget (nsChildView on mac, nsWindow
> > > on Windows).  Prior to this patch, the OS widgets didn't distinguish
> > > between main and content InputContexts.  When GetInputContext was
> > > called on the OS widget, it would return whatever InputContext it
> > > had set last.  This patch distinguishes between InputContexts in the
> > > widget.
> > 
> > Hmm, I don't understand the your new design yet. The InputContext is
> > synchronized with IME state of its related native IME context. So, managing
> > two input context per one native IME context doesn't make sense to me...
> 
> You are right that the content InputContext is extraneous -- it could be
> replaced by a boolean that indicates whether the input context in the widget
> is from an ORIGIN_REMOTE or ORIGIN_LOCAL action.  The other can be assumed
> to be DISABLED (ignoring the init-time case I mentioned earlier).  The
> PuppetWidget change made all of this unnecessary.  I'll make the change.

I don't understand well, though. But sounds better to me. I'd like to see the next patch. While content process has focus, InputContext should represents the state of content process's. Otherwise, parent process's. Then, it's clear for others in widget module.

So, not managing two context in widget, you can make the patch much simpler.

> > > NS_IMETHODIMP_(void)
> > > nsChildView::SetInputContext(const InputContext& aContext,
> > >                              const InputContextAction& aAction)
> > > {
> > >   NS_ENSURE_TRUE_VOID(mTextInputHandler);
> > > 
> > >-  if (mTextInputHandler->IsFocused()) {
> > >+  InputContext& inputContext = 
> > >+    aAction.HasRemoteOrigin() ? mContentInputContext : mMainInputContext;
> > >+  IMEState::Enabled oldContextEnabled = inputContext.mIMEState.mEnabled;
> > >+
> > >+  // If either the old or new context is not DISABLED and this view has focus
> > >+  // then we properly update the OS secure-input state.
> > >+  if (mTextInputHandler->IsFocusedOnKeyFocus() &&
> > >+    (aContext.mIMEState.mEnabled != IMEState::DISABLED ||
> > >+      oldContextEnabled != IMEState::DISABLED)) {
> > 
> > I don't understand why we need to check if one of them are not disabled.
> 
> Ah, this is the core cause of the bug.  Only an enabled context can (and
> should) change system state.  There is nothing that DISABLED windows should
> have to say to the system

Yes, that's true. However, if this is necessary, it means that context does not match with focused content because aContext.IsPasswordEditor() should be enough and SetInputContext() should be called only for the focused content of the all processes. So, this check should be removed and the root cause should be fixed.

> > >-  mInputContext = aContext;
> > >+  inputContext = aContext;
> > >+
> > >+  // Update TextInputHandler to new state unless state is
> > >+  // transitioning from DISABLED to DISABLED.
> > >   switch (aContext.mIMEState.mEnabled) {
> > 
> > So, the native IME event handler state is set for the last set input context
> > which may be either chrome's or content's.
> <snip>
> > However, you always return chrome's input context. This may mismatch with
> > native IME state if content's input context was set. This means that if
> > their states are different, chrome's IMEStateManager may fail change IME
> > state.
> 
> nsChildView::GetInputContext is used as the widget implementation of
> GetInputContext on the main process and PuppetWidget::GetInputContext is
> used on the child process.

Yeah. And I believe that both of them should return same value. If PuppetWidget caches the value, parent process's SetInputContext() should tell the change to PuppetWidget if it's changed in parent process.

> So when each process' nsFocusManager, for
> example, calls GetInputContext on the 'widget', it gets the one for the
> process it's using.  Part of this crash was the IMEStateManager checking to
> see if a WindowActivated call was changing IMEState... by comparing it's
> active window's desired state (which is DISABLED in the STR since the chrome
> doesnt have focus)

So, I think that in this case, parent process shouldn't set input context because child process has focus. Why don't you make it so? This must be the reason why I don't understand the your approach.

> > >+InputContext *
> > >+nsChildView::GetActiveInputContext()
> > >+{
> > >+  if (mContentInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> > >+    if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> > >+      return nullptr;
> > >+    }
> > >+    return &mContentInputContext;
> > >+  }
> > >+  if (mMainInputContext.mIMEState.mEnabled != IMEState::DISABLED) {
> > >+    return &mMainInputContext;
> > >+  }
> > >+  return nullptr;
> > > }
> > 
> > I don't understand this method what does... InputContext has other
> > information of focused editor if its IMEState is editable. Therefore, this
> > may return wrong information to the caller...
> 
> I'm not sure I get what you mean.  This function is minor support to the
> MOZ_CRASH test in keyDown but it's supposed to help keyDown to know which
> context to test against system state (ie which context is not DISABLED). 
> You are right that the DISABLED case should be checked - this will be
> trivial when I replace the second InputContext with a boolean as suggested.
> 
> > >+  if (activeIC && mTextInputHandler && mTextInputHandler->IsFocused()) {
> > 
> > We won't log the state when there is no focused editor?
> 
> I take it you are referring to the "mTextInputHandler->IsFocused()" part
> but... that wasn't me :)

No, I mean why you check activeIC which may be null if both input context are disabled. Even if editable element has focus, IsFocused() returns true. IsFocused() checks native focus. On the other hand, the input context's state represents focused element in Gecko. We can call it as "pseudo focus".

> > I guess that your patch works well at least with *current* IMEStateManager.
> > But at reading the widget's input context management, the code does not make
> > sense because doubled input context is hard to understand why they are
> > necessary even though zero or one input context can be active.
> > 
> > I believe that we shouldn't change widget level input context management.
> 
> I'm not sure if I've clarified that but the overall gist is this:
> 
> The services involved here (the nsFocusManager, IMEStateHandler, etc) expect
> to have control over their widget.  They synchronize its state with the
> relevant content.  But they can't synchronize it with *two* contents --
> fortunately, at least one should always be DISABLED.  By not distinguishing
> the two in the widget, and then returning the information for the wrong
> state (via GetInputHandler), we confuse the services.
> 
> Thoughts?  Let me know if this still sounds wrong.  In the mean time, I'll
> implement the changes.

So, as I mentioned above, I believe that we should keep input context management in widget as far as possible. If parent process doesn't have focus, nsFocusManager and IMEStateManager shouldn't attempt to change input context. I don't understand why you don't make it so. Is it impossible? (I'm not familiar with e10s) Could you explain why you cannot take such approach if I'm wrong?

Comment 42

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

FYI:

I understand this bug as that input context sometimes represents wrong process's information. Therefore, I think that we should fix the setter rather than widget's input context management.

widget's code needs to struggle with native platform's bug. Therefore, its code is ugly. So, I don't like add complicated code to widget for XP level bug. And its maintain cost is expensive if same complicated code is in each platform's widget. Therefore, we should fix bugs in XP level as far as possible.

Comment 44

[David Parks [:handyman]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #41)
> 
> So, as I mentioned above, I believe that we should keep input context
> management in widget as far as possible. If parent process doesn't have
> focus, nsFocusManager and IMEStateManager shouldn't attempt to change input
> context. I don't understand why you don't make it so. Is it impossible? (I'm
> not familiar with e10s) Could you explain why you cannot take such approach
> if I'm wrong?


A few points (so that the following makes sense):

A. The main proc setting the IC when the content proc has focus is the cause of this bug.  The other way around (content setting IC when main has focus) does not happen because a hack in EventStateManager prevents the dispatch of the key event to the content proc.  This hack is the reason that the content proc doesn't immediately overwrite the main proc's IC.  Without the hack, the content proc *always* overwrites main (which is also wrong).

B. Both the parent or content proc can set the IC when they don't have focus... that's how they get focus :).  The bad stuff happens when they set the IC (in this case, to DISABLED) but the *other* proc already had focus.  A getter that recalls who set the widget's IC last (LOCAL or REMOTE) can be added to the widget.  (The old patch hid this origin information inside the widget.)

Potential solutions:

1. EventStateManager/nsPresShell should not dispatch the key event to the content proc IF the main proc will set "active" IME state.  This will require reversing the causal event chain (this event is dispatched by some of the handlers of events that make the main proc set an active state... ie we don't know if main will set an IME state until later).  This looks to be a fairly large change.  (I don't actually know how to approach this one.)  This also doesn't allow the content-proc's content to DISABLE itself when it loses focus (the current codebase has the same problem).

2. TabParent::RecvSetInputContext or nsChildView::SetInputContext should ignore calls from remote content if current widget IME setting is "active" (ENABLED/PASSWORD/PLUGIN) and not ORIGIN_REMOTE.  This seems delicate.  This is a variant of the old solution but this one assumes that, on a focus change, remote proc calls to SetInputContext happen after main proc calls to it (which it does).  The old solution used the DISABLE->DISABLE test to resolve the fragility.

3. Have the content-proc ask the widget if the current IME state is "active" and set from chrome.  If so, have the content-proc skip its calls to SetInputContext.  This is really the same solution as #2 but it adds a synchronous RPC call (to 'ask the widget' using the method proposed in point B above) instead of having the TabParent or widget filter the SetInputContext call.  This is also delicate for the same reason -- this solution needs to know that the main proc will have called SetInputContext (if its going to) before the RPC call to get the widget's IC's origin.


Option #1 seems the most robust... but that's because I don't know how to make it work.  #2 and #3 seem like weaker versions of the original idea.  I have a feeling that #1 is the desired solution but I'll need a lot of help to get it going.  Let me know if you have any suggestions on where to begin.  In the mean time, I'll look into unraveling the event chain that leads up to all of this.

Comment 45

[David Parks [:handyman]]

Attachment: 1 (alt) WIP: Make TabParent::RecvSetInputContext wise to having ownership stolen (option 2)

I took a run at option #2.  It works as expected and does resolve the crash.

Comment 46

[David Parks [:handyman]]

Attachment: 1 (alt) WIP: Make TabParent::RecvSetInputContext wise to having ownership stolen (option 2)

Doh, that was the wrong patch file.

Comment 47

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

I like the new patch much better!

But I still need more time to understand what you mentioned in comment 44. Please wait...

And I have a question. According to your patch, parent process sets InputContext as DISABLED when content process gets focus. And after that, content process sets InputContext. Is this correct? If it's correct, the order is *always* true?

Comment 48

[David Parks [:handyman]]

> And I have a question. According to your patch, parent process sets
> InputContext as DISABLED when content process gets focus. And after that,
> content process sets InputContext. Is this correct? If it's correct, the
> order is *always* true?

I am "pretty sure" it's true.  Its difficult to say for certain and, even if its true now, I'm not sure its guaranteed in the future but it's definitely true in the normal case.  Here's what happens:

1. Main proc (P1) gets the mouse click command that starts the whole thing.
2. P1 asynchronously sends the mouse event to the content proc (P2). 
3. P1 continues to process the mouse click with respect to its own focus behavior.  It *always* does this, regardless of if it had or will claim focus.  It is crucial that P1 does this as part of processing the mouse click -- therefore we are guaranteed that P1 does not process any events between the async call to P2 and updating focus.
4. Once P2 gets around to handling the mouse event, it updates the widget via the PuppetWidget/TabParent.  The TabParent filters out the update call if it would be incorrect.  The TabParent connection is the other part of the magic -- since this is how the widget gets updated by P2, the guarantee from step 3 means that the order is always true in this case.  QED.

The focus code is gargantuan.  I'm not brave enough to claim that I know that it won't do something weird in some other cases but this is why I *think* its safe.

I'll NI you so we don't forget this but take your time weighing the options.  And let me know if you've got other concerns (or if you manage to come up with a better idea).

Comment 49

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

(In reply to David Parks [:handyman] from comment #48)
> > And I have a question. According to your patch, parent process sets
> > InputContext as DISABLED when content process gets focus. And after that,
> > content process sets InputContext. Is this correct? If it's correct, the
> > order is *always* true?
> 
> I am "pretty sure" it's true.  Its difficult to say for certain and, even if
> its true now, I'm not sure its guaranteed in the future but it's definitely
> true in the normal case.

Thanks.

> Here's what happens:
> 
> 1. Main proc (P1) gets the mouse click command that starts the whole thing.
> 2. P1 asynchronously sends the mouse event to the content proc (P2). 
> 3. P1 continues to process the mouse click with respect to its own focus
> behavior.  It *always* does this, regardless of if it had or will claim
> focus.  It is crucial that P1 does this as part of processing the mouse
> click -- therefore we are guaranteed that P1 does not process any events
> between the async call to P2 and updating focus.

I think that in this case, P1 shouldn't update input context. nsFocusManager or IMEStateManager should have a state if the process has focus. If it's false, IMEStateManager should do nothing. If TabParent should do similar (i.e., the main process has focus, it should ignore the async request as outdated), we can avoid struggling with the race forever.

> 4. Once P2 gets around to handling the mouse event, it updates the widget
> via the PuppetWidget/TabParent.  The TabParent filters out the update call
> if it would be incorrect.  The TabParent connection is the other part of the
> magic -- since this is how the widget gets updated by P2, the guarantee from
> step 3 means that the order is always true in this case.  QED.
> 
> The focus code is gargantuan.  I'm not brave enough to claim that I know
> that it won't do something weird in some other cases but this is why I
> *think* its safe.

Yeah, I agree. Therefore, I still believe that only focused process sets input context is the safest way.

Is it possible?

Comment 52

[David Parks [:handyman]]

I might be missing something from what you are saying -- I'm not sure how to make what you are suggesting work.  Hopefully, it'll help if you can spell out how a few cases are supposed to work (but the cases are very involved... I've tried to simplify as much as possible but...):

* If chrome has focus, then I click on content, should chrome DISABLE focus before content ENABLEs it?
The current implementation and all patches do this.

* If content has focus, then I click chrome, should content DISABLE focus before chrome sets it?
No implementation/patch DISABLEs first.  This would require running the entire mouse-handler asynchronously (since the remote call has to complete before the main process call, and that is not allowed to be synchronous).
  * The only alternative is for content not to DISABLE focus at all (ie doing it after chrome takes focus would be wrong).  Not disabling is wrong and may have repercussions (like incorrect focus rings).  I'm not sure tho what, if any, the effects are.  The patches technically disable focus after chrome, but only locally to the content proc (either the widget filters the state transition as DISABLED->DISABLED (old patch, done in widget since only widget knows its current true IME state) or the TabParent filters the call to the OS widget (newer patch)).  I *believe* this is sufficient.  Note that, since the content proc doesn't know that the main proc has focus, it can't filter this on its own (which is why TabParent does it).  Alternatively, chrome could send special messages to content when focus has been taken.  Content could then cache this information in its proc's nsFocusManager.  Since this has to be done asynchronously (like all chrome -> content communication), this would all need to be made safe.
FYI, in this case, the current implementation doesn't even send the mouse event to the content process so it doesn't alter focus, which mostly works in this case but content is not DISABLEd.

* If content has focus, then I click more content, should chrome DISABLE focus before content restores it, or should chrome ignore it?
The current implementation DISABLEs, then reENABLEs.  The initial patch filters the chrome DISABLE in the widget (its a DISABLED->DISABLED state transition).  The recent patches behave as the current implementation.

* If chrome has focus, then I click more chrome, what should content do?  The current implementation doesn't forward the mouse event.  The current patch and, IIRC, the original patch all forward the event.  The content then generates a DISABLED state that is ignored by either the TabParent (new patch) or widget (old patch).

---

I got the impression from comment 49 that you were looking for something along the lines of a boolean in nsFrameLoader or IMEStateManager that tells if the process has focus, and then deciding to dispatch focus events (or not) based on this.  This can't be made to work well -- keeping this info synchronized would require additional IPC and a lot of asynchronicity.  Mainly, the remote content can't know if chrome has taken, or even stolen, focus without a special *asynchronous* (but probably still 'mostly' safe) message telling it so -- so in the patches it uses TabParent/widget to make that judgement.  I *could* put the boolean in nsFrameLoader and use it only on the chrome process, leaving TabParent to handle the chrome case and making the boolean invalid in the nsFrameLoader on the content proc, but this is needlessly confusing since the boolean isn't "only valid half the time" if I put it in the IMEState or the main proc widget instead, as the patches do.  Indeed, for the nsFrameLoader on the chrome process to keep the boolean synchronized, it would have to have the info pushed to it by the widget or TabParent anyway.  (That is not *technically* true since the event-processing order means that chrome should always have DISABLED itself before content can take focus, but if the measure is eliminating fragility, this accomplishes nothing.)  I would like it if windows DISABLEd themselves tho, as I anticipate (but haven't looked for) additional bugs, like with focus rings or password input.  (For that reason, I prefer the structure of the original patch.)

Thanks for looking this over.  Once we settle on a design, this should go quickly.

Comment 53

[Jan Moesen]

(In reply to David Parks [:handyman] from comment #2)
> First, the repro.

I cannot reproduce it using https://bugzilla.mozilla.org/, but I can reproduce it using https://www.strava.com/login in a clean profile.

Steps to reproduce:
* Set a master password (e.g. "test").
* Go to https://www.strava.com/login
* Enter a bogus e-mail address and password (e.g. "[e-mail]" and "test")
* Answer yes when asked if you want Nightly to remember
* Restart
* Go to https://www.strava.com/login
* Start typing in the master password entry dialog that pops up automatically

Example signatures:
https://crash-stats.mozilla.com/report/index/eaada82e-4231-421e-a1ba-76fb42141013
https://crash-stats.mozilla.com/report/index/b36fa619-a9ba-4edb-82b9-99d722141013
https://crash-stats.mozilla.com/report/index/7774ec85-fd3b-40d8-b4dd-f49262141013

Comment 55

[Benjamin Kerensa [:bkerensa]]

I too had this crash and also when it crashed somehow my master pass was auto pasted into an application I had just below which happened to be an IRC Client. I do not know how this is possible but it did happen and I did not have my master password in the system clipboard.

Comment 57

[Steven Michaud [:smichaud] (Retired)]

Bug 1096383 has STR.

Comment 58

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like this bug is stalled. What do we need to do to move this along Masayuki? It seems to be affecting a lot of users now that e10s is enabled on nightly.

Comment 59

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

Oops, I missed to catch the ni...

(In reply to David Parks [:handyman] from comment #52)
> * If chrome has focus, then I click on content, should chrome DISABLE focus
> before content ENABLEs it?
> The current implementation and all patches do this.

I don't think that chrome doesn't need to set the state as DISABLED because content will overwrite it. If both chrome and content will lose focus by inactivating the window, then, Gecko should keep the last enabled state even during deactive since on some platforms (including Mac) need to keep composition if there is.

> * If content has focus, then I click chrome, should content DISABLE focus
> before chrome sets it?

I don't think so because chrome should set new input context immediately.

> No implementation/patch DISABLEs first.  This would require running the
> entire mouse-handler asynchronously (since the remote call has to complete
> before the main process call, and that is not allowed to be synchronous).

Do you mean that keydown event could be fired between mouse click and set input context?

>   * The only alternative is for content not to DISABLE focus at all (ie
> doing it after chrome takes focus would be wrong).  Not disabling is wrong
> and may have repercussions (like incorrect focus rings).  I'm not sure tho
> what, if any, the effects are.  The patches technically disable focus after
> chrome, but only locally to the content proc (either the widget filters the
> state transition as DISABLED->DISABLED (old patch, done in widget since only
> widget knows its current true IME state) or the TabParent filters the call
> to the OS widget (newer patch)).  I *believe* this is sufficient.

If IME context is disabled temporarily during focus move and if user types keys this moment, IME will missed to catch key events. I'm not sure if this is possible scenario but if it's possible, we need to avoid this because similar bug provided very bad UX in our old version.

> Note that, since the content proc doesn't know that the main proc has focus, it
> can't filter this on its own (which is why TabParent does it). 
> Alternatively, chrome could send special messages to content when focus has
> been taken.  Content could then cache this information in its proc's
> nsFocusManager.  Since this has to be done asynchronously (like all chrome
> -> content communication), this would all need to be made safe.

Sounds good to me.

> FYI, in this case, the current implementation doesn't even send the mouse
> event to the content process so it doesn't alter focus, which mostly works
> in this case but content is not DISABLEd.
> 
> * If content has focus, then I click more content, should chrome DISABLE
> focus before content restores it, or should chrome ignore it?
> The current implementation DISABLEs, then reENABLEs.  The initial patch
> filters the chrome DISABLE in the widget (its a DISABLED->DISABLED state
> transition).  The recent patches behave as the current implementation.

I think that chrome should ignore it. I don't think that it should be filtered in widget level since most widget developers may not be specialists of e10s. So, it should be filtered in IMEStateManager or nsFocusManager. It can reduce the maintenance cost too. (Same logic is in each widget makes the maintenance cost increased)

> * If chrome has focus, then I click more chrome, what should content do? 
> The current implementation doesn't forward the mouse event.  The current
> patch and, IIRC, the original patch all forward the event.  The content then
> generates a DISABLED state that is ignored by either the TabParent (new
> patch) or widget (old patch).

I don't think content should do something in that case because focus is moving in chrome.

> I got the impression from comment 49 that you were looking for something
> along the lines of a boolean in nsFrameLoader or IMEStateManager that tells
> if the process has focus, and then deciding to dispatch focus events (or
> not) based on this.  This can't be made to work well -- keeping this info
> synchronized would require additional IPC and a lot of asynchronicity. 

Um, sounds bad...

Sorry for the delay to reply and I still don't understand perfectly about focus management across processes (and I forget a lot!).

My points are:

1. widget should be e10s free. I mean that callers of nsIWidget::SetInputContext() should check if it's necessary or not.
2. If we would need to disable IME context during focus move and if it could cause key event fired in this moment, we would provide very bad UX. So, we should avoid this scenario as far as possible if it's possible case.

Comment 60

[Benjamin Kerensa [:bkerensa]]

Do we have someone working on a patch for this?

Comment 61

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How's this going, David?

Comment 63

[Steven Michaud [:smichaud] (Retired)]

Bug 1109909 has STR.

Comment 64

[Jim Mathies [:jimm]]

davidp is on pto, he'll be back next Monday.

Comment 65

[David Parks [:handyman]]

Attachment: 1a. Fix concurrent FocusManager operation

Agnostic portion of the concurrency solution.

Comment 66

[David Parks [:handyman]]

Attachment: 1b. Use Cocoa concept of key-focus instead of main-focus

Test input-focus based on whether element would have focus if the window were main (instead of requiring window to be main).

Comment 67

[David Parks [:handyman]]

This is a tentative working solution, based on discussion with Masayuki.  Running the tests now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e3568af8b21

Comments on notes in comment 59:

>> * If content has focus, then I click more content, should chrome DISABLE
>> focus before content restores it, or should chrome ignore it?
>> The current implementation DISABLEs, then reENABLEs.  The initial patch
>> filters the chrome DISABLE in the widget (its a DISABLED->DISABLED state
>> transition).  The recent patches behave as the current implementation.
>
> I think that chrome should ignore it. I don't think that it
> should be filtered in widget level since
> most widget developers may not be specialists of e10s. So,
> it should be filtered in IMEStateManager or
> nsFocusManager. It can reduce the maintenance cost too.
> (Same logic is in each widget makes the
> maintenance cost increased)

I've added code to the IMEStateManager that handles this in the common case... but it cannot be made robust (without illegally blocking the main proc) for the concurrency reasons we discussed.  This isn't a big deal since it doesn't lose information... it just means a lot of useless IPC.  In the latest patch, this only happens if the remote process is setting focus *during* this operation.  If the remote proc has already told the main proc (via TabParent) that it's taken focus then the main proc does nothing.

Additionally, the widget (both nsChildView and nsIWidget) are free of this logic, as requested.  The nsChildView change (patch 1b) is needed simply because of OSX's "two concepts of focus".

Will update the bug as tests come in.

Comment 68

[David Parks [:handyman]]

Comment on attachment 8538127 [details] [diff] [review]
1a. Fix concurrent FocusManager operation

Test run look good.  Putting the new patches up for review.

Comment 69

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

Comment on attachment 8538127 [details] [diff] [review]
1a. Fix concurrent FocusManager operation

>@@ -1207,16 +1207,25 @@ EventStateManager::HandleCrossProcessEve
>       aEvent->message == NS_TOUCH_START) {
>     // If this event only has one target, and it's remote, add it to
>     // the array.
>     nsIFrame* frame = GetEventTarget();
>     nsIContent* target = frame ? frame->GetContent() : nullptr;
>     if (IsRemoteTarget(target)) {
>       targets.AppendElement(target);
>     }
>+    else {

nit: |} else {|

>+      // The event didn't target remote content.  We dispatch the
>+      // event to any remote content that does not have IMEState::Disabled
>+      // (the CrossProcessTarget).  This gives the remote content tree a
>+      // chance to dismiss focus if it has it.
>+      if(GetCrossProcessTarget()) {
>+        targets.AppendElement(GetCrossProcessTarget()->GetOwnerElement());
>+      }

I'm not sure what you do here. Please fine another reviewer for this.

>@@ -383,16 +384,30 @@ IMEStateManager::OnChangeFocusInternal(n
>   if (NS_WARN_IF(!widget)) {
>     PR_LOG(sISMLog, PR_LOG_ERROR,
>       ("ISM:   IMEStateManager::OnChangeFocusInternal(), FAILED due to "
>        "no widget to manage its IME state"));
>     return NS_OK;
>   }
> 
>   IMEState newState = GetNewIMEState(aPresContext, aContent);
>+
>+  // In e10s, remote content may have IME focus.  The main process (i.e. this process)
>+  // would attempt to set state to DISABLED if, for example, the user clicks
>+  // some other remote content.  The content process would later re-ENABLE IME, meaning
>+  // that all state-changes were unnecessary.
>+  // Here we filter the common case where the main process knows that the remote
>+  // process controls IME focus.  The DISABLED->re-ENABLED progression can
>+  // still happen since remote content may be concurrently communicating its claim
>+  // on focus to the main process... but this cannot cause bugs like missed keypresses.
>+  // (It just means a lot of needless IPC.)
>+  if ((newState.mEnabled == IMEState::DISABLED) && TabParent::GetIMETabParent()) {
>+    return NS_OK;
>+  }

I feel this is odd. TabParent::GetIMETabParent() returns a TabParent instance between NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR. They are fired from IMEContentObserver which is created when an editable element gets focus and destroyed when one loses focus. IMEContentObserver is caused by IMEStateManager::CreateIMEContentObserver() which is called when nsEditor receives DOM focus event. So, while focus is being moved from chrome to content, TabParent::GetIMETabParent() must return nullptr. Therefore, I don't understand what's checked by this... Is the DISABLED state is set by content process but chrome content tries to set focus DISABLED?

If so, widget->GetInputContext().mOrigin should be always InputContext::ORIGIN_REMOTE when the condition is true? If so, could you add MOZ_ASSERTION() for making clearer what the if condition means.

And I don't understand what occurs when child process is losing focus and chrome process tries to set IME state DISABLED? I guess that TabParent::GetIMETabParent() may be non-nullptr in this case...

> TabParent::RecvSetInputContext(const int32_t& aIMEEnabled,
>                                const int32_t& aIMEOpen,
>                                const nsString& aType,
>                                const nsString& aInputmode,
>                                const nsString& aActionHint,
>                                const int32_t& aCause,
>                                const int32_t& aFocusChange)
> {
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget || !AllowContentIME())
>+    return true;

Please use {} even if its block is only one line.

>+  InputContext oldContext = widget->GetInputContext();
>+
>+  // Check that the current InputContext originated remotely (ie from us).
>+  // It may have been overwritten by the main process.
>+  bool isCurrent = oldContext.HasRemoteOrigin();

isCurrent sounds unclear and this is used only once. So, using |oldContext.HasRemoteOrigin()| is clearer.

>+  // Ignore if current widget IME setting is not DISABLED and didn't come
>+  // from this Tab.

"this Tab" sounds odd. There may be a lot of "tabs" in the TabChild? I guess you meant "this instance" or just "TabParent"?

>+  if ((oldContext.mIMEState.mEnabled != IMEState::DISABLED) &&
>+      (!isCurrent)) {
>+    if (aIMEEnabled != IMEState::DISABLED) {
>+      NS_WARNING("Ignoring attempt to enable IME from multiple sources.");
>+    }

Could be NS_ASSERTION() or MOZ_ASSERT()?

However, isn't this a possible case? E.g., focus is moving from content process's <input> to chrome process's <input>? And also related with plugin whose content is in chrome process? If this is possible scenario, this NS_WARNING is just a spammer...

> #define NS_IWIDGET_IID \
>-{ 0x13239ca, 0xaf3f, 0x4f27, \
>-  { 0xaf, 0x83, 0x47, 0xa9, 0x82, 0x3d, 0x99, 0xee } };
>+{ 0x13239ca, 0xaf3f, 0x4f27, \
>+  { 0xaf, 0x83, 0x47, 0xa9, 0x82, 0x3d, 0x99, 0xee } };

Do we need to change the IID of nsIWidget? You don't change the vtable of nsIWidget.

> struct InputContext {
>-  InputContext() : mNativeIMEContext(nullptr) {}
>+  InputContext()
>+    : mNativeIMEContext(nullptr)
>+    , mOrigin(ORIGIN_MAIN)
>+ {}

Hmm, according to your patch, we can trust the mOrigin value only when after it's set to widget. For example, if it's instance is created in child process, but don't modified the mOrigin, it lies.  E.g., here is a liar: http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEStateManager.cpp#769

So, should it be mOrigin(XRE_IsParentProcess())? However, if checking it every time does waste the run-time cost, it should be cached in static variables in the constructor local, though.

>@@ -423,16 +426,31 @@ struct InputContext {
>+  /**
>+   * mOrigin indicates whether this focus event refers to main or remote content.
>+   */
>+  enum Origin {

Put the |{| to the next line.

>+    // Adjusting focus of content on the main process
>+    ORIGIN_MAIN,
>+    // Adjusting focus of content in a remote process
>+    ORIGIN_REMOTE

Isn't this "CONTENT" better? If it's referred from non-main process, "REMOTE" sounds strange to me.

>+  };
>+  Origin mOrigin;
>+
>+  bool HasRemoteOrigin() const {

Put the |{| to the next line.

And also, "has" sounds not good to me. Perhaps, |IsOriginContentProcess()|?

And also, then, |IsOriginMainProcess()| and |IsOriginCurrentProcess()| may be useful for others.

>-  InputContextAction() :
>-    mCause(CAUSE_UNKNOWN), mFocusChange(FOCUS_NOT_CHANGED)
>-  {
>-  }
>+  InputContextAction()
>+    : mCause(CAUSE_UNKNOWN)
>+    , mFocusChange(FOCUS_NOT_CHANGED)
>+  {}
> 
>   explicit InputContextAction(Cause aCause,
>-                              FocusChange aFocusChange = FOCUS_NOT_CHANGED) :
>-    mCause(aCause), mFocusChange(aFocusChange)
>-  {
>-  }
>+                              FocusChange aFocusChange = FOCUS_NOT_CHANGED)
>+    : mCause(aCause)
>+    , mFocusChange(aFocusChange)
>+  {}
> };

I don't think these changes are necessary for now. Please do it when you need to change here or file a code clean up bug.

But, almost looks good to me ;-)

Comment 70

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

Comment on attachment 8538128 [details] [diff] [review]
1b. Use Cocoa concept of key-focus instead of main-focus

I think that this is related to the focus behavior of MacOS X. So, Steven is better to review this.

Comment 71

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8538128 [details] [diff] [review]
1b. Use Cocoa concept of key-focus instead of main-focus

I think this patch makes a lot of sense.  But I don't think IsFocusedOnKeyFocus() does a good job of indicating what the method is about.  How about IsOrWouldBeFocused()?  And how about changing the comment as follows:

/**
 * True if our view has keyboard focus (and our window is key), or if
 * it would have keyboard focus if our window were key.
 */

Of course the devil is in the details.  For example, timing problems may cause the OS's and Gecko's notion of who has the focus to disagree, temporarily.  But let's cross that bridge when we come to it.

Comment 72

[David Parks [:handyman]]

Attachment: 1b. Use Cocoa concept of key-focus instead of main-focus (r+ed by smichaud)

smichaud's naming/comment changes

Comment 73

[David Parks [:handyman]]

Attachment: 1a. Fix concurrent FocusManager operation

Masayuki, the new patch includes all of your cosmetic changes and:


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #69)
> Comment on attachment 8538127 [details] [diff] [review]
> 1a. Fix concurrent FocusManager operation
> 
> >+      // The event didn't target remote content.  We dispatch the
> >+      // event to any remote content that does not have IMEState::Disabled
> >+      // (the CrossProcessTarget).  This gives the remote content tree a
> >+      // chance to dismiss focus if it has it.
> >+      if(GetCrossProcessTarget()) {
> >+        targets.AppendElement(GetCrossProcessTarget()->GetOwnerElement());
> >+      }
> 
> I'm not sure what you do here. Please fine another reviewer for this.

I've beefed up the EventStateManager stuff and separated it out for separate review.

> 
> >@@ -383,16 +384,30 @@ IMEStateManager::OnChangeFocusInternal(n
> >   if (NS_WARN_IF(!widget)) {
> >     PR_LOG(sISMLog, PR_LOG_ERROR,
> >       ("ISM:   IMEStateManager::OnChangeFocusInternal(), FAILED due to "
> >        "no widget to manage its IME state"));
> >     return NS_OK;
> >   }
> > 
> >   IMEState newState = GetNewIMEState(aPresContext, aContent);
> >+
> >+  // In e10s, remote content may have IME focus.  The main process (i.e. this process)
> >+  // would attempt to set state to DISABLED if, for example, the user clicks
> >+  // some other remote content.  The content process would later re-ENABLE IME, meaning
> >+  // that all state-changes were unnecessary.
> >+  // Here we filter the common case where the main process knows that the remote
> >+  // process controls IME focus.  The DISABLED->re-ENABLED progression can
> >+  // still happen since remote content may be concurrently communicating its claim
> >+  // on focus to the main process... but this cannot cause bugs like missed keypresses.
> >+  // (It just means a lot of needless IPC.)
> >+  if ((newState.mEnabled == IMEState::DISABLED) && TabParent::GetIMETabParent()) {
> >+    return NS_OK;
> >+  }
> 
> I feel this is odd. TabParent::GetIMETabParent() returns a TabParent
> instance between NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR. They are fired
> from IMEContentObserver which is created when an editable element gets focus
> and destroyed when one loses focus. IMEContentObserver is caused by
> IMEStateManager::CreateIMEContentObserver() which is called when nsEditor
> receives DOM focus event. So, while focus is being moved from chrome to
> content, TabParent::GetIMETabParent() must return nullptr. Therefore, I
> don't understand what's checked by this... Is the DISABLED state is set by
> content process but chrome content tries to set focus DISABLED?

TabParent::GetIMETabParent() actually also returns the value set when content has claimed IME focus... see TabParent::RecvSetInputContext().  When the tab's content no longer holds IME focus, TabParent::RecvSetInputContext() nulls it out.

> If so, widget->GetInputContext().mOrigin should be always
> InputContext::ORIGIN_REMOTE when the condition is true? If so, could you add
> MOZ_ASSERTION() for making clearer what the if condition means.
>
> And I don't understand what occurs when child process is losing focus and
> chrome process tries to set IME state DISABLED? I guess that
> TabParent::GetIMETabParent() may be non-nullptr in this case...

In that case, if both processes are DISABLEing but the content process has focus then only content will succeed.  This code was inserted to satisfy the request that the chrome FocusManager not make redundant SetInputContext calls, when it is not targeted, that would just be overruled when the content FocusManager issued its SetInputContext calls (asynchronously via TabParent) in response to handling an input event.

> So, should it be mOrigin(XRE_IsParentProcess())? However, if checking it
> every time does waste the run-time cost, it should be cached in static
> variables in the constructor local, though.

Actually, XRE_IsParentProcess() itself just fronts a static variable so I've changed it -- no big deal.

Comment 74

[David Parks [:handyman]]

Attachment: 1c. While content process holds focus, forward events that dont target it

Send input events to content until it relinquishes focus so that it knows when it has lost focus.

Comment 75

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

Comment on attachment 8539854 [details] [diff] [review]
1a. Fix concurrent FocusManager operation

Thanks, almost looks good to me.

>+  // In e10s, remote content may have IME focus.  The main process (i.e. this process)
>+  // would attempt to set state to DISABLED if, for example, the user clicks
>+  // some other remote content.  The content process would later re-ENABLE IME, meaning
>+  // that all state-changes were unnecessary.
>+  // Here we filter the common case where the main process knows that the remote
>+  // process controls IME focus.  The DISABLED->re-ENABLED progression can
>+  // still happen since remote content may be concurrently communicating its claim
>+  // on focus to the main process... but this cannot cause bugs like missed keypresses.
>+  // (It just means a lot of needless IPC.)
>+  if ((newState.mEnabled == IMEState::DISABLED) && TabParent::GetIMETabParent()) {

Okay, then, for making this condition clearer for other developers, please insert this:

MOZ_ASSERT(XRE_IsParentProcess(),
  "TabParent::GetIMETabParent() should never return non-null value "
  "in the content process");

I assume that this if condition can true only in parent process.  Then, other developers can understand this case is only for parent process's IMEStateManager from this assert without any running cost and this guarantees the expected behavior at least in debug build.

And also, please add logging code in this block:

PR_LOG(sISMLog, PR_LOG_DEBUG,
  ("ISM:   IMEStateManager::OnChangeFocusInternal(), "
   "Parent process cancels to set DISABLED state because the content process "
   "has IME focus and has already sets IME state"));

>+  // Ignore if current widget IME setting is not DISABLED and didn't come
>+  // from remote content.  Chrome content may have taken over.
>+  if ((oldContext.mIMEState.mEnabled != IMEState::DISABLED) &&
>+      (!oldContext.IsOriginContentProcess())) {
>+    return true;

Please remove redundant () and I think that it's better to use IsOriginParentProcess() in this case. I.e.,

if (oldContext.mIMEState.mEnabled != IMEState::DISABLED &&
    oldContext.IsOriginParentProcess()) {


The latter is just my favorite, so, if you don't like that, please keep using IsOriginContentProcess() with !.

Comment 76

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8539855 [details] [diff] [review]
1c. While content process holds focus, forward events that dont target it

Review of attachment 8539855 [details] [diff] [review]:
-----------------------------------------------------------------

Smaug, I think you'd be the best reviewer for this

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8539855 [details] [diff] [review]
1c. While content process holds focus, forward events that dont target it

This looks wrong to me.
We don't want mousemoves for example to be dispatched to child process even
if it has focus.
Don't you want to check aEvent->IsTargetedAtFocusedContent()?

And I think you should just make GetFrameLoader() in TabParent public
so that it would be easy to get frameLoader from GetCrossProcessTarget().

Comment 78

[David Parks [:handyman]]

Smaug,

The mousemoves (for example) that are dispatched by this are (only) the ones that have both of the following properties:

* The last time we (the main process) heard something relevant from the content process, the content process believed it had focus.  When it dismisses focus, it will use IPC to tell us.
* The event doesn't target remote content.  It causes that content to lose focus (if it has it).

Therefore, this code forwards those mousemoves to the content until the content's IPC message gets back to the main process.  At that point, it stops forwarding events.  So, we can expect very few events to be sent during that period, at least one but no more than are generated in a time frame of the scale of an IPC message (small fraction of a second).  Any messages after the first are superfluous (i.e. they dismiss focus that has already been dismissed) so they are all overhead.  So the fact that they are so exceedingly rare is important.

So, ignoring the extra messages, the question boils down to do we send zero or one message to the content to tell it to let go of focus.  With no messages, we don't dismiss focus, leading to this bug.  I had considered creating and sending a new, special IPC message to that effect instead of just forwarding the mouse event for this but in the end I couldn't find a reason to.

> Don't you want to check aEvent->IsTargetedAtFocusedContent()?

I'd mostly need to send the message regardless of the aEvent->IsTargetedAtFocusedContent property anyway.  I suppose, if it returns true, then we must be in one of the extraneous extra messages I mentioned (chrome has already taken focus, since we wouldn't be here if the event targeted content) and we could skip sending the message.  But that's not going to do much since the case is so rare and brief.

Making GetFrameLoader public is much cleaner.  I'll do that.

Let me know if there is something else you suggest for this.

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

mousemoves don't cause focus changes. And focus may be in the child process but mousemoves should be
dispatched to browser chrome if mouse is over browser chrome.

Comment 80

[David Parks [:handyman]]

Attachment: 1c. While content process holds focus, forward events that dont target it

Using smaug's suggestions including to filter irrelevant messages from forwarding to the content process (comment 77).

TBPL (1a-1c): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6219f4f5c1ce

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8545988 [details] [diff] [review]
1c. While content process holds focus, forward events that dont target it


>+      // The event didn't target remote content.  We dispatch the
>+      // event to the CrossProcessTarget (if one exists) if the
>+      // event is of a type that could cause the remote content to
>+      // lose focus if it has it (i.e. IsTargetedAtFocusedContent()
>+      // is true).  Once it (asynchronously) completes that,
>+      // CrossProcessTarget will be unset.  For our return
>+      // value, we do not count this as having handled the event.
>+      if(GetCrossProcessTarget() && aEvent->IsTargetedAtFocusedContent()) {
>+        nsRefPtr<nsFrameLoader> frameLoader = GetCrossProcessTarget()->GetFrameLoader();
>+        if (frameLoader) {
You could just do the following in the if(frameLoader)
Element* frameLoaderOwner = frameLoader->GetOwnerContent();
if (frameLoaderOwner) {
  targets.AppendElement(frameLoader->GetOwnerContent());
}

Comment 82

[David Parks [:handyman]]

I'd put the DispatchCrossProcessEvent call in the conditional because the function's return value is incorrect if I do it the other way.  The return value should only be true if we 'handled' the event, which we actually do not here.

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

Sure we handle the event.
Why would we want return value to be false?

Comment 84

[David Parks [:handyman]]

Attachment: 1c. While content process holds focus, forward events that dont target it (r+ed by smaug)

I guess it depends on the definition of 'handling' the event -- the current use of the return value is such that it doesn't really matter what we return in this case.  I'd read 'handling the event' as 'triggering the relevant event handler', in which case its the chrome that handles the event here (before this function is even called).  This patch includes the requested change.

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

Actually, last night I started to think... why do we need 1c at all.
If focus in the parent isn't in the <xul:browser remote>, key events shouldn't go there.
The question is, why focus isn't there.

Comment 86

[David Parks [:handyman]]

Comment on attachment 8546256 [details] [diff] [review]
1c. While content process holds focus, forward events that dont target it (r+ed by smaug)

(In reply to Olli Pettay [:smaug] from comment #85)
> Actually, last night I started to think... why do we need 1c at all.
> If focus in the parent isn't in the <xul:browser remote>, key events
> shouldn't go there.
> The question is, why focus isn't there.


(There is no question here… the NI is just so that you know whats been done.)

…you’re right.  This patch is (no longer) needed — the circumstances of the bug have changed considerably.  In fact, the STR in comment 2 isn’t even valid anymore.  An updated STR would be:

Steps:
* Open an e10s window
* Go to a page with a password text field
* Select the password text field and type characters
* Command-Tab or click away from the browser, then click on the web search bar (or any chrome text field).  Do not interact with the dock or anything like that as part of this step.
* Type something

Expected Behavior: 
* The search bar shows the typed characters.

Actual Behavior:
* Crash!

The previous STR would focus a different text field, causing (as I recall) the password-setting inconsistency.  Command-Tabbing back to the window now properly refocuses the same (password) element, fixing that crash.  IIRC this was a component of the original problem and I believe the EventStateManager code was done because I couldn't justify that things were so fragile with no real reason.  But since they work now, its extraneous.  I’m removing the patch from the series.

Comment 87

[David Parks [:handyman]]

Attachment: 1. Use Cocoa concept of key-focus instead of main-focus (r+ed by smichaud)

De-bitrot.

Comment 88

[David Parks [:handyman]]

Attachment: 2. Fix concurrent FocusManager operation (r+ed by masayuki)

Includes changes from masayuki's review in comment 75.

Comment 89

[David Parks [:handyman]]

Comment on attachment 8547769 [details] [diff] [review]
2. Fix concurrent FocusManager operation (r+ed by masayuki)

Mentioning r+ (comment 75)

Comment 90

[David Parks [:handyman]]

Attachment: 3. Normalize secure input requests (r+ed by masayuki)

r+ from (between comment 14 and 15).

NI isn't part of a question.  I just wanted to let you know that I've dumped attachment 8476481 [details] [diff] [review] (Make the PBrowser::SetInputContext message synchronous).  After getting deep into this, its clearer how this operates without SetInputContext being synchronous.  (It wasn’t actually used for this bug — we had just thought that it ‘looked right’ early on — see comment 1 and comment 2.)  Let me know if that's a problem.

Comment 91

[David Parks [:handyman]]

TBPL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54b1889f65f7

Comment 92

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

(In reply to David Parks [:handyman] from comment #90)
> Created attachment 8547771 [details] [diff] [review]
> 3. Normalize secure input requests (r+ed by masayuki)
> 
> r+ from (between comment 14 and 15).
> 
> NI isn't part of a question.  I just wanted to let you know that I've dumped
> attachment 8476481 [details] [diff] [review] (Make the
> PBrowser::SetInputContext message synchronous).  After getting deep into
> this, its clearer how this operates without SetInputContext being
> synchronous.  (It wasn’t actually used for this bug — we had just thought
> that it ‘looked right’ early on — see comment 1 and comment 2.)  Let me know
> if that's a problem.

Hmm, the race may cause other bugs too. I think that we should make it synchronous. Do you think it causes some other issues, like performance regression? If not, it should be synchronous since NotifyIMEFocus() is synchronous.

Comment 93

[David Parks [:handyman]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92)
> 
> Hmm, the race may cause other bugs too. I think that we should make it
> synchronous. Do you think it causes some other issues, like performance
> regression? If not, it should be synchronous since NotifyIMEFocus() is
> synchronous.

I don't forsee any failures or deadlocks from making this synchronous (they should still be impossible) so I'll resurrect the patch.  The only issue this could raise is performance.  I don't think that NotifyIMEFocus being synchronous can affect the behavior -- IPDL will still guarantee that all messages with the same source/destination processes are processed in queue order -- so if content sets the input context and later calls NotifyIMEFocus, the main process will receive them in that order (sync messages drain those in front of them in the queue).

I'll repost the patch and then check this stuff in.  Thanks for the help.

Comment 94

[David Parks [:handyman]]

Attachment: 4. Make the PBrowser::SetInputContext message synchronous (r+ed by masayuki)

r+ed in comment 15

Comment 95

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

(In reply to David Parks [:handyman] from comment #93)
> The only issue this could raise is performance.

Indeed. However, it shouldn't be so serious because it's only occurs when moving focus.

> I don't think that NotifyIMEFocus being
> synchronous can affect the behavior -- IPDL will still guarantee that all
> messages with the same source/destination processes are processed in queue
> order -- so if content sets the input context and later calls
> NotifyIMEFocus, the main process will receive them in that order (sync
> messages drain those in front of them in the queue).

How about this case? When focus is moving in content process, users may input first character for new focused content during processing focus move. Then, the input is handled by parent (chrome) process. Then, it could be handled during NotifyIMEFocus() and SetInputContext(). We should make this gap is zero as far as possible.

> I'll repost the patch and then check this stuff in.  Thanks for the help.

Thank you very much! Awesome!

Comment 96

[David Parks [:handyman]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #95)
> (In reply to David Parks [:handyman] from comment #93)

> How about this case? When focus is moving in content process, users may
> input first character for new focused content during processing focus move.
> Then, the input is handled by parent (chrome) process. Then, it could be
> handled during NotifyIMEFocus() and SetInputContext(). We should make this
> gap is zero as far as possible.

In case the case where focus is moving to content (from chrome or from other content), the chrome "focus manager" will resign focus (event #1), assigning it to a <browser remote="true"> node.  The subsequent 'first' character (event #2) will be directed to the remote process -- the asynchronicity of the IME setting doesn't come into play because the focus manager's setting determines that the first character is sent to the remote process and IPDL message order will guarantee that the handling of event #2 happens after the handling of event #1 and, to process event #2, the remote process will need to get in line after event #1 in the "content -> chrome" IPDL queue (event #1 for the SetInputContext call, event #2 for OSX widget IME handling, which only exist in chrome proc).

Comment 97

[David Parks [:handyman]]

TBPL tests with all of the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b6421c0785

Comment 98

[Carsten Book [:Tomcat]]

David, so only part 4 need checkin ?

Comment 99

[David Parks [:handyman]]

(In reply to Carsten Book [:Tomcat] from comment #98)
> David, so only part 4 need checkin ?

Actually, all four parts still need checkin, in the order they are numbered.  The test run does in fact include all four patches (the revision list display is hiding them but they are visible as parent patches there).

Comment 100

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7310c42d346e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1dd58a149d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/90ca7355bbcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/734517b508b5

Comment 101

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7310c42d346e
https://hg.mozilla.org/mozilla-central/rev/e1dd58a149d2
https://hg.mozilla.org/mozilla-central/rev/90ca7355bbcc
https://hg.mozilla.org/mozilla-central/rev/734517b508b5

Comment 102

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Can anybody here think of why these patches might be causing window leaks? Bug 1122208 really exploded over the weekend. We'll need to fix these leaks somehow.

Comment 103

[Julian Viereck]

Using the Firefox Nightly from 20-Jan-2015 I still get crashes: https://crash-stats.mozilla.com/report/index/471bbb0d-9b41-483e-b02d-af4fa2150120

Steps to reproduce:
1. Restart Firefox
2. Go to a page that has a stored password
3. As I have locked my saved passwords via a master password, Firefox prompts me to enter the password
4. In roughly 50% of the cases, after entering the first few characters of my password, the browser crashes

This is on OSX 10.10.1.

@David, I am not sure what's the best way to track these crashes. Should I open a new bug or should this bug be reopened?

Comment 104

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #103)

This might be bug 893973.  For at least a while we thought it only happened in non-e10s mode.  But it might also happen in e10s mode, and simply have been drowned out by this bug (which was much more common).

Comment 105

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Unfortunately had to back this out because of bug 1122208.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21d4b5f35e8f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5b4551a035
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d61b3b84f8a3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/472dff21ee37

Comment 106

[Ryan VanderMeulen [:RyanVM]]

Merge of backouts:
https://hg.mozilla.org/mozilla-central/rev/21d4b5f35e8f
https://hg.mozilla.org/mozilla-central/rev/cf5b4551a035
https://hg.mozilla.org/mozilla-central/rev/d61b3b84f8a3
https://hg.mozilla.org/mozilla-central/rev/472dff21ee37

Comment 107

[David Parks [:handyman]]

(In reply to Julian Viereck from comment #103)
> Steps to reproduce:
> 1. Restart Firefox
> 2. Go to a page that has a stored password
> 3. As I have locked my saved passwords via a master password, Firefox
> prompts me to enter the password
> 4. In roughly 50% of the cases, after entering the first few characters of
> my password, the browser crashes
> 

I am able to reproduce this, intermittently, as well.  RE: Steven's comment 104, I see it about half the time, same as the bug reporter... but I've tried half a dozen times with non-e10s and not been able to reproduce.

> @David, I am not sure what's the best way to track these crashes. Should I
> open a new bug or should this bug be reopened?

I'm speculating that the mechanism used by the chrome popup widgets are not properly integrated into the IME procedure.  This feels like a separate bug but I want to keep investigating for a bit.  If it needs a new bug, I'll take care of it.

(Bill's fubar tests are another issue...)

Comment 108

[David Parks [:handyman]]

Masayuki, we've run into a problem with intermittent Linux ASAN test failures, due to a memory leak from these patches - the direct cause of which is unknown.  However, the issue seems to go away if we dump the fourth patch -- the one that makes SetInputContext async.  Since this patch isn't needed to fix this crash, we'd like to revert it (and not the other patches).  Are you ok with that?

Tests with the SetInputContext patch undone:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b31cd56d58
(Currently shows 8 successful runs -- I've triggered more -- apparently it failed about 50% of the time without the change).

I'm going to create a new bug for Julian's issue, which I still have not confirmed is unrelated.

NIing billm just so that he knows to re-land the patches if the plan is approved.

Comment 109

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

(In reply to David Parks [:handyman] from comment #108)
> Masayuki, we've run into a problem with intermittent Linux ASAN test
> failures, due to a memory leak from these patches - the direct cause of
> which is unknown.  However, the issue seems to go away if we dump the fourth
> patch -- the one that makes SetInputContext async.  Since this patch isn't
> needed to fix this crash, we'd like to revert it (and not the other
> patches).  Are you ok with that?

Yeah, it's okay. But I hope you file a new bug for it.

Comment 110

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3607c4edca31
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/520e69d246c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d15d3013dc3b

Comment 111

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

Hey! Looks like that your backout of this makes nsIWidget.h's line breaks CRLF!!!
http://hg.mozilla.org/mozilla-central/rev/d61b3b84f8a3

Comment 112

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm very sorry about that. I just used hg backout on a Linux machine. I have no idea how it happened. Anyway, I checked in a fix that removes all the CRs.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f857dd4147a7

Comment 113

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

(In reply to Bill McCloskey (:billm) from comment #112)
> I'm very sorry about that. I just used hg backout on a Linux machine. I have
> no idea how it happened.

Wow, really? IIRC, there was some lines whose line breaks are CRLF. I guess that hg was confused by the mixed line breaks.

> Anyway, I checked in a fix that removes all the CRs.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f857dd4147a7

Thanks!

Comment 115

[Schalk Neethling [:espressive]]

I got this today when attempting to enter the password in the Lastpass addon: see https://bugzilla.mozilla.org/show_bug.cgi?id=1124555

Comment 116

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3607c4edca31
https://hg.mozilla.org/mozilla-central/rev/520e69d246c7
https://hg.mozilla.org/mozilla-central/rev/d15d3013dc3b
https://hg.mozilla.org/mozilla-central/rev/f857dd4147a7