Bug 599550 - Properly sync PuppetWidget IME notifications; r=blassey,cjones a=blocking-fennec
authorJim Chen <jimnchen@gmail.com>
Fri, 01 Oct 2010 10:17:37 -0400
changeset 54871 ba32daebb14a00700e8b5ab2ed7d89dccb95a6fd
parent 54870 5f0370936fc9c5d9b9bae32a5fe72df2007f709f
child 54872 c5c3d5c78727860992df3a1aa5f4f8eda4c6d84c
push idunknown
push userunknown
push dateunknown
reviewersblassey, cjones, blocking-fennec
bugs599550
milestone2.0b7pre
Bug 599550 - Properly sync PuppetWidget IME notifications; r=blassey,cjones a=blocking-fennec
dom/ipc/PBrowser.ipdl
dom/ipc/TabParent.cpp
dom/ipc/TabParent.h
widget/public/nsGUIEvent.h
widget/public/nsGUIEventIPC.h
widget/src/xpwidgets/PuppetWidget.cpp
widget/src/xpwidgets/PuppetWidget.h
--- a/dom/ipc/PBrowser.ipdl
+++ b/dom/ipc/PBrowser.ipdl
@@ -87,25 +87,46 @@ parent:
     Event(RemoteDOMEvent aEvent);
 
     rpc CreateWindow() returns (PBrowser window);
 
     sync SyncMessage(nsString aMessage, nsString aJSON)
       returns (nsString[] retval);
 
     /**
+     * The IME sequence number (seqno) parameter is used to make sure
+     * that a notification is discarded if it arrives at the chrome process
+     * too late. If the notification is late and we accept it, we will have
+     * an out-of-date view of the content process, which means events that we
+     * dispatch based on this out-of-date view will be wrong also.
+     * (see Bug 599550 and Bug 591047 comments 44, 50, and 54)
+     *
+     * Chrome increments seqno and includes it in each IME event sent to
+     * content, and content sends its current seqno back to chrome with each
+     * notification. A notification is up-to-date only if the content
+     * seqno is the same as the current chrome seqno, meaning no additional
+     * event was sent to content before the notification was received
+     *
+     * On blur, chrome returns the current seqno to content, and content
+     * uses it to discard subsequent events until the content seqno and
+     * chrome seqno-on-blur match again. These events, meant for the blurred
+     * textfield, are discarded to prevent events going to the wrong target
+     */
+
+    /**
      * Notifies chrome that there is a focus change involving an editable
      * object (input, textarea, document, contentEditable. etc.)
      *
      *  focus        PR_TRUE if editable object is receiving focus
      *               PR_FALSE if losing focus
      *  preference   Native widget preference for IME updates
+     *  seqno        Current seqno value on the chrome side
      */
     sync NotifyIMEFocus(PRBool focus)
-      returns (nsIMEUpdatePreference preference);
+      returns (nsIMEUpdatePreference preference, PRUint32 seqno);
 
     /**
      * Notifies chrome that there has been a change in text content
      * One call can encompass both a delete and an insert operation
      * Only called when NotifyIMEFocus returns PR_TRUE for mWantUpdates
      *
      *  offset       Starting offset of the change
      *  end          Ending offset of the range deleted
@@ -115,20 +136,21 @@ parent:
      *  for deletion, offset == newEnd
      */
     NotifyIMETextChange(PRUint32 offset, PRUint32 end, PRUint32 newEnd);
 
     /**
      * Notifies chrome that there has been a change in selection
      * Only called when NotifyIMEFocus returns PR_TRUE for mWantUpdates
      *
+     *  seqno        Current seqno value on the content side
      *  anchor       Offset where the selection started
      *  focus        Offset where the caret is
      */
