Closed Bug 1549930 Opened 5 years ago Closed 5 years ago

cannot input with ATOK Passport's moji pallet

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 + verified
firefox69 --- verified

People

(Reporter: ayakawa.m, Assigned: hsivonen)

References

(Regression)

Details

(Keywords: inputmethod, regression)

Attachments

(3 files, 1 obsolete file)

Bug 1549930 - Avoid popping BrowserParent in response to window lowering.
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
Bug 1549930 addendum - Notify widgets of IME blur on quit (on non-Android).
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
Bug 1549930 test adjustment - Disable browser_privatebrowsing_about.js due to perma-orange.
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. use IME:ATOK Passport
  2. set current cursor to Editable item(URL bar, Text area)
  3. enable ATOK and open ATOK 文字パレット(moji pallet)
  4. select some charactor and push 確定

Actual results:

no charactor input.

Expected results:

input selected charactor.

I couldn't reproduce the problem on the location bar of the Firefox itself. However, I've successfully reproduced it on a contenteditable=true Web contents, for example: https://piro.sakura.ne.jp/apps/emoji-editor.html

My environment is:

  • UA: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0
  • Build ID: 20190507214514
  • OS: Windows 10 1809
  • ATOK Passport ver.31.1.6

I retried this bug, URL bar accept to input with moji pallet.
thank you piro for your infomation.

regression range
2019/03/18 - 2019/03/19
build aa59ec8e : this bug is happen. But, after input char with keyboard, can input with moji pallet.
build 2244c803 : never input.

Component: Untriaged → Widget: Win32
Keywords: inputmethod
Product: Firefox → Core

regression by "Bug 1524242 - Capture TabParent of out-of-process iframe when creating TextComposition. r=masayuki" according to mozgression.

Keywords: regression
Regressed by: 1524242

moving to right component since this is regression by bug 1524242

Component: Widget: Win32 → User events and focus handling

[Tracking Requested - why for this release]: This is new regression of 68, and breaks features of IME (and perhaps, some a11y tools).

When user clicks in Moji-pallet of ATOK, Firefox has lost focus. In such case, does BrowserParent::GetFocused() return nullptr?
https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/layout/base/PresShell.cpp#8267

If so, TextComposition is created for the main process even when active element has TabParent in it. So, perhaps, there should be BrowserParent::GetActive() which returns TabParent for active element (i.e., active element in the window is not associated with TabParent, should return nullptr. I.e., not simply returns /last/ focused TabParent).

Status: UNCONFIRMED → NEW
status-firefox67: --- → unaffected
status-firefox68: --- → affected
tracking-firefox68: --- → ?
Ever confirmed: true
Flags: needinfo?(hsivonen)
OS: Unspecified → Windows
Priority: -- → P1
Hardware: Unspecified → All

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response)(away 5/10~5/19) from comment #6)

[Tracking Requested - why for this release]: This is new regression of 68, and breaks features of IME (and perhaps, some a11y tools).

When user clicks in Moji-pallet of ATOK, Firefox has lost focus. In such case, does BrowserParent::GetFocused() return nullptr?

I believe so.

If so, TextComposition is created for the main process even when active element has TabParent in it. So, perhaps, there should be BrowserParent::GetActive() which returns TabParent for active element (i.e., active element in the window is not associated with TabParent, should return nullptr. I.e., not simply returns /last/ focused TabParent).

How should we differentiate between "Before Firefox as a whole lost focus, the focus was in Web content" and "Before Firefox as a whole lost focus, the focus was in chrome"? Or is there some other check elsewhere which would make returning the last-focused Web content OK here even if logically focus was in chrome before Firefox as a whole lost focus?

Can we make Widget trigger caching of the focused BrowserParent (including nullptr if chrome is focused) when Firefox as a whole is about to lose focus? What would be the right place in the Widget code to do stuff before letting Firefox as a whole lose focus before actually triggeting the focus deactivation?

Flags: needinfo?(hsivonen) → needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #7)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response)(away 5/10~5/19) from comment #6)

[Tracking Requested - why for this release]: This is new regression of 68, and breaks features of IME (and perhaps, some a11y tools).

When user clicks in Moji-pallet of ATOK, Firefox has lost focus. In such case, does BrowserParent::GetFocused() return nullptr?

I believe so.

Okay, that means it's the cause.

If so, TextComposition is created for the main process even when active element has TabParent in it. So, perhaps, there should be BrowserParent::GetActive() which returns TabParent for active element (i.e., active element in the window is not associated with TabParent, should return nullptr. I.e., not simply returns /last/ focused TabParent).

How should we differentiate between "Before Firefox as a whole lost focus, the focus was in Web content" and "Before Firefox as a whole lost focus, the focus was in chrome"?

