1172466 - IMEContentObserver shouldn't notify IME during reflow

Status: RESOLVED FIXED

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

Description

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

IMEContentObserver shouldn't notify IME during reflow because if IME tries to query content synchronously, ContentEventHandler will call nsIPresShell::FlushPendingNotifications() again.

Comment 1

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

Don't we have some checks in presshell to prevent calling Flush...() if we're currently flushing.

Comment 2

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

(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #1)
> Don't we have some checks in presshell to prevent calling Flush...() if
> we're currently flushing.

Yes. PresShell::FlushPendingNotifications() does nothing without error. Therefore, ContentEventHandler tries to query content but sometimes failed that due to some frames are still reflowing. I confirmed this occurs at reframing an editor, see following log:

> 03:06:31     INFO -  [Child 1894] ###!!! ASSERTION: Can only call this on frames that have been reflowed: '!(GetStateBits() & NS_FRAME_FIRST_REFLOW) || (GetParent()->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE)', file /builds/slave/try-lx-d-000000000000000000000/build/src/layout/generic/nsTextFrame.cpp, line 2705
> 03:06:31     INFO -  #01: nsTextFrame::GetTrimmedOffsets(nsTextFragment const*, bool, bool) [layout/generic/nsTextFrame.cpp:2702]
> 03:06:31     INFO -  #02: nsTextFrame::PeekOffsetCharacter(bool, int*, bool) [layout/generic/nsTextFrame.cpp:6961]
> 03:06:31     INFO -  #03: mozilla::ContentEventHandler::ExpandToClusterBoundary(nsIContent*, bool, unsigned int*) [dom/events/ContentEventHandler.cpp:639]
> 03:06:31     INFO -  #04: mozilla::ContentEventHandler::SetRangeFromFlatTextOffset(nsRange*, unsigned int, unsigned int, mozilla::LineBreakType, bool, unsigned int*) [dom/events/ContentEventHandler.cpp:718]
> 03:06:31     INFO -  #05: mozilla::ContentEventHandler::OnQueryTextRect(mozilla::WidgetQueryContentEvent*) [dom/events/ContentEventHandler.cpp:918]
> 03:06:31     INFO -  #06: mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) [dom/events/EventStateManager.cpp:764]
> 03:06:31     INFO -  #07: PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*) [layout/base/nsPresShell.cpp:8163]
> 03:06:31     INFO -  #08: PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) [layout/base/nsPresShell.cpp:7900]
> 03:06:31     INFO -  #09: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) [view/nsViewManager.cpp:788]
> 03:06:31     INFO -  #10: nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) [view/nsView.cpp:1094]
> 03:06:31     INFO -  #11: mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) [widget/PuppetWidget.cpp:317]
> 03:06:31     INFO -  #12: mozilla::ContentCache::QueryCharRect(nsIWidget*, unsigned int, mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel>&) const [xpcom/glue/nsDebug.h:40]
> 03:06:31     INFO -  #13: mozilla::ContentCache::CacheTextRects(nsIWidget*, mozilla::widget::IMENotification const*) [xpcom/glue/nsDebug.h:40]
> 03:06:31     INFO -  #14: mozilla::ContentCache::CacheSelection(nsIWidget*, mozilla::widget::IMENotification const*) [widget/ContentCache.cpp:433]
> 03:06:31     INFO -  #15: mozilla::ContentCache::CacheText(nsIWidget*, mozilla::widget::IMENotification const*) [widget/ContentCache.cpp:549]
> 03:06:31     INFO -  #16: mozilla::ContentCache::CacheAll(nsIWidget*, mozilla::widget::IMENotification const*) [xpcom/glue/nsDebug.h:40]
> 03:06:31     INFO -  #17: mozilla::widget::PuppetWidget::NotifyIMEOfFocusChange(mozilla::widget::IMENotification const&) [xpcom/glue/nsDebug.h:40]
> 03:06:32     INFO -  #18: nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) [widget/nsBaseWidget.cpp:1678]
> 03:06:32     INFO -  #19: mozilla::IMEContentObserver::Init(nsIWidget*, nsPresContext*, nsIContent*, nsIEditor*) [dom/events/IMEContentObserver.cpp:185]
> 03:06:32     INFO -  #20: mozilla::IMEStateManager::CreateIMEContentObserver(nsIEditor*) [dom/events/IMEStateManager.cpp:1252]
> 03:06:32     INFO -  #21: mozilla::IMEStateManager::UpdateIMEState(mozilla::widget::IMEState const&, nsIContent*, nsIEditor*) [dom/events/IMEStateManager.cpp:725]
> 03:06:32     INFO -  #22: nsEditor::SetFlags(unsigned int) [editor/libeditor/nsEditor.cpp:524]
> 03:06:32     INFO -  #23: nsEditorEventListener::SpellCheckIfNeeded() [editor/libeditor/nsEditorEventListener.cpp:1136]
> 03:06:32     INFO -  #24: nsEditor::PostCreate() [editor/libeditor/nsEditor.cpp:328]
> 03:06:32     INFO -  #25: nsTextEditorState::PrepareEditor(nsAString_internal const*) [dom/html/nsTextEditorState.cpp:1473]
> 03:06:32     INFO -  #26: PrepareEditorEvent::Run() [ipc/chromium/src/base/task.h:27]
> 03:06:32     INFO -  #27: nsContentUtils::RemoveScriptBlocker() [xpcom/glue/nsCOMPtr.h:1038]
> 03:06:32     INFO -  #28: nsAutoScriptBlocker::~nsAutoScriptBlocker() [dom/base/nsContentUtils.h:2471]
> 03:06:32     INFO -  #29: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4319]
> 03:06:32     INFO -  #30: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [xpcom/base/nsRefPtr.h:231]

I wonder why PresShell thinks that it's safe to run script during reflow...

Comment 3

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

Attachment: part.1 Make helper classes to notify IME nested classes of IMEContentObserver

Comment 4

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

Attachment: part.2 Use runnable event for notifying IME of focus/blur

Comment 5

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

Attachment: part.3 Create an abstruct class which is a base class of classes notifying IME

Comment 6

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

Attachment: part.4 Don't notify IME during reflow

Comment 7

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

Attachment: part.1 Make helper classes to notify IME nested classes of IMEContentObserver

First, all helper classes which notifies IME of text/selection/position change with IMEContentObserver should be nested classes of it since it allows to access whole members without friend class definitions.

This doesn't change any logic.

Comment 8

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

Attachment: part.2 Use runnable event for notifying IME of focus

Next, this makes IMEContentObserver use nsRunnable to notify IME of focus.

I was tried to do same thing at blur. However, it needs big and complicated changes because current IMEContentObserver cannot guarantee the blur notification is retried after it's destroyed but before next focus in another instance. However, fortunately, as far as I know nobody queries content at/after blur. So, this must not cause any problem in most environments. Therefore, I give up to do this for now.

This patch makes IMEContentObserver stores if it's notified IME of focus because focus notification is now notified asynchronously. So, checking mWidget is not same as checking mIMEHasFocus now.

Comment 9

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

Attachment: part.3 Create an abstruct class which is a base class of classes notifying IME

Each helper class should check almost same things before notifying IME. Therefore, we should have an abstract class which implements such utility methods.

Comment 10

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

Attachment: part.4 Don't notify IME during reflow

This is the main patch of this bug.

If it's in reflowing when each helper class is run, it needs to register itself to IMEContentObserver again. Then, IMEContentObserver::FlushMergeableNotification() should be called later.

If it's in reflow, IMEContentObserver::Reflow() or ReflowInterruptible() should be called. Therefore, we need additionally to guarantee at the end of Init() only for async focus notification.

Comment 11

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

Comment on attachment 8621402 [details] [diff] [review]
part.1 Make helper classes to notify IME nested classes of IMEContentObserver

(personally I'm not a fan of nested classes, but fine.)

Comment 12

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

Comment on attachment 8621406 [details] [diff] [review]
part.2 Use runnable event for notifying IME of focus

> IMEContentObserver::NotifyIMEOfBlur()
> {
>-  // If this failed to initialize, mRootContent may be null, then, we
>-  // should not call NotifyIME(IMENotification(NOTIFY_IME_OF_BLUR))
>-  if (!mRootContent || !mWidget) {
>+  // Prevent any notifications to be sent IME.
>+  nsCOMPtr<nsIWidget> widget(mWidget);
>+  mWidget = nullptr;
Perhaps use swap() here.

>@@ -289,18 +302,16 @@ void
> IMEContentObserver::Destroy()
> {
>   // WARNING: When you change this method, you have to check Unlink() too.
> 
>   NotifyIMEOfBlur();
>   UnregisterObservers();
> 
>   mEditor = nullptr;
>-  // Even if there are some pending notification, it'll never notify the widget.
>-  mWidget = nullptr;
I wonder if we should still keep this, though perhaps remove the comment.

Comment 13

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

Comment on attachment 8621407 [details] [diff] [review]
part.3 Create an abstruct class which is a base class of classes notifying IME

I guess this makes the code a bit less error prone.
Though, one needs to always remember to update CanNotifyIME() when
changing what various classes do, and that is a bit annoying.

Comment 14

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

Comment on attachment 8621408 [details] [diff] [review]
part.4 Don't notify IME during reflow

> IMEContentObserver::FocusSetEvent::Run()
> {
>   if (!CanNotifyIME()) {
>     // If IMEContentObserver has already gone, we don't need to notify IME of
>     // focus.
>     mIMEContentObserver->ClearPendingNotifications();
>     return NS_OK;
>   }
> 
>+  if (!IsSafeToNotifyIME()) {
>+    mIMEContentObserver->MaybeNotifyIMEOfFocusSet();
>+    return NS_OK;
>+  }
I don't understand this. We're in Run method of a runnable. How could it not be safe here?

> IMEContentObserver::TextChangeEvent::Run()
> {
>   if (!CanNotifyIME()) {
>     return NS_OK;
>   }
> 
>+  if (!IsSafeToNotifyIME()) {
>+    mIMEContentObserver->MaybeNotifyIMEOfTextChange(mData);
>+    return NS_OK;
>+  }

ditto

> IMEContentObserver::PositionChangeEvent::Run()
> {
>   if (!CanNotifyIME()) {
>     return NS_OK;
>   }
> 
>+  if (!IsSafeToNotifyIME()) {
>+    mIMEContentObserver->MaybeNotifyIMEOfPositionChange();
>+    return NS_OK;
>+  }
ditto

Comment 15

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

(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #14)
> Comment on attachment 8621408 [details] [diff] [review]
> part.4 Don't notify IME during reflow
> 
> > IMEContentObserver::FocusSetEvent::Run()
> > {
> >   if (!CanNotifyIME()) {
> >     // If IMEContentObserver has already gone, we don't need to notify IME of
> >     // focus.
> >     mIMEContentObserver->ClearPendingNotifications();
> >     return NS_OK;
> >   }
> > 
> >+  if (!IsSafeToNotifyIME()) {
> >+    mIMEContentObserver->MaybeNotifyIMEOfFocusSet();
> >+    return NS_OK;
> >+  }
> I don't understand this. We're in Run method of a runnable. How could it not
> be safe here?

Because IMEContentObserver may be initialized during reflow. Unfortunately, ContentEventHandler may not work well during reflow if the target has not finished to be reflowed yet. So, we can say, it's not safe to notify IME of anything during reflow. If script blocker also checks the reflow state, we don't need this patch. However, making it so is too risky and I'm not 100% sure if it's correct.

Comment 16

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

But we don't call nsRunnables (or their Run() method) during reflow.
(or if we do, that is a sec-critical bug)

Comment 17

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

Oh, are we using these Runnables as script runners, not as things dispatched to event loop?
If so, yeah, then IsSafeToNotifyIME() in Run() makes sense.

Comment 18

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

Comment on attachment 8621408 [details] [diff] [review]
part.4 Don't notify IME during reflow

I see. The AddScriptRunner call wasn't just anywhere in this patch so I assume DispatchToCurrentThread was used.


But, 
GetChangeEventTypeName nor GetStateName aren't used anywhere.

Comment 19

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

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8621408 [details] [diff] [review]
> part.4 Don't notify IME during reflow
> But, 
> GetChangeEventTypeName nor GetStateName aren't used anywhere.

Ah, they are used when I debugged some bugs in the patches. I'll remove them.

Comment 20

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

Attachment: part.2 Use runnable event for notifying IME of focus (r=smaug)

Comment 21

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

Attachment: part.4 Don't notify IME during reflow

Comment 22

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

Comment on attachment 8622160 [details] [diff] [review]
part.4 Don't notify IME during reflow

I don't understand IMEContentObserver::SelectionChangeEvent::Run() calling
mIMEContentObserver->MaybeNotifyIMEOfSelectionChange(mCausedByComposition);
Doesn't that lead to endless recursion, since if we're
in ::Run(), we don't have any script blocker, and that means 
nsContentUtils::AddScriptRunner will execute the runnable immediately.

Similar with TextChangeEvent::Run() and PositionChangeEvent::Run()

Comment 23

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

Attachment: part.4 Don't notify IME during reflow

Right. When IMEContentObserver::IsSafeToNotifyIME() returns true but AChangeEvent::IsSafeToNotifyIME() returns false, it causes infinite recursive calls.

So, they should just mark the flags as pending notification. Then, Init() or Reflow() will flush the notifications again.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c215b9a30b73
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a659053a52
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e481878f2b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a633cd8ac3d9

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c215b9a30b73
https://hg.mozilla.org/mozilla-central/rev/83a659053a52
https://hg.mozilla.org/mozilla-central/rev/6e481878f2b0
https://hg.mozilla.org/mozilla-central/rev/a633cd8ac3d9