-    NotifyIMESelection(PRUint32 anchor, PRUint32 focus);
+    NotifyIMESelection(PRUint32 seqno, PRUint32 anchor, PRUint32 focus);
 
     /**
      * Notifies chrome to refresh its text cache 
      * Only called when NotifyIMEFocus returns PR_TRUE for mWantHints
      *
      *  text         The entire content of the text field
      */
     NotifyIMETextHint(nsString text);
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -312,22 +312,24 @@ bool
 TabParent::RecvAsyncMessage(const nsString& aMessage,
                             const nsString& aJSON)
 {
   return ReceiveMessage(aMessage, PR_FALSE, aJSON, nsnull);
 }
 
 bool
 TabParent::RecvNotifyIMEFocus(const PRBool& aFocus,
-                              nsIMEUpdatePreference* aPreference)
+                              nsIMEUpdatePreference* aPreference,
+                              PRUint32* aSeqno)
 {
   nsCOMPtr<nsIWidget> widget = GetWidget();
   if (!widget)
     return true;
 
+  *aSeqno = mIMESeqno;
   mIMETabParent = aFocus ? this : nsnull;
   mIMESelectionAnchor = 0;
   mIMESelectionFocus = 0;
   nsresult rv = widget->OnIMEFocusChange(aFocus);
 
   if (aFocus) {
     if (NS_SUCCEEDED(rv) && rv != NS_SUCCESS_IME_NO_UPDATES) {
       *aPreference = widget->GetIMEUpdatePreference();
@@ -350,26 +352,29 @@ TabParent::RecvNotifyIMETextChange(const
   if (!widget)
     return true;
 
   widget->OnIMETextChange(aStart, aEnd, aNewEnd);
   return true;
 }
 
 bool
-TabParent::RecvNotifyIMESelection(const PRUint32& aAnchor,
+TabParent::RecvNotifyIMESelection(const PRUint32& aSeqno,
+                                  const PRUint32& aAnchor,
                                   const PRUint32& aFocus)
 {
   nsCOMPtr<nsIWidget> widget = GetWidget();
   if (!widget)
     return true;
 
-  mIMESelectionAnchor = aAnchor;
-  mIMESelectionFocus = aFocus;
-  widget->OnIMESelectionChange();
+  if (aSeqno == mIMESeqno) {
+    mIMESelectionAnchor = aAnchor;
+    mIMESelectionFocus = aFocus;
+    widget->OnIMESelectionChange();
+  }
   return true;
 }
 
 bool
 TabParent::RecvNotifyIMETextHint(const nsString& aText)
 {
   // Replace our cache with new text
   mIMECacheText = aText;
@@ -439,56 +444,59 @@ TabParent::HandleQueryContentEvent(nsQue
       aEvent.mSucceeded = PR_TRUE;
     }
     break;
   }
   return true;
 }
 
 bool
-TabParent::SendCompositionEvent(const nsCompositionEvent& event)
+TabParent::SendCompositionEvent(nsCompositionEvent& event)
 {
   mIMEComposing = event.message == NS_COMPOSITION_START;
   mIMECompositionStart = PR_MIN(mIMESelectionAnchor, mIMESelectionFocus);
   if (mIMECompositionEnding)
     return true;
+  event.seqno = ++mIMESeqno;
   return PBrowserParent::SendCompositionEvent(event);
 }
 
 /**
  * During ResetInputState or CancelComposition, widget usually sends a
  * NS_TEXT_TEXT event to finalize or clear the composition, respectively
  *
  * Because the event will not reach content in time, we intercept it
  * here and pass the text as the EndIMEComposition return value
  */
 bool
-TabParent::SendTextEvent(const nsTextEvent& event)
+TabParent::SendTextEvent(nsTextEvent& event)
 {
   if (mIMECompositionEnding) {
     mIMECompositionText = event.theText;
     return true;
   }
 
   // We must be able to simulate the selection because
   // we might not receive selection updates in time
   if (!mIMEComposing) {
     mIMECompositionStart = PR_MIN(mIMESelectionAnchor, mIMESelectionFocus);
   }
   mIMESelectionAnchor = mIMESelectionFocus =
       mIMECompositionStart + event.theText.Length();
 
+  event.seqno = ++mIMESeqno;
   return PBrowserParent::SendTextEvent(event);
 }
 
 bool
-TabParent::SendSelectionEvent(const nsSelectionEvent& event)
+TabParent::SendSelectionEvent(nsSelectionEvent& event)
 {
   mIMESelectionAnchor = event.mOffset + (event.mReversed ? event.mLength : 0);
   mIMESelectionFocus = event.mOffset + (!event.mReversed ? event.mLength : 0);
+  event.seqno = ++mIMESeqno;
   return PBrowserParent::SendSelectionEvent(event);
 }
 
 bool
 TabParent::RecvEndIMEComposition(const PRBool& aCancel,
                                  nsString* aComposition)
 {
   nsCOMPtr<nsIWidget> widget = GetWidget();
--- a/dom/ipc/TabParent.h
+++ b/dom/ipc/TabParent.h
@@ -87,21 +87,23 @@ public:
 
     virtual bool AnswerCreateWindow(PBrowserParent** retval);
     virtual bool RecvSyncMessage(const nsString& aMessage,
                                  const nsString& aJSON,
                                  nsTArray<nsString>* aJSONRetVal);
     virtual bool RecvAsyncMessage(const nsString& aMessage,
                                   const nsString& aJSON);
     virtual bool RecvNotifyIMEFocus(const PRBool& aFocus,
-                                    nsIMEUpdatePreference* aPreference);
+                                    nsIMEUpdatePreference* aPreference,
+                                    PRUint32* aSeqno);
     virtual bool RecvNotifyIMETextChange(const PRUint32& aStart,
                                          const PRUint32& aEnd,
                                          const PRUint32& aNewEnd);
-    virtual bool RecvNotifyIMESelection(const PRUint32& aAnchor,
+    virtual bool RecvNotifyIMESelection(const PRUint32& aSeqno,
+                                        const PRUint32& aAnchor,
                                         const PRUint32& aFocus);
     virtual bool RecvNotifyIMETextHint(const nsString& aText);
     virtual bool RecvEndIMEComposition(const PRBool& aCancel,
                                        nsString* aComposition);
     virtual bool RecvGetIMEEnabled(PRUint32* aValue);
     virtual bool RecvSetIMEEnabled(const PRUint32& aValue);
     virtual bool RecvGetIMEOpenState(PRBool* aValue);
     virtual bool RecvSetIMEOpenState(const PRBool& aValue);
@@ -174,19 +176,19 @@ public:
     NS_DECL_NSIAUTHPROMPTPROVIDER
     NS_DECL_NSISECUREBROWSERUI
     NS_DECL_NSISSLSTATUSPROVIDER
 
     void HandleDelayedDialogs();
 
     static TabParent *GetIMETabParent() { return mIMETabParent; }
     bool HandleQueryContentEvent(nsQueryContentEvent& aEvent);
-    bool SendCompositionEvent(const nsCompositionEvent& event);
-    bool SendTextEvent(const nsTextEvent& event);
-    bool SendSelectionEvent(const nsSelectionEvent& event);
+    bool SendCompositionEvent(nsCompositionEvent& event);
+    bool SendTextEvent(nsTextEvent& event);
+    bool SendSelectionEvent(nsSelectionEvent& event);
 protected:
     bool ReceiveMessage(const nsString& aMessage,
                         PRBool aSync,
                         const nsString& aJSON,
                         nsTArray<nsString>* aJSONRetVal = nsnull);
 
     void ActorDestroy(ActorDestroyReason why);
 
@@ -227,16 +229,17 @@ protected:
     PRUint32 mIMESelectionAnchor;
     PRUint32 mIMESelectionFocus;
     PRPackedBool mIMEComposing;
     PRPackedBool mIMECompositionEnding;
     // Buffer to store composition text during ResetInputState
     // Compositions in almost all cases are small enough for nsAutoString
     nsAutoString mIMECompositionText;
     PRUint32 mIMECompositionStart;
+    PRUint32 mIMESeqno;
 
 private:
     already_AddRefed<nsFrameLoader> GetFrameLoader() const;
     already_AddRefed<nsIWidget> GetWidget() const;
 };
 
 } // namespace dom
 } // namespace mozilla
--- a/widget/public/nsGUIEvent.h
+++ b/widget/public/nsGUIEvent.h
@@ -1058,16 +1058,19 @@ class nsTextEvent : public nsInputEvent
 #ifdef MOZ_IPC
 private:
   friend class mozilla::dom::PBrowserParent;
   friend class mozilla::dom::PBrowserChild;
 
   nsTextEvent()
   {
   }
+
+public:
+  PRUint32 seqno;
 #endif // MOZ_IPC
 
 public:
   nsTextEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w)
     : nsInputEvent(isTrusted, msg, w, NS_TEXT_EVENT),
       rangeCount(0), rangeArray(nsnull), isChar(PR_FALSE)
   {
   }
