Closed Bug 1805816 Opened 2 years ago Closed 2 years ago

`IMEStateManager::WidgetOnQuit` should clean up correctly with sending `NOTIFY_IME_OF_BLUR`

Categories

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

defect

Tracking

()

RESOLVED FIXED
110 Branch
Tracking Status
firefox110 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(1 file)

Currently, it has a hack:

  // And then in case the normal way didn't work:
  nsCOMPtr<nsIWidget> quittingWidget(aWidget);
  quittingWidget->NotifyIME(IMENotification(NOTIFY_IME_OF_BLUR));

hsivonen said:

Unfortunately, we need this. Clearly, the leaking widget isn't sFocusedIMEWidget:

That's appeared only on Windows according to the trychooser data.

Only on Windows, IME wants to keep composing even while the application is inactive since most IMEs allow it. Due to this request, IMEStateManager does not destroy IMEContentObserver when IMEStateManager::OnChangeFocusInternal is called with nullptr for both aPresConext and aElement.

Therefore, IMEStateManager lost a chance to notify widget/IME/IME handler of blur at quitting application because from IMEStateManager::OnChangeFocusInternal point of view, the focus change looks like that the application is just inactivated. Then, the last TSFTextStore instance does not have a chance to release itself due to no NOTIFY_IME_OF_BLUR notification. Finally, IMEHandler::OnDestroyWindow hits the assertion if the IMEStateManager hack is removed.

Fortunately, IMEStateManager::WidgetOnQuit is called with widget which is not
yet destroyed. Therefore, the widget still be able to handle notifications to
IME.

The problem here is, the widget still have editable input context and there is
a native IME handler instance if it's alive during NOTIFY_IME_OF_FOCUS and
NOTIFY_IME_OF_BLUR. TSFTextStore which is IME handler in Windows matches
this case, and it wants to keep composition even if the application becomes
inactive. Therefore, preceding IMEStateManager::OnChangeFocusInternal call
with nullptr for both aPresContext and aElement cannot notify IME of blur
with destroying active IMEContentObserver [1] because at the moment, the
shutting down process has not started yet.

For solving this issue, IMEStateManager::WidgetOnQuit directly notifies
the widget of NOTIFY_IME_OF_BLUR to clean it up the last TSFTextStore
instance. However, this would cause NS_ASSERTION [3] after changing something
of NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR. Therefore, I need to fix
this bug for other issues.

For solving this, IMEStateManager::WidgetOnQuit should destroy active
IMEContentObserver with normal path to send NOTIFY_IME_OF_BLUR and release
other unnecessary objects before calling
IMEStateManager::DestroyIMEContentObserver which clears sFocusedIMEWidget
and makes active IMEContentObserver impossible to send NOTIFY_IME_OF_BLUR
due to illegal state.

  1. https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/events/IMEStateManager.cpp#549-551
  2. https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/events/IMEStateManager.cpp#242-244
  3. https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/widget/windows/TSFTextStore.cpp#5746
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/dbf6711c5788 Make `IMEStateManager::WidgetOnQuit` call `DestroyIMEContentObserver` before calling `WidgetDestroyed` r=smaug
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: