1469875 - Crash in mozilla::widget::PuppetWidget::RequestIMEToCommitComposition

Status: RESOLVED FIXED

(Core :: Plug-ins, defect, P3, critical)

Description

[Mats Palmgren (:mats)]

This bug was filed from the Socorro interface and is
report bp-b479f9b7-0044-4fb9-bbee-8edef0180618.
=============================================================


MOZ_CRASH Reason:  MOZ_RELEASE_ASSERT(composition->IsRequestingCommitOrCancelComposition()) (Requesting commit or cancel composition should be requested via TextComposition instance)


Top 10 frames of crashing thread:

0 xul.dll mozilla::widget::PuppetWidget::RequestIMEToCommitComposition widget/PuppetWidget.cpp:682
1 xul.dll mozilla::widget::TextEventDispatcher::NotifyIME widget/TextEventDispatcher.cpp:463
2 xul.dll nsBaseWidget::NotifyIME widget/nsBaseWidget.cpp:1906
3 xul.dll nsPluginInstanceOwner::RequestCommitOrCancel dom/plugins/base/nsPluginInstanceOwner.cpp:898
4 xul.dll mozilla::plugins::PluginInstanceParent::RecvRequestCommitOrCancel dom/plugins/ipc/PluginInstanceParent.cpp:2294
5 xul.dll mozilla::plugins::PPluginInstanceParent::OnMessageReceived ipc/ipdl/PPluginInstanceParent.cpp:1592
6 xul.dll mozilla::plugins::PPluginModuleParent::OnMessageReceived ipc/ipdl/PPluginModuleParent.cpp:795
7 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2134
8 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2064
9 xul.dll mozilla::ipc::MessageChannel::Call ipc/glue/MessageChannel.cpp:1686

=============================================================

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

Ah, nsPluginInstanceOwner::RequestCommitOrCancel() does not call nsIWidget::NotifyIME() via TextComposition. That is the reason why the users hit the assertion.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

I do not exactly remember how we handle composition events between widget and plugin, but according to nsPluginInstanceOwner::DispatchCompositionToPlugin() and nsPluginInstanceOwner::HandleEvent(), we dispatch composition events normally from widget, creates TextComposition instance normally. So, nsPluginInstanceOwner::RequestCommitOrCancel() should use TextComposition. If there is no instance, it should do nothing since requesting to commit/cancel composition doesn't make sense when there is no composition.

Comment 3

[Jim Mathies [:jimm]]

Hey Masayuki, do you consider this serious? Looks to be super low volume (8 crashes over 7 days). Marking this fix option bu feel free to re-prioritize.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

(In reply to Jim Mathies [:jimm] from comment #3)
> Hey Masayuki, do you consider this serious? Looks to be super low volume (8
> crashes over 7 days). Marking this fix option bu feel free to re-prioritize.

Must be rare case since this occurs when Flash Player tries to commit existing composition forcibly, perhaps, for example, moving focus with committing composition string when composing string becomes what they want. So, with usual Flash Player apps, this must occur. On the other hand, such app may cause this crash almost always.  Anyway, the fix must be easy. I'll take a look, perhaps, I have much time for this today.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1636dc770e2563c8d192a25942800c4de5504885

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

Attachment: Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

Any content code except TextComposition shouldn't call nsIWidget::NotifyIM()
since IMEStateManager and TextComposition manage state of the composition.
Therefore, we need to make nsPluginInstanceOwner::RequestCommitOrCancel()
call IMEStateManager::NotifyIME() instead.

Additionally, this method should ignore the request if composition has
already been gone.  This patch makes check whether there is a TextComposition
instance for the widget and the composition is handled in the plugin owner
element.

Review commit: https://reviewboard.mozilla.org/r/254170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254170/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

FYI: This patch fixes only the crash.  After applying this patch, composition string (underlined string) may remain even after committing composition actually.  As far as I've debugged, it must be out of scope of this bug.

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8989105 [details]
Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

https://reviewboard.mozilla.org/r/254170/#review260946

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28aec373fa43
Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME() r=m_kato

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/28aec373fa43

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this safe to consider for backport to Beta? Looks like we won't be able to determine how effective the patch is until it arrives there.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

Sure, I'll try to check whether this is upliftable.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

The patch is cleanly graftable. I'll request Beta approval.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

Comment on attachment 8989105 [details]
Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1208944.

[User impact if declined]:
May meet the crash.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes. We don't have crash reports with Nightly after landing the patch:
https://crash-stats.mozilla.com/signature/?version=63.0a1&signature=mozilla%3A%3Awidget%3A%3APuppetWidget%3A%3ARequestIMEToCommitComposition&date=%3E%3D2018-07-08T23%3A48%3A24.000Z&date=%3C2018-08-08T23%3A48%3A24.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
Nothing.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
The method called nsIWidget API directly. However, before calling that, the API caller needs to manage TextComposition instance which represents current composition.  Instead, this patch makes the method calls the API via IMEStateManager since it manages TextComposition and this is standard manner in content.

[String changes made/needed]:
No.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8989105 [details]
Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

IME crash fix, for 62.0b16

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/242c40439f4d