@@ -1086,16 +1089,19 @@ class nsCompositionEvent : public nsInpu
 #ifdef MOZ_IPC
 private:
   friend class mozilla::dom::PBrowserParent;
   friend class mozilla::dom::PBrowserChild;
 
   nsCompositionEvent()
   {
   }
+
+public:
+  PRUint32 seqno;
 #endif // MOZ_IPC
 
 public:
   nsCompositionEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w)
     : nsInputEvent(isTrusted, msg, w, NS_COMPOSITION_EVENT)
   {
   }
 };
@@ -1304,16 +1310,19 @@ class nsSelectionEvent : public nsGUIEve
 #ifdef MOZ_IPC
 private:
   friend class mozilla::dom::PBrowserParent;
   friend class mozilla::dom::PBrowserChild;
 
   nsSelectionEvent()
   {
   }
+
+public:
+  PRUint32 seqno;
 #endif // MOZ_IPC
 
 public:
   nsSelectionEvent(PRBool aIsTrusted, PRUint32 aMsg, nsIWidget *aWidget) :
     nsGUIEvent(aIsTrusted, aMsg, aWidget, NS_SELECTION_EVENT),
     mExpandToClusterBoundary(PR_TRUE), mSucceeded(PR_FALSE)
   {
   }
--- a/widget/public/nsGUIEventIPC.h
+++ b/widget/public/nsGUIEventIPC.h
@@ -160,26 +160,28 @@ struct ParamTraits<nsTextRange>
 template<>
 struct ParamTraits<nsTextEvent>
 {
   typedef nsTextEvent paramType;
 
   static void Write(Message* aMsg, const paramType& aParam)
   {
     WriteParam(aMsg, static_cast<nsInputEvent>(aParam));
+    WriteParam(aMsg, aParam.seqno);
     WriteParam(aMsg, aParam.theText);
     WriteParam(aMsg, aParam.isChar);
     WriteParam(aMsg, aParam.rangeCount);
     for (PRUint32 index = 0; index < aParam.rangeCount; index++)
       WriteParam(aMsg, aParam.rangeArray[index]);
   }
 
   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
   {
     if (!ReadParam(aMsg, aIter, static_cast<nsInputEvent*>(aResult)) ||
+        !ReadParam(aMsg, aIter, &aResult->seqno) ||
         !ReadParam(aMsg, aIter, &aResult->theText) ||
         !ReadParam(aMsg, aIter, &aResult->isChar) ||
         !ReadParam(aMsg, aIter, &aResult->rangeCount))
       return false;
 
     if (!aResult->rangeCount) {
       aResult->rangeArray = nsnull;
       return true;
@@ -207,21 +209,23 @@ struct ParamTraits<nsTextEvent>
 template<>
 struct ParamTraits<nsCompositionEvent>
 {
   typedef nsCompositionEvent paramType;
 
   static void Write(Message* aMsg, const paramType& aParam)
   {
     WriteParam(aMsg, static_cast<nsInputEvent>(aParam));
+    WriteParam(aMsg, aParam.seqno);
   }
 
   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
   {
-    return ReadParam(aMsg, aIter, static_cast<nsInputEvent*>(aResult));
+    return ReadParam(aMsg, aIter, static_cast<nsInputEvent*>(aResult)) &&
+           ReadParam(aMsg, aIter, &aResult->seqno);
   }
 };
 
 template<>
 struct ParamTraits<nsQueryContentEvent>
 {
   typedef nsQueryContentEvent paramType;
 
@@ -256,26 +260,28 @@ struct ParamTraits<nsQueryContentEvent>
 template<>
 struct ParamTraits<nsSelectionEvent>
 {
   typedef nsSelectionEvent paramType;
 
   static void Write(Message* aMsg, const paramType& aParam)
   {
     WriteParam(aMsg, static_cast<nsGUIEvent>(aParam));
+    WriteParam(aMsg, aParam.seqno);
     WriteParam(aMsg, aParam.mOffset);
     WriteParam(aMsg, aParam.mLength);
     WriteParam(aMsg, aParam.mReversed);
     WriteParam(aMsg, aParam.mExpandToClusterBoundary);
     WriteParam(aMsg, aParam.mSucceeded);
   }
 
   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
   {
     return ReadParam(aMsg, aIter, static_cast<nsGUIEvent*>(aResult)) &&
+           ReadParam(aMsg, aIter, &aResult->seqno) &&
            ReadParam(aMsg, aIter, &aResult->mOffset) &&
            ReadParam(aMsg, aIter, &aResult->mLength) &&
            ReadParam(aMsg, aIter, &aResult->mReversed) &&
            ReadParam(aMsg, aIter, &aResult->mExpandToClusterBoundary) &&
            ReadParam(aMsg, aIter, &aResult->mSucceeded);
   }
 };
 
--- a/widget/src/xpwidgets/PuppetWidget.cpp
+++ b/widget/src/xpwidgets/PuppetWidget.cpp
@@ -106,17 +106,18 @@ PuppetWidget::Create(nsIWidget        *a
   mEnabled = PR_TRUE;
   mVisible = PR_TRUE;
 
   mSurface = gfxPlatform::GetPlatform()
              ->CreateOffscreenSurface(gfxIntSize(1, 1),
                                       gfxASurface::ContentFromFormat(gfxASurface::ImageFormatARGB32));
 
   mIMEComposing = PR_FALSE;
-  mIMESuppressNotifySel = PR_FALSE;
+  mIMELastReceivedSeqno = 0;
+  mIMELastBlurSeqno = 0;
 
   PuppetWidget* parent = static_cast<PuppetWidget*>(aParent);
   if (parent) {
     parent->SetChild(this);
     mLayerManager = parent->GetLayerManager();
   }
   else {
     Resize(mBounds.x, mBounds.y, mBounds.width, mBounds.height, PR_FALSE);
@@ -272,25 +273,38 @@ PuppetWidget::DispatchEvent(nsGUIEvent* 
   debug_DumpEvent(stdout, event->widget, event,
                   nsCAutoString("PuppetWidget"), nsnull);
 #endif
 
   aStatus = nsEventStatus_eIgnore;
   if (mEventCallback) {
     if (event->message == NS_COMPOSITION_START) {
       mIMEComposing = PR_TRUE;
-    } else if (event->message == NS_SELECTION_SET) {
-      mIMESuppressNotifySel = PR_TRUE;
+    }
+    switch (event->eventStructType) {
+    case NS_COMPOSITION_EVENT:
+      mIMELastReceivedSeqno = static_cast<nsCompositionEvent*>(event)->seqno;
+      if (mIMELastReceivedSeqno < mIMELastBlurSeqno)
+        return NS_OK;
+      break;
+    case NS_TEXT_EVENT:
+      mIMELastReceivedSeqno = static_cast<nsTextEvent*>(event)->seqno;
+      if (mIMELastReceivedSeqno < mIMELastBlurSeqno)
+        return NS_OK;
+      break;
+    case NS_SELECTION_EVENT:
+      mIMELastReceivedSeqno = static_cast<nsSelectionEvent*>(event)->seqno;
+      if (mIMELastReceivedSeqno < mIMELastBlurSeqno)
+        return NS_OK;
+      break;
     }
     aStatus = (*mEventCallback)(event);
 
     if (event->message == NS_COMPOSITION_END) {
       mIMEComposing = PR_FALSE;
-    } else if (event->message == NS_SELECTION_SET) {
-      mIMESuppressNotifySel = PR_FALSE;
     }
   } else if (mChild) {
     event->widget = mChild;
     mChild->DispatchEvent(event, aStatus);
   }
 
   return NS_OK;
 }
@@ -396,26 +410,29 @@ PuppetWidget::OnIMEFocusChange(PRBool aF
     if (queryEvent.mSucceeded) {
       mTabChild->SendNotifyIMETextHint(queryEvent.mReply.mString);
     }
   } else {
     // ResetInputState might not have been called yet
     ResetInputState();
   }
 
+  PRUint32 chromeSeqno;
   mIMEPreference.mWantUpdates = PR_FALSE;
   mIMEPreference.mWantHints = PR_FALSE;
-  if (!mTabChild->SendNotifyIMEFocus(aFocus, &mIMEPreference))
+  if (!mTabChild->SendNotifyIMEFocus(aFocus, &mIMEPreference, &chromeSeqno))
     return NS_ERROR_FAILURE;
 
   if (aFocus) {
     if (!mIMEPreference.mWantUpdates && !mIMEPreference.mWantHints)
       // call OnIMEFocusChange on blur but no other updates
       return NS_SUCCESS_IME_NO_UPDATES;
     OnIMESelectionChange(); // Update selection
+  } else {
+    mIMELastBlurSeqno = chromeSeqno;
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 PuppetWidget::OnIMETextChange(PRUint32 aStart, PRUint32 aEnd, PRUint32 aNewEnd)
 {
   if (!mTabChild)
@@ -439,31 +456,25 @@ PuppetWidget::OnIMETextChange(PRUint32 a
 }
 
 NS_IMETHODIMP
 PuppetWidget::OnIMESelectionChange(void)
 {
   if (!mTabChild)
     return NS_ERROR_FAILURE;
 
-  // When we send selection notifications during a composition or during a
-  // set selection event, there is a race condition where the notification
-  // arrives at chrome too late, which leads to chrome thinking the
-  // selection was elsewhere. Suppress notifications here to avoid that.
-  if (mIMEComposing || mIMESuppressNotifySel)
-    return NS_OK;
-
   if (mIMEPreference.mWantUpdates) {
     nsEventStatus status;
     nsQueryContentEvent queryEvent(PR_TRUE, NS_QUERY_SELECTED_TEXT, this);
     InitEvent(queryEvent, nsnull);
     DispatchEvent(&queryEvent, status);
 
     if (queryEvent.mSucceeded) {
-      mTabChild->SendNotifyIMESelection(queryEvent.GetSelectionStart(),
+      mTabChild->SendNotifyIMESelection(mIMELastReceivedSeqno,
+                                        queryEvent.GetSelectionStart(),
                                         queryEvent.GetSelectionEnd());
     }
   }
   return NS_OK;
 }
 
 nsresult
 PuppetWidget::DispatchPaintEvent()
--- a/widget/src/xpwidgets/PuppetWidget.h
+++ b/widget/src/xpwidgets/PuppetWidget.h
@@ -211,15 +211,21 @@ private:
   PRPackedBool mEnabled;
   PRPackedBool mVisible;
   // XXX/cjones: keeping this around until we teach LayerManager to do
   // retained-content-only transactions
   nsRefPtr<gfxASurface> mSurface;
   // IME
   nsIMEUpdatePreference mIMEPreference;
   PRPackedBool mIMEComposing;
-  PRPackedBool mIMESuppressNotifySel;
+  // Latest seqno received through events
+  PRUint32 mIMELastReceivedSeqno;
+  // Chrome's seqno value when last blur occurred
+  // arriving events with seqno up to this should be discarded
+  // Note that if seqno overflows (~50 days at 1 ms increment rate),
+  // events will be discarded until new focus/blur occurs
+  PRUint32 mIMELastBlurSeqno;
 };
 
 }  // namespace widget
 }  // namespace mozilla
 
 #endif  // mozilla_widget_PuppetWidget_h__