1166436 - crash in mozilla::dom::TabParent::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent&)

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-7cbb9804-ad13-47ed-ad8a-d409a2150516.
=============================================================

New top crash on nightly starting in build 20150515084717.

This happens on Windows 64-bit builds only. It looks like mIMECompositionRects is a bad pointer. 

User comments:
"I turned on IME and input Japanese charancters, then Firefox crashed."
"site: Google+"
"When I tried to input message into text box, then crashed."

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=127a78bac3f1&tochange=1a8343f8ed83

I assume bug 1130935.

Comment 1

[(Away)]

[Tracking Requested - why for this release]: New topcrash on 41

Comment 2

[(Away)]

> It looks like mIMECompositionRects is a bad pointer. 
Or maybe "i" is bad?

Comment 3

[(Away)]

      if (aEvent.mInput.mOffset < mIMECompositionRectOffset ||
          (aEvent.mInput.mOffset + aEvent.mInput.mLength >
            mIMECompositionRectOffset + mIMECompositionRects.Length())) {
        // XXX
        // we doesn't have cache for this request.
        break;
      }

Should the ">" be ">=" ?

Comment 4

[(Away)]

In the minidumps, "i" is something like 0xfffffff7. Since this happens only on Win64, maybe there is an overflow/underflow somewhere?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Major [:dmajor] from comment #3)
>       if (aEvent.mInput.mOffset < mIMECompositionRectOffset ||
>           (aEvent.mInput.mOffset + aEvent.mInput.mLength >
>             mIMECompositionRectOffset + mIMECompositionRects.Length())) {
>         // XXX
>         // we doesn't have cache for this request.
>         break;
>       }
> 
> Should the ">" be ">=" ?

I don't think so... It compares end offsets.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I built x64 debug build locally, but I cannot reproduce this... We need STR for this.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

I reproduced this problem last night.

Environment:
 Mac Book Air(OS X 10.10.3 Yosemite)
 Firefox Nightly 41.0a1(2015-05-20)

Reproduce step:
 1) open MDN edit page.[1]
 2) input Japanese charancters.

[1] https://developer.mozilla.org/ja/docs/JavaScript/Reference/Operators/yield

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracking for FF41 as it's a top crash (#4) at the moment.

Comment 9

[(Away)]

