Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process r?m_kato
ContentCacheInParent::mPendingCompositionCount is now managed with composition events which TabParent received. However, TextComposition doesn't dispatch composition events after coming request to commit active composition. Therefore, composition is committed forcibly in a remote process over 255 times, the main process crashes.
It's the safest way to use TextComposition to manage ContentCacheInParent::mPendingCompositionCount.
MozReview-Commit-ID: DEhzYcK1zcW
--- a/dom/events/IMEStateManager.cpp
+++ b/dom/events/IMEStateManager.cpp
@@ -272,18 +272,19 @@ IMEStateManager::OnDestroyPresContext(ns
if (sTextCompositions) {
TextCompositionArray::index_type i =
sTextCompositions->IndexOf(aPresContext);
if (i != TextCompositionArray::NoIndex) {
MOZ_LOG(sISMLog, LogLevel::Debug,
(" OnDestroyPresContext(), "
"removing TextComposition instance from the array (index=%" PRIuSIZE ")", i));
// there should be only one composition per presContext object.
- sTextCompositions->ElementAt(i)->Destroy();
+ RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
sTextCompositions->RemoveElementAt(i);
+ composition->Destroy();
if (sTextCompositions->IndexOf(aPresContext) !=
TextCompositionArray::NoIndex) {
MOZ_LOG(sISMLog, LogLevel::Error,
(" OnDestroyPresContext(), FAILED to remove "
"TextComposition instance from the array"));
MOZ_CRASH("Failed to remove TextComposition instance from the array");
}
}
@@ -1288,20 +1289,21 @@ IMEStateManager::DispatchCompositionEven
if ((!aIsSynthesized ||
composition->WasNativeCompositionEndEventDiscarded()) &&
aCompositionEvent->CausesDOMCompositionEndEvent()) {
TextCompositionArray::index_type i =
sTextCompositions->IndexOf(aCompositionEvent->mWidget);
if (i != TextCompositionArray::NoIndex) {
MOZ_LOG(sISMLog, LogLevel::Debug,
(" DispatchCompositionEvent(), "
- "removing TextComposition from the array since NS_COMPOSTION_END "
+ "removing TextComposition from the array since eCompositionEnd "
"was dispatched"));
- sTextCompositions->ElementAt(i)->Destroy();
+ RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
sTextCompositions->RemoveElementAt(i);
+ composition->Destroy();
}
}
}
// static
IMEContentObserver*
IMEStateManager::GetActiveContentObserver()
{
--- a/dom/events/TextComposition.cpp
+++ b/dom/events/TextComposition.cpp
@@ -65,26 +65,40 @@ TextComposition::TextComposition(nsPresC
, mIsRequestingCommit(false)
, mIsRequestingCancel(false)
, mRequestedToCommitOrCancel(false)
, mWasNativeCompositionEndEventDiscarded(false)
, mAllowControlCharacters(
Preferences::GetBool("dom.compositionevent.allow_control_characters",
false))
, mWasCompositionStringEmpty(true)
+ , mHasDispatchedCompositionEvents(false)
{
MOZ_ASSERT(aCompositionEvent->mNativeIMEContext.IsValid());
}
+TextComposition::~TextComposition()
+{
+ // WARNING: mPresContext may be destroying, so, be careful if you touch it.
+ if (NS_WARN_IF(mTabParent)) {
+ Destroy();
+ }
+}
+
void
TextComposition::Destroy()
{
mPresContext = nullptr;
mNode = nullptr;
- mTabParent = nullptr;
+ if (mTabParent) {
+ RefPtr<TabParent> tabParent = mTabParent.forget();
+ if (mHasDispatchedCompositionEvents) {
+ tabParent->OnDestroyTextComposition();
+ }
+ }
// TODO: If the editor is still alive and this is held by it, we should tell
// this being destroyed for cleaning up the stuff.
}
bool
TextComposition::IsValidStateForComposition(nsIWidget* aWidget) const
{
return !Destroyed() && aWidget && !aWidget->Destroyed() &&
@@ -146,16 +160,17 @@ void
TextComposition::DispatchEvent(WidgetCompositionEvent* aDispatchEvent,
nsEventStatus* aStatus,
EventDispatchingCallback* aCallBack,
const WidgetCompositionEvent *aOriginalEvent)
{
nsPluginInstanceOwner::GeneratePluginEvent(aOriginalEvent,
aDispatchEvent);
+ mHasDispatchedCompositionEvents = true;
EventDispatcher::Dispatch(mNode, mPresContext,
aDispatchEvent, nullptr, aStatus, aCallBack);
OnCompositionEventDispatched(aDispatchEvent);
}
void
TextComposition::OnCompositionEventDiscarded(
@@ -244,16 +259,17 @@ TextComposition::DispatchCompositionEven
EventDispatchingCallback* aCallBack,
bool aIsSynthesized)
{
mWasCompositionStringEmpty = mString.IsEmpty();
// If the content is a container of TabParent, composition should be in the
// remote process.
if (mTabParent) {
+ mHasDispatchedCompositionEvents = true;
Unused << mTabParent->SendCompositionEvent(*aCompositionEvent);
aCompositionEvent->StopPropagation();
if (aCompositionEvent->CausesDOMTextEvent()) {
mLastData = aCompositionEvent->mData;
mLastRanges = aCompositionEvent->mRanges;
// Although, the composition event hasn't been actually handled yet,
// emulate an editor to be handling the composition event.
EditorWillHandleCompositionChangeEvent(aCompositionEvent);
--- a/dom/events/TextComposition.h
+++ b/dom/events/TextComposition.h
@@ -173,20 +173,17 @@ public:
RefPtr<TextComposition> mComposition;
CompositionChangeEventHandlingMarker();
CompositionChangeEventHandlingMarker(
const CompositionChangeEventHandlingMarker& aOther);
};
private:
// Private destructor, to discourage deletion outside of Release():
- ~TextComposition()
- {
- // WARNING: mPresContext may be destroying, so, be careful if you touch it.
- }
+ ~TextComposition();
// sHandlingSelectionEvent is true while TextComposition sends a selection
// event to ContentEventHandler.
static bool sHandlingSelectionEvent;
// This class holds nsPresContext weak. This instance shouldn't block
// destroying it. When the presContext is being destroyed, it's notified to
// IMEStateManager::OnDestroyPresContext(), and then, it destroy
@@ -257,31 +254,36 @@ private:
// both composition string and data attribute of compositionupdate
// and compositionend events.
bool mAllowControlCharacters;
// mWasCompositionStringEmpty is true if the composition string was empty
// when DispatchCompositionEvent() is called.
bool mWasCompositionStringEmpty;
+ // mHasDispatchedCompositionEvents is true if the instance has dispatched
+ // one or more composition events.
+ bool mHasDispatchedCompositionEvents;
+
// Hide the default constructor and copy constructor.
TextComposition()
: mPresContext(nullptr)
, mNativeContext(nullptr)
, mCompositionStartOffset(0)
, mTargetClauseOffsetInComposition(0)
, mIsSynthesizedForTests(false)
, mIsComposing(false)
, mIsEditorHandlingEvent(false)
, mIsRequestingCommit(false)
, mIsRequestingCancel(false)
, mRequestedToCommitOrCancel(false)
, mWasNativeCompositionEndEventDiscarded(false)
, mAllowControlCharacters(false)
, mWasCompositionStringEmpty(true)
+ , mHasDispatchedCompositionEvents(false)
{}
TextComposition(const TextComposition& aOther);
/**
* GetEditor() returns nsIEditor pointer of mEditorWeak.
*/
already_AddRefed<nsIEditor> GetEditor() const;
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -2062,16 +2062,22 @@ TabParent::SendPasteTransferable(const I
const bool& aIsPrivateData,
const IPC::Principal& aRequestingPrincipal)
{
return PBrowserParent::SendPasteTransferable(aDataTransfer,
aIsPrivateData,
aRequestingPrincipal);
}
+void
+TabParent::OnDestroyTextComposition()
+{
+ mContentCache.OnDestroyTextComposition();
+}
+
/*static*/ TabParent*
TabParent::GetFrom(nsFrameLoader* aFrameLoader)
{
if (!aFrameLoader) {
return nullptr;
}
PBrowserParent* remoteBrowser = aFrameLoader->GetRemoteBrowser();
return static_cast<TabParent*>(remoteBrowser);
--- a/dom/ipc/TabParent.h
+++ b/dom/ipc/TabParent.h
@@ -502,16 +502,22 @@ public:
bool SendCompositionEvent(mozilla::WidgetCompositionEvent& aEvent);
bool SendSelectionEvent(mozilla::WidgetSelectionEvent& aEvent);
bool SendPasteTransferable(const IPCDataTransfer& aDataTransfer,
const bool& aIsPrivateData,
const IPC::Principal& aRequestingPrincipal);
+ /**
+ * OnDestroyTextComposition() should be called when TextComposition instance
+ * which dispatched composition events to this instance is being destroyed.
+ */
+ void OnDestroyTextComposition();
+
static TabParent* GetFrom(nsFrameLoader* aFrameLoader);
static TabParent* GetFrom(nsIFrameLoader* aFrameLoader);
static TabParent* GetFrom(nsITabParent* aTabParent);
static TabParent* GetFrom(PBrowserParent* aTabParent);
--- a/widget/ContentCache.cpp
+++ b/widget/ContentCache.cpp
@@ -1157,32 +1157,38 @@ void
ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget,
EventMessage aMessage)
{
// This is called when the child process receives WidgetCompositionEvent or
// WidgetSelectionEvent.
MOZ_LOG(sContentCacheLog, LogLevel::Info,
("0x%p OnEventNeedingAckHandled(aWidget=0x%p, "
- "aMessage=%s), mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
- this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck, mPendingCompositionCount));
-
- if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage)) {
- MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
- mPendingCompositionCount--;
- }
+ "aMessage=%s), mPendingEventsNeedingAck=%u",
+ this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck));
MOZ_RELEASE_ASSERT(mPendingEventsNeedingAck > 0);
if (--mPendingEventsNeedingAck) {
return;
}
FlushPendingNotifications(aWidget);
}
+void
+ContentCacheInParent::OnDestroyTextComposition()
+{
+ MOZ_LOG(sContentCacheLog, LogLevel::Info,
+ ("0x%p OnDestroyTextComposition(), "
+ "mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
+ this, mPendingEventsNeedingAck, mPendingCompositionCount));
+ MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
+ mPendingCompositionCount--;
+}
+
bool
ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
bool aCancel,
nsAString& aCommittedString)
{
MOZ_LOG(sContentCacheLog, LogLevel::Info,
("0x%p RequestToCommitComposition(aWidget=%p, "
"aCancel=%s), mWidgetHasComposition=%s, mCommitStringByRequest=%p",
--- a/widget/ContentCache.h
+++ b/widget/ContentCache.h
@@ -370,16 +370,23 @@ public:
*
* WARNING: This may send notifications to IME. That might cause destroying
* TabParent or aWidget. Therefore, the caller must not destroy
* this instance during a call of this method.
*/
void OnEventNeedingAckHandled(nsIWidget* aWidget, EventMessage aMessage);
/**
+ * OnDestroyTextComposition() should be called when TextComposition instance
+ * which dispatched composition events to the owner of this instance is being
+ * destroyed.
+ */
+ void OnDestroyTextComposition();
+
+ /**
* RequestIMEToCommitComposition() requests aWidget to commit or cancel
* composition. If it's handled synchronously, this returns true.
*
* @param aWidget The widget to be requested to commit or cancel
* the composition.
* @param aCancel When the caller tries to cancel the composition, true.
* Otherwise, i.e., tries to commit the composition, false.
* @param aCommittedString The committed string (i.e., the last data of