In the latter case, and <input> in chrome widget, 3rd party apps can input text into it. This is what the behavior mentioned in comment 2.

Or is there some other check elsewhere which would make returning the last-focused Web content OK here even if logically focus was in chrome before Firefox as a whole lost focus?

No, we shouldn't need such checks. We should handle input events even on inactive windows with active element in active document in it.

Can we make Widget trigger caching of the focused BrowserParent (including nullptr if chrome is focused) when Firefox as a whole is about to lose focus? What would be the right place in the Widget code to do stuff before letting Firefox as a whole lose focus before actually triggeting the focus deactivation?

Currently, BrowserParent::PopFocus() makes itself forget focus information when BrowserParent::Deactivate() is called. From a point of view of user input event handling, this must be wrong. Any 3rd party applications (including IME and a11y tools) can synthesize input events even on inactive window (i.e., on inactive nsIWidget instance). So, if we can keep sFocusStack even during inactive, this must be able to fix easy. Can we do that? Or we need to move the information into another one?

Note that if web content moves focus with Element.focus() on inactive window, the target should be changed to new active element. So, I think that BrowserParent needs to keep managing focused TabParent even while it's inactive.

Flags: needinfo?(masayuki) → needinfo?(hsivonen)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response)(away 5/10~5/19) from comment #8)

No, we shouldn't need such checks. We should handle input events even on inactive windows with active element in active document in it.

This surprises me, considering that a window becoming inactive means that IME composition gets committed. If inactive windows are expected not to have normal IME compositions, how does TextComposition get created in this case?

Can we make Widget trigger caching of the focused BrowserParent (including nullptr if chrome is focused) when Firefox as a whole is about to lose focus? What would be the right place in the Widget code to do stuff before letting Firefox as a whole lose focus before actually triggeting the focus deactivation?

Currently, BrowserParent::PopFocus() makes itself forget focus information when BrowserParent::Deactivate() is called. From a point of view of user input event handling, this must be wrong. Any 3rd party applications (including IME and a11y tools) can synthesize input events even on inactive window (i.e., on inactive nsIWidget instance). So, if we can keep sFocusStack even during inactive, this must be able to fix easy. Can we do that?

The code I added for Fission IME handling is based on the assumption that inactive windows cannot receive keyboard events or perform IME compositions.

Activate/Deactive calls don't carry information on whether focus is moving or the whole native window is going to the background, so we can't simply keep the stack.

Or we need to move the information into another one?

We could take a snapshot of the top of the focus stack before the stack changes if we act from a widget signal before actually performing the Deactivate calls, but...

Note that if web content moves focus with Element.focus() on inactive window, the target should be changed to new active element. So, I think that BrowserParent needs to keep managing focused TabParent even while it's inactive.

...just taking a snapshot wouldn't handle the case of Web content moving focus in an inactive window. I don't have any obvious ideas of how the case of focus moving in an inactive window between processes could be handled. Do you have a suggestion? How bad would it be if we didn't handle that case?

Flags: needinfo?(hsivonen) → needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #9)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response)(away 5/10~5/19) from comment #8)

No, we shouldn't need such checks. We should handle input events even on inactive windows with active element in active document in it.

This surprises me, considering that a window becoming inactive means that IME composition gets committed.

IIRC, on Linux, IME itself does it, but not so on Windows. On Windows, if parent process becomes busy, it temporarily loses focus, but we shouldn't commit composition in the moment.

If inactive windows are expected not to have normal IME compositions, how does TextComposition get created in this case?

No, we've not expected that since some software keyboard may steal focus from active application like this case. So, we need to handle it with active element in active document in active TabParent in the window which receives input events during inactive.

Can we make Widget trigger caching of the focused BrowserParent (including nullptr if chrome is focused) when Firefox as a whole is about to lose focus? What would be the right place in the Widget code to do stuff before letting Firefox as a whole lose focus before actually triggeting the focus deactivation?

Currently, BrowserParent::PopFocus() makes itself forget focus information when BrowserParent::Deactivate() is called. From a point of view of user input event handling, this must be wrong. Any 3rd party applications (including IME and a11y tools) can synthesize input events even on inactive window (i.e., on inactive nsIWidget instance). So, if we can keep sFocusStack even during inactive, this must be able to fix easy. Can we do that?

The code I added for Fission IME handling is based on the assumption that inactive windows cannot receive keyboard events or perform IME compositions.

Activate/Deactive calls don't carry information on whether focus is moving or the whole native window is going to the background, so we can't simply keep the stack.

I think that we don't need to check the difference. For example, on Windows, other applications can send text input messages to inactive window even when another window of the process is active (although not realistic scenario). So, we should keep managing focus in each window.

Or we need to move the information into another one?

We could take a snapshot of the top of the focus stack before the stack changes if we act from a widget signal before actually performing the Deactivate calls, but...

