Closed Bug 1469875 Opened Last year Closed Last year

Crash in mozilla::widget::PuppetWidget::RequestIMEToCommitComposition

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: mats, Assigned: masayuki)

References

Details

(Keywords: crash, inputmethod, regression)

Crash Data

Attachments

(1 file)

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

=============================================================
Flags: needinfo?(masayuki)
Ah, nsPluginInstanceOwner::RequestCommitOrCancel() does not call nsIWidget::NotifyIME() via TextComposition. That is the reason why the users hit the assertion.
Blocks: 1208944
Component: Widget → Plug-ins
Flags: needinfo?(masayuki)
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.
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.
Flags: needinfo?(masayuki)
Priority: -- → P3
(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.
Flags: needinfo?(masayuki)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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 on attachment 8989105 [details]
Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

https://reviewboard.mozilla.org/r/254170/#review260946
Attachment #8989105 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/28aec373fa43
Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME() r=m_kato
https://hg.mozilla.org/mozilla-central/rev/28aec373fa43
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Flags: needinfo?(masayuki)
Sure, I'll try to check whether this is upliftable.
The patch is cleanly graftable. I'll request Beta approval.
Flags: needinfo?(masayuki)
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.
Attachment #8989105 - Flags: approval-mozilla-beta?
Comment on attachment 8989105 [details]
Bug 1469875 - Make nsPluginInstanceOwner::RequestCommitOrCancel() call IMEStateManager::NotifyIME() rather than calling nsIWidget::NotifyIME()

IME crash fix, for 62.0b16
Attachment #8989105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.