Does STR from comment 7 work for you?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I could work on this bug soon (I'm still working on another crash bug).

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Got it. aEvent.mInput.mOffset is sometimes -1.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I still don't get a good STR. Looks like that this is a bug of the interface between PuppetWidget and TabParent. PuppetWidget updates cache in TabParent with two or more IPC calls. Therefore, TabParent may have incomplete information in its cache. So, we need to send all information once.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So, I meant that it's difficult to test it after creating the patch if my idea is right.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I believe that we need big change for fixing this completely. However, if this should be fixed in the branches, i.e., we need to uplift the fix, I'll create a wallpaper to avoid the crash.

Comment 15

[Makoto Kato [:m_kato]]

mIMECompositonRects is empty.  We should check whether it is empty.

0:000> dt -r xul!nsTArray<mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel>
 > 00000028`9f33fc00+0x1a8
   +0x000 mHdr             : 0x00007ff9`5d93dcb0 nsTArrayHeader
      =00007ff9`5d93dcb0 sEmptyHdr        : nsTArrayHeader
         =00007ff9`5d93dcb0 sEmptyHdr        : nsTArrayHeader
         +0x000 mLength          : 0
         +0x004 mCapacity        : 0y0000000000000000000000000000000 (0)
         +0x004 mIsAutoArray     : 0y0
      +0x000 mLength          : 0
      +0x004 mCapacity        : 0y0000000000000000000000000000000 (0)
      +0x004 mIsAutoArray     : 0y0
   =00007ff9`5af00000 NoIndex          : 0x00000003`00905a4d

Comment 16

[Makoto Kato [:m_kato]]

Another issue that it is related to this.

When getting focus on PuppetWidget on content process, we sends UINT32_MAX as mOffset to chrome process.  If you need update selection, we should query it correctly then we have to call NotifyIMEOfSelecitonChange.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, that looks like different reason when I reproduce this crash. When I reproduce this, the array isn't empty but mInput.mOffset is wrong because selection is collapsed to 0, but start offset of the cached rects are collect offset (larger than 0).

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Anyway, both cases can be explained. If IME tries to query content while PuppetWidget is modifying TabParent's content cache. I believe that PuppetWidget should update all content cache with a call of method. I'm trying to separate content cache to a new class. Then, PuppetWidget can cache all information too. Then, PuppetWidget should send its cache to TabParent after modifying a part of the cache.

Comment 19

[Makoto Kato [:m_kato]]

I think that this is easy reproduce step.

- Env
e10s with IMM32 on Windows 8.1

- Step
1. Open about:home
2. Input "aa" into editbox
3. Press left arrow.  Then, caret position is between a and a.
4. Turn on IME
5. Input "あ” by pressing 'a'

Comment 20

[leichixian]

Not only happens with Japanese, on this site http://l10n.mozest.org/zh_CN/firefox-central/translate/ , when I try to input Chinese character, firefox have chance to crash. I think it's an extensive bug with all users with e10s on and IME enabled. Need to be fixed soon.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

What are you asking me? I'm working on this bug now but there are some bugs to fix this. Note that this isn't a recent regression, actually, there are some reports from Mac.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create mozilla::ContentCache and TabParent should store the text to it

First of all, we should create a capsule to store content process's information. I named it mozilla::ContentCache. First, it should be used in TabParent instead of it has a lot of members.

Note that WidgetUtils.cpp part is an existing bug. I'm not sure why it doesn't cause bustage, but |using namespace widget;| in ContentCache.cpp makes it bustage.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 mozilla::ContentCache should store a selection range and TabParent should use it

Let's move selection information into ContentCache.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 mozilla::ContentCache should store active composition information and TabParent should use them

Let's move the complicated "request to commit/cancel" code from TabParent to ContentCache.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 mozilla::ContentCache should store text rects and caret rect and TabParent should use them

Let's move caret information and text rects information to ContentCache.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 mContentCache should store writing mode at selection and TabParent should use it

Let's move the writing mode information of selected range to ContentCache.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 mContentCache should store editor rect and TabParent should use it

Let's move editor rect to ContentCache.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 mozilla::ContentCache should handle WidgetQueryContentEvent

Let's move WidgetQueryContentEventHandler. This makes a lot of public methods of ContentCache gone later.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Make mozilla::ContentCache available in IPC

Let's make ContentCache IPC-aware. Only the members should be copied from child process.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 PuppetWidget should have mozilla::ContentCache and send it to TabParent

PuppetWidget should cache the content into its mContentCache with Cache*() methods. Then, it should be sent to TabParent at every notification to IME.

With this approach, PuppetWidget can send all content date with a notification. This is safer because if it's not copied once, IME might query content (cache) before all data isn't modified yet.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 Optimize IME notification handling in PuppetWidget

Now, we see some redundant communication at IME notification. PuppetWidget should send notification to TabParent only once for a notification to IME since it can send all content data with it.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 Remove unnecessary public methods of mozilla::ContentCache

Now, the design of ContentCache is almost fixed. Let's remove or hide unnecessary public methods.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.12 Log the behavior of mozilla::ContentCache

Let's log the behavior! This must help us!!

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 mozilla::ContentCache should guess caret offset if the offset is in the range of cached text rects

This is an improvement.

First, we currently store caret position as target clause offset if there is composition. However, this is different behavior from non-e10s mode.

IME composition string is selected by a couple of IME selections for separating to multiple clauses and/or changes the visual style. But the caret is the "normal selection". So, caret position should be always anchor or focus of the selection.

And also when caret position is queried, we should guess the caret rect if the offset's text rect is in the cache.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.14 Store text rects after focus/anchor of selection

Let's cache the text rect at selection anchor and focus too. These points may be queried by IME.

Then, the order of calls of Cache*() becomes more important. So, we should guarantee the order by calling other Cache*() methods from Cache*().

CacheText(public) -> CacheSelection(public) -> CacheCaret(private) -> CacheTextRects(private)

SetSelection(public) -> CacheCaret(private) -> CacheTextRects(private)

CacheEditorRect(public)

CacheAll(public) -> CacheText()... -> CacheEditorRect().

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.15 ContentCache::CacheTextRects() should refer TextComposition::LastData() because TextComposition::String() may not be modified yet when it's called

I realized this bug.

CacheTextRects() calls TextComposition::String() but it may not be modified yet if it's called from one of editor observers (i.e., IMEContentObserver).

In such case, TextComposition::LastData() is the expected value which is modified when dispatching NS_COMPOSITION_CHANGE event from TextComposition. (TextComposition::String() is modified after all editor observers are performed)

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.16 Make each member of mozilla::ContentCache used by chrome process or content process

ContentCache is now a little bit complicated. Some members are chrome process only, others are content only and the others are available in both processes.

Let's restrict it with mIsChrome flag in each method. Then, we can prevent to land buggy patch.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.17 ContentCache::HandleQueryContentEvent() should return caret rect if mInput.mLength of NS_QUERY_TEXT_RECT event is 0

NS_QUERY_TEXT_RECT event's mInput.mLength may be 0. In such case, we should return caret rect instead of empty rect.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.18 ContentCache should guess caret rect if there is a text rect cache of the previous character

I see a lot of failure of NS_QUERY_CARET_RECT which is next of the composition string. So, if GetCaretRect() cannot find a text rect at the offset, it should guess the result from its previous character's rect if there is it.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

That's all. I think that, we can make ContentCache smarter, but for now, we should land this as far as possible for x64 testers.

And the actual cause of the crash is, mInput.mOffset is sometimes UINT32_MAX which is larger than mIMECompositionRectOffset and mInput.mOffset + mInput.mLength (only when > 0) is less than mIMECompositionRectOffset + mIMECompositionRects.Length() due to overflown! Therefore,

> 2268   case NS_QUERY_TEXT_RECT:
> 2269     {
> 2270       if (aEvent.mInput.mOffset < mIMECompositionRectOffset ||
> 2271           (aEvent.mInput.mOffset + aEvent.mInput.mLength >
> 2272             mIMECompositionRectOffset + mIMECompositionRects.Length())) {
> 2273         // XXX
> 2274         // we doesn't have cache for this request.
> 2275         break;
> 2276       }

This check won't work. I'm not sure why this happens only on x64 build, though. And I found this with the log (part.12).

Comment 41

[Makoto Kato [:m_kato]]

Comment on attachment 8615314 [details] [diff] [review]
part.4 mozilla::ContentCache should store text rects and caret rect and TabParent should use them

Review of attachment 8615314 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/ContentCache.cpp
@@ +97,5 @@
> +bool
> +ContentCache::GetCaretRect(uint32_t aOffset,
> +                           LayoutDeviceIntRect& aCaretRect) const
> +{
> +  if (mCaret.mOffset != aOffset) {

Also, if mCaret.mOffset is UINT32_MAX, should we return false?

::: widget/ContentCache.h
@@ +159,5 @@
> +    }
> +
> +    uint32_t StartOffset() const
> +    {
> +      NS_WARN_IF(mStart == UINT32_MAX);

We shouldn't use StartOffset() with mStart is UINT32_MAX.  So we should use NS_ASSERTION instead of NS_WARN_IF.

Comment 42

[Makoto Kato [:m_kato]]

Comment on attachment 8615309 [details] [diff] [review]
part.2 mozilla::ContentCache should store a selection range and TabParent should use it

Review of attachment 8615309 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/ContentCache.cpp
@@ +24,5 @@
> +void
> +ContentCache::SetSelection(uint32_t aAnchorOffset, uint32_t aFocusOffset)
> +{
> +  mSelection.mAnchor = aAnchorOffset;
> +  mSelection.mFocus = aFocusOffset;

Both values shouldn't set UINT32_MAX.  We should add assertion or warning when aAnchorOffset or aFocusOffset is UINT32_MAX

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I see, but instead of modifying the patch, I'll add a new patch for making invalid state of mSelection due to it's very early patch of the series.

Comment 44

[Makoto Kato [:m_kato]]

Comment on attachment 8615320 [details] [diff] [review]
part.7 mozilla::ContentCache should handle WidgetQueryContentEvent

Review of attachment 8615320 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/ContentCache.cpp
@@ +41,5 @@
> +  aEvent.mWasAsync = false;
> +  aEvent.mReply.mFocusedWidget = aWidget;
> +
> +  switch (aEvent.message) {
> +    case NS_QUERY_SELECTED_TEXT:

When NS_QUERY_SELECTED_TEXT on non-e10s cause, can it return true if selection is invalid??

If not so, when mSelection.StartOffset() returns UINT32_MAX, we should return false. In the other word, we should add like IsValid() method in Selection struct, then this method should check StartOffset is UINT32_MAX.

::: widget/TextEvents.h
@@ +541,5 @@
>    bool mWithFontRanges;
>    struct
>    {
>      uint32_t EndOffset() const
>      {

Also should we add NS_WARN_IF(mOffset == UINT32_MAX)?  If no situation, please ignore.

Comment 45

[Makoto Kato [:m_kato]]

Comment on attachment 8615329 [details] [diff] [review]
part.11 Remove unnecessary public methods of mozilla::ContentCache

Review of attachment 8615329 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/ContentCache.cpp
@@ +208,5 @@
>    WidgetQueryContentEvent queryText(true, NS_QUERY_TEXT_CONTENT, aWidget);
>    queryText.InitForQueryTextContent(0, UINT32_MAX);
>    aWidget->DispatchEvent(&queryText, status);
>    if (NS_WARN_IF(!queryText.mSucceeded)) {
> +    mText.Truncate(0);

nit: mText.Trucate();

Comment 46

[Makoto Kato [:m_kato]]

Comment on attachment 8615330 [details] [diff] [review]
part.12 Log the behavior of mozilla::ContentCache

Review of attachment 8615330 [details] [diff] [review]:
-----------------------------------------------------------------

By bug 1165515, logging code is replaced.  This code causes bustage.

Comment 47

[Makoto Kato [:m_kato]]

Comment on attachment 8615342 [details] [diff] [review]
part.17 ContentCache::HandleQueryContentEvent() should return caret rect if mInput.mLength of NS_QUERY_TEXT_RECT event is 0

Review of attachment 8615342 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/ContentCache.cpp
@@ +262,5 @@
> +        if (NS_WARN_IF(!GetUnionTextRects(aEvent.mInput.mOffset,
> +                                          aEvent.mInput.mLength,
> +                                          aEvent.mReply.mRect))) {
> +          // XXX We don't have cache for this request.
> +          MOZ_LOG(sContentCacheLog, PR_LOG_ERROR,

Replace 2nd param with LogLevel::Error

@@ +272,5 @@
> +      } else {
> +        // If the length is 0, we should return caret rect instead.
> +        if (NS_WARN_IF(!GetCaretRect(aEvent.mInput.mOffset,
> +                                     aEvent.mReply.mRect))) {
> +          MOZ_LOG(sContentCacheLog, PR_LOG_ERROR,

Replace 2nd param with LogLevel::Error

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.12 Log the behavior of mozilla::ContentCache

Thank you for the quick review!

Here is the new logging code with LogLevel.

# I'll post part.19 which includes the fix of part.2 and part.7.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.19 ContentCache::mSelection should have invalid state and it should be checked

This fixes your comment for part.2 and part.7.

And I include CheckedInt which I was advised by ehsan.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.19 ContentCache::mSelection should have invalid state and it should be checked

Oops, this is right patch.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you review part.7 again?

Comment 52

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #51)
> Could you review part.7 again?

reviewed.  I confirm Part 19 fix

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you very much!! I'll land it after I confirm part.19 doesn't cause new assertion in automated tests.

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7082d6cfb371
https://hg.mozilla.org/integration/mozilla-inbound/rev/c152e396e9c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b5f272f88a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ef3cb7f74
https://hg.mozilla.org/integration/mozilla-inbound/rev/a33f46c01ae8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5d3ee81d51
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f252ed1996
https://hg.mozilla.org/integration/mozilla-inbound/rev/7acbb10cf6d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30faf72451e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d813014e797c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8afeac76c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee382b70183
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f5a8d204d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/27bf218dee2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9201883c30
https://hg.mozilla.org/integration/mozilla-inbound/rev/d54622600414
https://hg.mozilla.org/integration/mozilla-inbound/rev/8feaf4e01d6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe725dba239
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaeacbb9700

Comment 55

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7082d6cfb371
https://hg.mozilla.org/mozilla-central/rev/c152e396e9c3
https://hg.mozilla.org/mozilla-central/rev/a4b5f272f88a
https://hg.mozilla.org/mozilla-central/rev/d64ef3cb7f74
https://hg.mozilla.org/mozilla-central/rev/a33f46c01ae8
https://hg.mozilla.org/mozilla-central/rev/cc5d3ee81d51
https://hg.mozilla.org/mozilla-central/rev/73f252ed1996
https://hg.mozilla.org/mozilla-central/rev/7acbb10cf6d6
https://hg.mozilla.org/mozilla-central/rev/f30faf72451e
https://hg.mozilla.org/mozilla-central/rev/d813014e797c
https://hg.mozilla.org/mozilla-central/rev/c8afeac76c0e
https://hg.mozilla.org/mozilla-central/rev/6ee382b70183
https://hg.mozilla.org/mozilla-central/rev/d5f5a8d204d1
https://hg.mozilla.org/mozilla-central/rev/27bf218dee2b
https://hg.mozilla.org/mozilla-central/rev/cc9201883c30
https://hg.mozilla.org/mozilla-central/rev/d54622600414
https://hg.mozilla.org/mozilla-central/rev/8feaf4e01d6a
https://hg.mozilla.org/mozilla-central/rev/6fe725dba239
https://hg.mozilla.org/mozilla-central/rev/cbaeacbb9700

Comment 56

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows zero crashes over the past 4 weeks in builds after the fix landed.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=mozilla%3A%3Adom%3A%3ATabParent%3A%3AHandleQueryContentEvent%28mozilla%3A%3AWidgetQueryContentEvent%26%29#tab-reports