Note that if web content moves focus with Element.focus() on inactive window, the target should be changed to new active element. So, I think that BrowserParent needs to keep managing focused TabParent even while it's inactive.

...just taking a snapshot wouldn't handle the case of Web content moving focus in an inactive window. I don't have any obvious ideas of how the case of focus moving in an inactive window between processes could be handled. Do you have a suggestion? How bad would it be if we didn't handle that case?

For short-term goal to fix this in realistic scenarios, snap-shot at inactivating may fix this (unless web apps changes focus immediately before that).

Unfortunately, I've not yet understood the focus handling in Fission world. So, I have no idea which can help you for now...

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response)(away 5/10~5/19) from comment #10)

For short-term goal to fix this in realistic scenarios, snap-shot at inactivating may fix this (unless web apps changes focus immediately before that).

I'll pursue this approach first, because I don't have ideas of how to fix this properly.

This seems to be the closest to platform-specific code for handling window moving to background:
https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/xpfe/appshell/nsWebShellWindow.cpp#808

Is the regressing patch something we can back out of 68, to give us more time?

status-firefox66: --- → unaffected
status-firefox-esr60: --- → unaffected
tracking-firefox68: ? → +

(In reply to Julien Cristau [:jcristau] from comment #14)

Is the regressing patch something we can back out of 68, to give us more time?

The patch won't back out cleanly. There has been quite a bit of Fission work with interdependencies and intervening renames. In any case it would require manually undoing the changes instead of just backing out, and it's likely that the amount of code affected in such reversal would be rather large.

I think there's a good chance that the snapshot approach discussed in earlier comments will fix the user impact for practical purposes despite being obviously inadequate to handle scenarios where page-provided JS moves focus between content processes in a background window. Since Fission isn't yet enabled to the point that the user base in general would be running with Fission, the edge cases should not be relevant at this time (unless page JS manages to hand focus from Web content to chrome in a background window).

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Does the build available from https://queue.taskcluster.net/v1/task/TZ80qIZHQGu4ASlghuHu_w/runs/0/artifacts/public/build/target.zip fix the problem?

Flags: needinfo?(yuki)
Flags: needinfo?(ayakawa.m)

I tested it, not fix.

Flags: needinfo?(ayakawa.m)

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #19)

I tested it, not fix.

Thank you.

I'll re-verify that my patch works the way I expected it to, but other than that, I don't now have ideas of what might be wrong. :-(

Flags: needinfo?(yuki)

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #3)

build aa59ec8e : this bug is happen

Ooh, part of this uses pasting and not a composition.

not fix.

Flags: needinfo?(ayakawa.m)

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #24)

not fix.

Thank you.

I think it's not enough to be able to deliver composition events, but it's probably also necessary to be able to respond to content queries properly. My current guess is that with the current patch the content cache in parent goes into a state that's inconsistent with the ability to deliver composition events. I fail to see what might flush the content cache, though.

Masayuki, does that guess seem right? Considering the current patch, what should I change to keep the content cache responsive when Firefox is in the background?

Flags: needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #26)

I think it's not enough to be able to deliver composition events,

Yes. Query content events and keypress events should also be delivered to the active document/element.

but it's probably also necessary to be able to respond to content queries properly.

Exactly.

My current guess is that with the current patch the content cache in parent goes into a state that's inconsistent with the ability to deliver composition events. I fail to see what might flush the content cache, though.

Masayuki, does that guess seem right? Considering the current patch, what should I change to keep the content cache responsive when Firefox is in the background?

I guess so. I'll check what's going with debugger. (And do you have a bug# which implements new TabParent management? I'd like to check how the main process knows new TabParent when it gets focus again.)

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #27)

(In reply to Henri Sivonen (:hsivonen) from comment #26)

I think it's not enough to be able to deliver composition events,

Yes. Query content events and keypress events should also be delivered to the active document/element.

OK. Key events aren't handled by the patch, but my attempt to deliver query content events doesn't work, because for whatever reason, the event dispatch doesn't get as far as the point where the event is actually routed.

but it's probably also necessary to be able to respond to content queries properly.

Exactly.

My current guess is that with the current patch the content cache in parent goes into a state that's inconsistent with the ability to deliver composition events. I fail to see what might flush the content cache, though.

Masayuki, does that guess seem right? Considering the current patch, what should I change to keep the content cache responsive when Firefox is in the background?

I guess so. I'll check what's going with debugger.

Thank you.

(And do you have a bug# which implements new TabParent management? I'd like to check how the main process knows new TabParent when it gets focus again.)

Bug 1535537. nsFocusManager knows to initiate the right Activate chain when a window is raised.

Ah, when active window becomes inactive, in the main process, nsFocusManager::WindowLowered() calls nsFocusManager::Blur(), then, it calls BrowserParent::Deactive(), then, it calls BrowserParent::PopFocus(), then, [it calls IMEStateManager::OnFocusMovedBetweenBrowsers()],(https://searchfox.org/mozilla-central/rev/952521e6164ddffa3f34bc8cfa5a81afc5b859c4/dom/ipc/BrowserParent.cpp#2699) then, IMEStateManager::NotifyIME() is called with NOTIFY_IME_OF_BLUR and finally, TSFTextStore is destroyed. So, we've already notified Windows of stopping handling composition. Therefore, moji pallet does nothing even when user tries to input some characters.

I have a question here, why do we need to forget focused BrowserParents when it goes inactive? Instead, cannot we make nsFocusManager forget all focused BrowerParents when another window becomes active?

Flags: needinfo?(hsivonen)

Fortunately, in content process, StopIMEStateManagement() is called from IMEStateManager::OnFocusMovedBetweenBrowsers() in the main process. Therefore, there is no conflict between main process IME state and content process IME state.

However, this fact means that, 68 has another regression which is same as bug 835262.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #29)

I have a question here, why do we need to forget focused BrowserParents when it goes inactive? Instead, cannot we make nsFocusManager forget all focused BrowerParents when another window becomes active?

When focus moves out of an out-of-process iframe, BrowserParent::Deactivate and the stack is popped.

AFAICT, we don't have a way to distinguish between "This out-of-process iframe is getting Deactivate because Firefox as a whole is deactivating" and "This out-of-process iframe is getting Deactivate because focus is moving to its Web-hierarchy parent".

I guess one thing that would not work for Fission but would make the Fission code not break non-Fission for now would be not popping top-level BrowserParent until another top-level BrowserParent is pushed.

Flags: needinfo?(hsivonen)

The most important cause of this bug is, Fission code tries to manage "focused" element, but we need to manage "active" element (i.e., the result of document.activeElement). So, I think that Fission code should keep focused BrowserParents even after entirely inactivated. Additionally, it should keep managing BrowserParent array when active element is modified during inactive. Anyway, such changes cannot be fixed in 68 though.

What is the reason why Fission code manages "focus" elements rather than "active" elements?

Flags: needinfo?(hsivonen)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #32)

What is the reason why Fission code manages "focus" elements rather than "active" elements?

Because I wasn't aware of the finer points of the distinction, and Activate/Deactivate were the pre-existing cross-process hooks.

Flags: needinfo?(hsivonen)
Attachment #9064401 - Attachment description: Bug 1549930 - Make deactivated windows remember their last-focused BrowserParent for text event targeting. → Bug 1549930 - Avoid popping BrowserParent in response to window lowering.

The latest patch avoids popping the stack in response to window lowering. MOZ_LOG on Linux indicates it works like it is supposed to.

Does the build at https://queue.taskcluster.net/v1/task/IajrwCQ9QcORCQjxkM5xwQ/runs/0/artifacts/public/build/target.zip fix the problem for you?

Flags: needinfo?(ayakawa.m)

Good. I can input with Moji Panel.
But, need to input some char with keyboard before we input with Moji Panel. ( same as comment 3 good case )

Flags: needinfo?(ayakawa.m)

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #36)

Good. I can input with Moji Panel.

Thank you.

But, need to input some char with keyboard before we input with Moji Panel. ( same as comment 3 good case )

Masayuki, do you have insight into what mechanism causes prior keyboard input to make a difference?

Flags: needinfo?(masayuki)

(In reply to Henri Sivonen (:hsivonen) from comment #37)

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #36)

But, need to input some char with keyboard before we input with Moji Panel. ( same as comment 3 good case )

Masayuki, do you have insight into what mechanism causes prior keyboard input to make a difference?

Sorry for the delay. I've not completely investigated it yet, the root cause is, IMEContentObserver::KeepAliveDuringDeactive() returns false only until user type a character in editable content on the puppet widget. It seems that this is a regression of bug 1349255. So, please go ahead with ignoring this symptom (out of scope of this bug).

Flags: needinfo?(masayuki)

Thanks.

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79436da3bf79
Avoid popping BrowserParent in response to window lowering. r=masayuki
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07b973eb1f19
Backed out changeset 79436da3bf79 for causing perma browser chrome failures in browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js CLOSED TREE

rrosario, browser_privatebrowsing_about.js seems to first fail with "url bar has hidden focused" for me locally. However, that bit didn't fail in CI: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=79436da3bf79&selectedJob=248547861

Any idea why that test might fail with ASAN, which generally makes things slower?

Flags: needinfo?(rrosario)

(In reply to Henri Sivonen (:hsivonen) from comment #45)

Any idea why that test might fail with ASAN, which generally makes things slower?

I guess we should be waiting for the condition instead of checking it synchronously. Want to try this change below? Or file a bug and I'll take care of it. Thanks

diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js
index 14fd6c7bd2635..b276473dbe051 100644
--- a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js
+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js
@@ -105,14 +105,16 @@ add_task(async function test_myths_link() {
   await BrowserTestUtils.closeWindow(win);
 });

-function urlBarHasHiddenFocus(win) {
-  return win.gURLBar.hasAttribute("focused") &&
-    win.gURLBar.textbox.classList.contains("hidden-focus");
+async function urlBarHasHiddenFocus(win) {
+  await BrowserTestUtils.waitForCondition(
+    () => win.gURLBar.hasAttribute("focused") && win.gURLBar.textbox.classList.contains("hidden-focus"),
+    "URL bar has hidden focus");
 }

-function urlBarHasNormalFocus(win) {
-  return win.gURLBar.hasAttribute("focused") &&
-    !win.gURLBar.textbox.classList.contains("hidden-focus");
+async function urlBarHasNormalFocus(win) {
+  await BrowserTestUtils.waitForCondition(
+    () => win.gURLBar.hasAttribute("focused") && !win.gURLBar.textbox.classList.contains("hidden-focus"),
+    "URL bar has normal focus");
 }

 /**
@@ -127,13 +129,13 @@ add_task(async function test_search_handoff_on_keydown() {
     btn.click();
     ok(btn.classList.contains("focused"), "in-content search has focus styles");
   });
-  ok(urlBarHasHiddenFocus(win), "url bar has hidden focused");
+  await urlBarHasHiddenFocus(win);
   await new Promise(r => EventUtils.synthesizeKey("f", {}, win, r));
   await ContentTask.spawn(tab, null, async function() {
     ok(content.document.getElementById("search-handoff-button").classList.contains("hidden"),
       "in-content search is hidden");
   });
-  ok(urlBarHasNormalFocus(win), "url bar has normal focused");
+  await urlBarHasNormalFocus(win);
   is(win.gURLBar.value, "@google f", "url bar has search text");
   await UrlbarTestUtils.promiseSearchComplete(win);
   // Close the popup.
@@ -159,9 +161,10 @@ add_task(async function test_search_handoff_on_composition_start() {
   await ContentTask.spawn(tab, null, async function() {
     content.document.getElementById("search-handoff-button").click();
   });
-  ok(urlBarHasHiddenFocus(win), "url bar has hidden focused");
+  await urlBarHasHiddenFocus(win);
   await new Promise(r => EventUtils.synthesizeComposition({type: "compositionstart"}, win, r));
-  ok(urlBarHasNormalFocus(win), "url bar has normal focused");
+  await urlBarHasNormalFocus(win);
+

   await BrowserTestUtils.closeWindow(win);
 });
@@ -176,7 +179,7 @@ add_task(async function test_search_handoff_on_paste() {
   await ContentTask.spawn(tab, null, async function() {
     content.document.getElementById("search-handoff-button").click();
   });
-  ok(urlBarHasHiddenFocus(win), "url bar has hidden focused");
+  await urlBarHasHiddenFocus(win);
   var helper = SpecialPowers.Cc["@mozilla.org/widget/clipboardhelper;1"]
      .getService(SpecialPowers.Ci.nsIClipboardHelper);
   helper.copyString("words");
@@ -186,7 +189,7 @@ add_task(async function test_search_handoff_on_paste() {
   if (UrlbarPrefs.get("quantumbar")) {
     await UrlbarTestUtils.promiseSearchComplete(win);
   }
-  ok(urlBarHasNormalFocus(win), "url bar has normal focused");
+  await urlBarHasNormalFocus(win);
   is(win.gURLBar.value, "@google words", "url bar has search text");

   await BrowserTestUtils.closeWindow(win);
Flags: needinfo?(rrosario)

Why might the change here, not popping the active BrowserParent stack when a window is lowered and no Firefox window is raised, cause APZ stuff to leak on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec596e58136c258581c6849703438336de458c87&selectedJob=249746155 ?

Notably, the stack doesn't increment the refcount for BrowserParent objects.

Flags: needinfo?(kats)

(In reply to Henri Sivonen (:hsivonen) from comment #48)

Notably, the stack doesn't increment the refcount for BrowserParent objects.

The observable change should be BrowserParent::GetFocused returning non-null where it previously returned nullptr, but I don't see obvious APZ callers: https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom13BrowserParent10GetFocusedEv&redirect=false

The "APZ stuff" you see being leaked has the misfortune of being alphabetically superior to all the other things being leaked and so shows up in the TH summary while everything else is omitted. If you look at the full log you'll see all the things being leaked, and in that list is a nsWindow and nsBaseWidget. The APZ stuff is just hanging off those, so if you fix the window leak the APZ stuff will automatically go away.

Flags: needinfo?(kats)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50)

The "APZ stuff" you see being leaked has the misfortune of being alphabetically superior to all the other things being leaked and so shows up in the TH summary while everything else is omitted. If you look at the full log you'll see all the things being leaked, and in that list is a nsWindow and nsBaseWidget. The APZ stuff is just hanging off those, so if you fix the window leak the APZ stuff will automatically go away.

Thanks.

Masayuki, it seems to me that IMEStateManager holds non-owning pointers to nsIWidget. What might be holding an unreleased strong ref?

Flags: needinfo?(masayuki)
Depends on: 1556745

(In reply to Henri Sivonen (:hsivonen) from comment #47)

Thanks. Let's see it on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af7514ecbcced4213fdcc49ab447f711ec773e1

Still fails. Filed bug 1556745.

Well, there is TSFTextStore in the list and the number of leaked widget is just 1. So, I think that your patch does not send NOTIFY_IME_OF_BLUR at shutting down, therefore, there is no change to release TSFTextStore for widget/windows and it keeps grabbing nsWindow.

Flags: needinfo?(masayuki)

Still leaks the window even when observing shutdown:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8310ba2ade2cefaf0b15a4ce7138b691b22c5fd5&selectedJob=250588695

Masayuki, how did TSFTextStore manage not to leak the window prior to the Fission changes? Also, do you have advice relating to the structure of TSFTextStrore regarding how to add nsIAsyncShutdown to it? (The patch whose try run is in comment 58 just hopes that TSFTextStore gets cleaned up soon enough, but since it still leaks, it looks like nsIAsyncShutdown is needed.)

Flags: needinfo?(masayuki)

Also, since TSFTextStore didn't leak before the patch here, does it mean that our shutdown sequence involves lowering the last window right before shutting down? Maybe instead of adding nsIAsyncShutdown, the first popping avoidance code should check if we are shutting down and let the popping happen if we are shutting down.

Let's try checking if we're shutting down in nsFocusManager:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d45c588f310a4c4a4d564f03aafb6a7cda519cc2

(In reply to Henri Sivonen (:hsivonen) from comment #59)

Masayuki, how did TSFTextStore manage not to leak the window prior to the Fission changes? Also, do you have advice relating to the structure of TSFTextStrore regarding how to add nsIAsyncShutdown to it? (The patch whose try run is in comment 58 just hopes that TSFTextStore gets cleaned up soon enough, but since it still leaks, it looks like nsIAsyncShutdown is needed.)

When the widget in the main process is destroyed at shutdown, IMEStateManager::WidgetDestroyed() is called, and it calls IMEStateManager::NotifyIMEOfBlurForChildProcess() and it notifies the destroyed with of NOTIFY_IME_OF_BLUR, then, it notifies TSFTextStore of the blur, finally, TSFTextStore destroys the active instance.

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #62)

When the widget in the main process is destroyed at shutdown, IMEStateManager::WidgetDestroyed() is called

This requires nsBaseWidget::~nsBaseWidget() to run, since it is what calls IMEStateManager::WidgetDestroyed(). What was the previous mechanism that ensured that the reference to the widget from TSFTextStore was nulled out first to allow nsBaseWidget::~nsBaseWidget() to run?

Trying a weak reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8e463559620579749a3c0c2f6b5e989680c140d

(In reply to Henri Sivonen (:hsivonen) from comment #63)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #62)

When the widget in the main process is destroyed at shutdown, IMEStateManager::WidgetDestroyed() is called

This requires nsBaseWidget::~nsBaseWidget() to run

Ah, sorry.

Then, I guess, IMEStateManager::OnDestroyPresContext() -> IMEStateManager::DestroyIMEContentObserver() -> IMEContentObserver::Destroy(), or IMEStateManager::OnTabParentDestoying() -> IMEStateManager::NotifyIMEOfBlurForChildProcess()?

By making the reference from TSFTextStore to the window weak, the leak became substantially smaller:

09:24:40     INFO - TEST-INFO | leakcheck | default leaked 1 TSFTextStore
09:24:40     INFO - TEST-INFO | leakcheck | default leaked 1 TextEventDispatcher
09:24:40     INFO - TEST-INFO | leakcheck | default leaked 1 WeakReference<nsWindowBase>
09:24:40     INFO - TEST-INFO | leakcheck | default leaked 3 nsTArray_base

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251119561&repo=try&lineNumber=14914

I'm guessing we're left with Windows itself holding a reference to TSFTextStore. Let's see if this makes that go away:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb81fb83e1cec29fcb6b578dede596ffc5c7bf7

Why don't you make BrowserParent tell its destruction to IMEStateManager? If so, IMEStateManager can notify widget of NOTIFY_IME_OF_BLUR and that must clean up the leaked objects...

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #66)

Why don't you make BrowserParent tell its destruction to IMEStateManager? If so, IMEStateManager can notify widget of NOTIFY_IME_OF_BLUR and that must clean up the leaked objects...

That's already supposed to happen:
https://searchfox.org/mozilla-central/source/dom/ipc/BrowserParent.cpp#576

Yet, there's this shutdown leak.

(In reply to Henri Sivonen (:hsivonen) from comment #67)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #66)

Why don't you make BrowserParent tell its destruction to IMEStateManager? If so, IMEStateManager can notify widget of NOTIFY_IME_OF_BLUR and that must clean up the leaked objects...

That's already supposed to happen:
https://searchfox.org/mozilla-central/source/dom/ipc/BrowserParent.cpp#576

Trying to restore some old code on the assumption that the current code at that point isn't forceful enough:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a22f74c1d459b5f1e2480fc11a0b44e1219a8389

(In reply to Henri Sivonen (:hsivonen) from comment #68)

(In reply to Henri Sivonen (:hsivonen) from comment #67)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #66)

Why don't you make BrowserParent tell its destruction to IMEStateManager? If so, IMEStateManager can notify widget of NOTIFY_IME_OF_BLUR and that must clean up the leaked objects...

That's already supposed to happen:
https://searchfox.org/mozilla-central/source/dom/ipc/BrowserParent.cpp#576

Trying to restore some old code on the assumption that the current code at that point isn't forceful enough:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a22f74c1d459b5f1e2480fc11a0b44e1219a8389

Still leaks. By code inspection I found one case where we null out sFocusedIMEWidget without notifying it of blur first. Let's try changing that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffd11c1f2205169ab6f9904149997d0135742986

Or perhaps we need to null out sContent and sPresContext on shutdown?

(In reply to Henri Sivonen (:hsivonen) from comment #71)

Or perhaps we need to null out sContent and sPresContext on shutdown?

Trying to set sWidget later and nulling out sContent and sPresContext when the chrome focus gets focus:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0265b853f4162cca110f8144035deab8b38b3251

(In reply to Henri Sivonen (:hsivonen) from comment #72)

(In reply to Henri Sivonen (:hsivonen) from comment #71)

Or perhaps we need to null out sContent and sPresContext on shutdown?

Trying to set sWidget later and nulling out sContent and sPresContext when the chrome focus gets focus:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0265b853f4162cca110f8144035deab8b38b3251

Still leaks.

Let's try combining this with the earlier attempt of trying to make Windows release TSFTextStore on the TSFTextStore termination path if not already released:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=431007145d48b8f110fff8175f3b503cd38a100a

Indeed, looks like that NOTIFY_IME_OF_BLUR is called via DestroyIMEContentObserver() or directly from IMEStateManager::OnFocusMovedBetweenBrowsers().

I wonder, IMEStateManager::sFocusedIMEBrowserParent is StaticRefPtr and it's managed at same time of sFocusedIMEWidget. I guess that it means that NotifyIME(NOTIFY_IME_OF_BLUR) failed at some point. That might be:

However, I don't see the latter's warning in the log. TextEventDispatcher::mListener is cleared when OnDestroyWidget() is called. And it's called when nsIWidget::OnDestroy() is called. So, it must be too early in this case.

One possible solution is, you make nsBaseWidget::OnDestroy() call IMEStateManager::WidgetDestroyed(). Then, NOTIFY_IME_OF_BLUR must be handled as expected.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #75)

One possible solution is, you make nsBaseWidget::OnDestroy() call IMEStateManager::WidgetDestroyed(). Then, NOTIFY_IME_OF_BLUR must be handled as expected.

Thanks. Trying that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8505b09c1436101221d0b91c5bf4b58b3aad76

(In reply to Henri Sivonen (:hsivonen) from comment #76)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #75)

One possible solution is, you make nsBaseWidget::OnDestroy() call IMEStateManager::WidgetDestroyed(). Then, NOTIFY_IME_OF_BLUR must be handled as expected.

Thanks. Trying that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8505b09c1436101221d0b91c5bf4b58b3aad76

Still leaks. :-(

Upon quit, notify all remaining widgets of IME blur no matter what:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f095060bd092d9df64dbe1017038af46f089049

Also prevent IME from re-gaining focus once we're quitting:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0fd8d571c9be0dabf215e9dbc09265ca933bd2

(In reply to Henri Sivonen (:hsivonen) from comment #79)

Also prevent IME from re-gaining focus once we're quitting:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0fd8d571c9be0dabf215e9dbc09265ca933bd2

Still leaks.

Masayuki, do you have more ideas of what could either cause Gecko not to release references to TSFTextStore or to cause Windows not to do so?

AFAICT, the only case where notifying IME of blur does not decrement the reference count immediately is the case where we are in the middle of processing a key message from Windows, but I'd hope the test harness isn't sending Gecko key events upon shutdown.

Oops. Forgot to register the observer for the right topic.

Attachment #9069881 - Attachment is obsolete: true

(In reply to Henri Sivonen (:hsivonen) from comment #82)

With the additional topic registration:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a60cb341a59204e26a6456d8dd267f962e596bce

Progress! Let's see if some of the intermediate steps were unnecessary:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=357224c0bc30db01aac38b9177deacb4a6944e21

Another try run for comparing random oranges:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59553097f91c15ea62c7a255799c46d85f5f50f

(Looks like the fix for the leak on Windows needs to be disabled on Android.)

Congratulations! But I think that it should be enabled in non-Android platforms because only Android widget handles IME in each content process so that in the future, same leak may occur in macOS and Linux if something depends on NOTIFY_IME_OF_BLUR.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #87)

Congratulations! But I think that it should be enabled in non-Android platforms because only Android widget handles IME in each content process so that in the future, same leak may occur in macOS and Linux if something depends on NOTIFY_IME_OF_BLUR.

OK. Let's try that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b1a1b6dcce3fce1824b1ef276afa377edd38ed5

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05f060556310
Avoid popping BrowserParent in response to window lowering. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/164ae5b4ff06
addendum - Notify widgets of IME blur on quit (on non-Android). r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9b9e4c12e414
test adjustment - Disable browser_privatebrowsing_about.js due to perma-orange. r=r1cky

Per comment 6 it sounds like we probably want this in 68? Please request uplift if so.

Flags: needinfo?(hsivonen)

If it's too late/risky for 68, I think that we should fix this only in 68 ESR when this patch goes to late beta.

(In reply to Julien Cristau [:jcristau] from comment #93)

Per comment 6 it sounds like we probably want this in 68? Please request uplift if so.

I intend to request uplift, but I'd like to wait for a Nightly cycle just in case.

Flags: needinfo?(hsivonen)

Also, I can't verify Nightly with Atok myself at the moment, so it would be great if someone with access to Atok could confirm that the latest Nightly works OK.

I tested ATOK MOJI Pallet with build 40c99f4752f968db32f189243b4432b18d1b3471 (2019-06-17).
There is no problem.

Comment on attachment 9064401 [details]
Bug 1549930 - Avoid popping BrowserParent in response to window lowering.

Beta/Release Uplift Approval Request

  • User impact if declined: Users of ATOK, a third-party Japanese IME, won't be able to use a character palette feature of ATOK to enter text into Firefox on Windows. (The primary IME functionality of ATOK is unaffected: The bug relates only to a secondary character palette feature of ATOK.)
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): I estimate the risk is somewhere between low and medium. The reason why risk should be low are 1) the behavior of keeping IME focused when Firefox stops being the frontmost app restores a behavior that existed previously, 2) when switching windows within Firefox, the result with the change is supposed to go through the same state transition only with a different trigger mechanism, and 3) the shutdown changes affect the app only when shutting down and IME doesn't need to work when we've committed to shutting down. The reason for saying this should be towards medium is that when switching Firefox windows, state changes that previously triggered on one of the windows getting lowered are now triggered from the other window getting raised instead. The effect is supposed to be equivalent in the case Firefox as a whole stays as the frontmost app, but there's a non-zero chance that it isn't exactly equivalent after all.

The alternatives would be either to wait a little longer on Nightly before uplift to 68 beta or to ship non-ESR 68 with the regression and uplifting only to 68 ESR after even more time on Nightly.

  • String changes made/needed: None
Attachment #9064401 - Flags: approval-mozilla-beta?
Attachment #9072097 - Flags: approval-mozilla-beta?
Attachment #9072098 - Flags: approval-mozilla-beta?

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #97)

I tested ATOK MOJI Pallet with build 40c99f4752f968db32f189243b4432b18d1b3471 (2019-06-17).
There is no problem.

Thank you for testing (and for reporting the bug in the first place). Based on your comment, I marked this as verified in Nightly in comment 98.

Comment on attachment 9064401 [details]
Bug 1549930 - Avoid popping BrowserParent in response to window lowering.

ime fix for 68.0b12

Attachment #9064401 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9072097 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9072098 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Takeshi Ichimaru would you mind testing on latest beta build as well?
https://archive.mozilla.org/pub/firefox/candidates/68.0b12-candidates/build1/

status-firefox69: fixed → verified
Flags: needinfo?(ayakawa.m)

I tested with 68.0b12-candidates build1, and there is no problem.

Flags: needinfo?(ayakawa.m)
Regressions: 1562796

(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #103)

I tested with 68.0b12-candidates build1, and there is no problem.

Thanks so much. Closing the bug based on your verification.

Status: RESOLVED → VERIFIED
status-firefox68: fixed → verified
Depends on: 1592544
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: