Closed
Bug 1469875
Opened 4 years ago
Closed 4 years ago
Crash in mozilla::widget::PuppetWidget::RequestIMEToCommitComposition
Categories
(Core Graveyard :: Plug-ins, defect, P3)
Tracking
(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)
RESOLVED
FIXED
mozilla63
People
(Reporter: MatsPalmgren_bugz, Assigned: masayuki)
References
Details
(Keywords: crash, inputmethod, regression)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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)
Assignee | ||
Comment 1•4 years ago
|
||
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)
Keywords: inputmethod,
regression
Assignee | ||
Comment 2•4 years ago
|
||
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•4 years ago
|
||
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)
![]() |
||
Updated•4 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•4 years ago
|
||
(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 | ||
Comment 5•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1636dc770e2563c8d192a25942800c4de5504885
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cba27289ba80f7e6af87c5ed224ff3e584dff03
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•4 years ago
|
||
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•4 years ago
|
||
mozreview-review |
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+
Comment 10•4 years ago
|
||
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
Comment 11•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28aec373fa43
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•4 years ago
|
status-firefox61:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Comment 12•4 years ago
|
||
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)
Assignee | ||
Comment 13•4 years ago
|
||
Sure, I'll try to check whether this is upliftable.
Assignee | ||
Comment 14•4 years ago
|
||
The patch is cleanly graftable. I'll request Beta approval.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 15•4 years ago
|
||
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 16•4 years ago
|
||
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+
Comment 17•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/242c40439f4d
Updated•5 days ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•