Commit 5f53d66d authored by Masayuki Nakano's avatar Masayuki Nakano
Browse files

Bug 1825693 - Make `PuppetWidget` stop trying to cache `Selection` directly r=m_kato 

Currently, `PuppetWidget` calls `ContentCacheInChild::CacheSelection` directly.
However, `mText` can be `Nothing` or `mText` can be outdated, but `mSelection`
becomes the latest one.  Therefore, we may notify the parent process with
invalid data combination.

The callers in `PuppetWidget` are:
1. `NotifyIMEOfCompositionUpdate`
2. `NotifyIMEOfPositionChange`

I think that the former does not need to cache anything here because
`IMEContentObserver` should've updated the text/selection changes.  However,
stopping caching everything at this point is risky.  In the most cases, outdated
data appears as odd IME UI position.  Therefore, let's keep updating only caret
and text rectangles.

The latter is reported with new crash reports which is crashed by a
`MOZ_DIAGNOSTIC_ASSERT` failure added by the previous patch.  In the case,
if `mText` and `mSelection` has not been cached, we don't need to notify
the parent process with them because they will be sent later.  And also even
if they are not available, it may be useful that the character rectangles not
related to `Selection` (e.g., the first character rect).  Therefore, let's
keep caching same things as the former case.

Therefore, this patch makes `CacheSelection` a private method and add
`CacheCaretAndTextRects` for them.

Differential Revision: https://phabricator.services.mozilla.com/D178145
parent 4c5f7cf6

widget/ContentCache.cpp

+17 −5
Original line number Diff line number Diff line
@@ -195,10 +195,8 @@ bool ContentCacheInChild::CacheSelection(nsIWidget* aWidget, 
    mSelection.emplace(querySelectedTextEvent);

  }



  const bool caretCached = CacheCaret(aWidget, aNotification);

  const bool textRectsCached = CacheTextRects(aWidget, aNotification);

  MOZ_DIAGNOSTIC_ASSERT(IsValid());

  return caretCached || textRectsCached || querySelectedTextEvent.Succeeded();

  return CacheCaretAndTextRects(aWidget, aNotification) ||

         querySelectedTextEvent.Succeeded();

}



bool ContentCacheInChild::CacheCaret(nsIWidget* aWidget,
@@ -270,6 +268,19 @@ bool ContentCacheInChild::CacheEditorRect( 
  return true;

}



bool ContentCacheInChild::CacheCaretAndTextRects(

    nsIWidget* aWidget, const IMENotification* aNotification) {

  MOZ_LOG(sContentCacheLog, LogLevel::Info,

          ("0x%p CacheCaretAndTextRects(aWidget=0x%p, aNotification=%s)", this,

           aWidget, GetNotificationName(aNotification)));



  const bool caretCached =

      mSelection.isSome() && CacheCaret(aWidget, aNotification);

  const bool textRectsCached = CacheTextRects(aWidget, aNotification);

  MOZ_DIAGNOSTIC_ASSERT(IsValid());

  return caretCached || textRectsCached;

}



bool ContentCacheInChild::CacheText(nsIWidget* aWidget,

                                    const IMENotification* aNotification) {

  MOZ_LOG(sContentCacheLog, LogLevel::Info,
@@ -941,7 +952,8 @@ bool ContentCacheInParent::HandleQueryContentEvent( 
                                            textInQueriedRange,

                                            OffsetAndDataFor::EditorString);

      // XXX This may be wrong if storing range isn't in the selection range.

      aEvent.mReply->mWritingMode = mSelection->mWritingMode;

      aEvent.mReply->mWritingMode =

          mSelection.isSome() ? mSelection->mWritingMode : WritingMode();

      MOZ_LOG(sContentCacheLog, LogLevel::Info,

              ("0x%p   HandleQueryContentEvent(), Succeeded, aEvent={ "

               "mMessage=eQueryTextRect mReply=%s }",

widget/ContentCache.h

+8 −4
Original line number Diff line number Diff line
@@ -327,12 +327,14 @@ class ContentCacheInChild final : public ContentCache { 


  /**

   * Cache*() retrieves the latest content information and store them.

   * Be aware, CacheSelection() calls CacheTextRects(), and also CacheText()

   * calls CacheSelection().  So, related data is also retrieved automatically.

   * Be aware, CacheSelection() calls CacheCaretAndTextRects(),

   * CacheCaretAndTextRects() calls CacheCaret() and CacheTextRects(), and

   * CacheText() calls CacheSelection().  So, related data is also retrieved

   * automatically.

   */

  bool CacheEditorRect(nsIWidget* aWidget,

                       const IMENotification* aNotification = nullptr);

  bool CacheSelection(nsIWidget* aWidget,

  bool CacheCaretAndTextRects(nsIWidget* aWidget,

                              const IMENotification* aNotification = nullptr);

  bool CacheText(nsIWidget* aWidget,

                 const IMENotification* aNotification = nullptr);
@@ -355,6 +357,8 @@ class ContentCacheInChild final : public ContentCache { 
                     LayoutDeviceIntRect& aCharRect) const;

  bool QueryCharRectArray(nsIWidget* aWidget, uint32_t aOffset,

                          uint32_t aLength, RectArray& aCharRectArray) const;

  bool CacheSelection(nsIWidget* aWidget,

                      const IMENotification* aNotification = nullptr);

  bool CacheCaret(nsIWidget* aWidget,

                  const IMENotification* aNotification = nullptr);

  bool CacheTextRects(nsIWidget* aWidget,

widget/PuppetWidget.cpp

+4 −2
Original line number Diff line number Diff line
@@ -786,7 +786,8 @@ nsresult PuppetWidget::NotifyIMEOfCompositionUpdate( 
    return NS_ERROR_FAILURE;

  }



  if (NS_WARN_IF(!mContentCache.CacheSelection(this, &aIMENotification))) {

  if (NS_WARN_IF(

          !mContentCache.CacheCaretAndTextRects(this, &aIMENotification))) {

    return NS_ERROR_FAILURE;

  }

  mBrowserChild->SendNotifyIMECompositionUpdate(mContentCache,
@@ -872,7 +873,8 @@ nsresult PuppetWidget::NotifyIMEOfPositionChange( 
  if (NS_WARN_IF(!mContentCache.CacheEditorRect(this, &aIMENotification))) {

    return NS_ERROR_FAILURE;

  }

  if (NS_WARN_IF(!mContentCache.CacheSelection(this, &aIMENotification))) {

  if (NS_WARN_IF(

          !mContentCache.CacheCaretAndTextRects(this, &aIMENotification))) {

    return NS_ERROR_FAILURE;

  }

  if (mIMENotificationRequestsOfParent.WantPositionChanged()) {