The assertion reason "MOZ_DIAGNOSTIC_ASSERT(IsValid())" does not help us to
debug. Make it assert with the minimum data of `ContentCache` instead.
Depends on D182652
Differential Revision: https://phabricator.services.mozilla.com/D182653
Once `PuppetWidget` sends invalid data, the parent process will return error
and that makes the sender content process crashed. Therefore, if we detect
that we created invalid data, we should not send the invalid data to avoid
the crash.
Differential Revision: https://phabricator.services.mozilla.com/D182652
E.g., when a character is inserted with a composition, i.e., without composing
state, like the Emoji picker of Windows, `BrowserParent` dispatches
`eCompositionStart`, `eCompositionChange` and `eCompositionCommit` message
within a short time. Then, if focus is moved at `compositionstart` event in
the remote message, `eCompositionChange` and `eCompositionCommit` are not yet
handled by the remote process when `BrowserParent::OnEventNeedingAckHandled()`
received a commit request handled message. Therefore, it's not invalid case
that a commit is handled without acknowledges of multiple pending composition
messages. Therefore, the assertion should be removed, and instead, for making
it easier to understand the log, let's put the information of that. Then, it's
obvious that ignored log of following `OnEventNeedingAckHandled()` calls is
valid thing.
Differential Revision: https://phabricator.services.mozilla.com/D181217
Then, we can know whether the message notification and commit request is for
proper one or outdated one, etc. Therefore, this patch makes
`OnEventNeedingAckHandled()` and `RequestIMEToCommitComposition()` ignores
unexpected and/or outdated calls. This should fix the crash reported in
bug 1466596 too.
Differential Revision: https://phabricator.services.mozilla.com/D179313
It's now not necessary. We can check all pending composition data whether
there are some composition which waits commit in the remote process.
Differential Revision: https://phabricator.services.mozilla.com/D179312
We need to make `ContentCacheInParent` store each pending composition data
for ignoring unexpected (including outdated) composition request/notifications
for protecting the main process's data, e.g., current composition.
This patch creates the place (array) and just simply store whether the
pending composition has already sent `eCompositionCommit(AsIs)` to the
remote process.
Differential Revision: https://phabricator.services.mozilla.com/D179311
For handling (ignoring) "too late" composition commit request from content
process, we need to distinguish a request is for which composition. Therefore,
we need to number each composition originated in the parent process.
This patch makes `TextComposition` instance which is created at first
composition event for a composition consider a composition ID in the parent
process and set it to composition events which are dispatched into the DOM
tree in the parent or sent to a remote process.
And also this patch adds the composition ID param to the request method of
committing composition and reply methods to notify the parent process of ending
a composition event handling.
The last patch handle them in `ContentCacheInParent` to consider
whether a request/reply should be ignored or handled.
Differential Revision: https://phabricator.services.mozilla.com/D179310
Then, we can know whether the message notification and commit request is for
proper one or outdated one, etc. Therefore, this patch makes
`OnEventNeedingAckHandled()` and `RequestIMEToCommitComposition()` ignores
unexpected and/or outdated calls. This should fix the crash reported in
bug 1466596 too.
Differential Revision: https://phabricator.services.mozilla.com/D179313
It's now not necessary. We can check all pending composition data whether
there are some composition which waits commit in the remote process.
Differential Revision: https://phabricator.services.mozilla.com/D179312
We need to make `ContentCacheInParent` store each pending composition data
for ignoring unexpected (including outdated) composition request/notifications
for protecting the main process's data, e.g., current composition.
This patch creates the place (array) and just simply store whether the
pending composition has already sent `eCompositionCommit(AsIs)` to the
remote process.
Differential Revision: https://phabricator.services.mozilla.com/D179311
For handling (ignoring) "too late" composition commit request from content
process, we need to distinguish a request is for which composition. Therefore,
we need to number each composition originated in the parent process.
This patch makes `TextComposition` instance which is created at first
composition event for a composition consider a composition ID in the parent
process and set it to composition events which are dispatched into the DOM
tree in the parent or sent to a remote process.
And also this patch adds the composition ID param to the request method of
committing composition and reply methods to notify the parent process of ending
a composition event handling.
The last patch handle them in `ContentCacheInParent` to consider
whether a request/reply should be ignored or handled.
Differential Revision: https://phabricator.services.mozilla.com/D179310
It creates no range `Selection` if `mSelection` has not been emplaced.
However, we don't want `Selection` in the case because `mText` may be nothing
and that violates the dependency. Therefore, it should stop creating
`Selection` in the case.
Note that first character rect will be cached later even if there is no
`Selection`. However, this should not occur in usual case because `focus`
notification should've already initialized `mText` and `mSelection`.
Differential Revision: https://phabricator.services.mozilla.com/D178731
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
There are some crash reports crashed in TSF module which may be caused by
passing invalid selection range (e.g., out of bounds of text). However,
the cache is created in the child process and that causes the invalid cache
creation does not appear in the crash reports. Therefore, let's try to
crash as soon as possible if `ContentCache` has invalid data.
Note that this does not detect all of the invalid cases because it's hard to
(re-)understand the edge cases. Therefore, this tries to detect the cases
checked in `ContentCacheInParent::HandleQueryContentEvent` (*1) and some other
obvious odd cases.
1. https://searchfox.org/mozilla-central/rev/0ffaecaa075887ab07bf4c607c61ea2faa81b172/widget/ContentCache.cpp#776-778
Differential Revision: https://phabricator.services.mozilla.com/D176747
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.
Depends on D176747
Differential Revision: https://phabricator.services.mozilla.com/D178145
There are some crash reports crashed in TSF module which may be caused by
passing invalid selection range (e.g., out of bounds of text). However,
the cache is created in the child process and that causes the invalid cache
creation does not appear in the crash reports. Therefore, let's try to
crash as soon as possible if `ContentCache` has invalid data.
Note that this does not detect all of the invalid cases because it's hard to
(re-)understand the edge cases. Therefore, this tries to detect the cases
checked in `ContentCacheInParent::HandleQueryContentEvent` (*1) and some other
obvious odd cases.
1. https://searchfox.org/mozilla-central/rev/0ffaecaa075887ab07bf4c607c61ea2faa81b172/widget/ContentCache.cpp#776-778
Differential Revision: https://phabricator.services.mozilla.com/D176747
There are some crash reports crashed in TSF module which may be caused by
passing invalid selection range (e.g., out of bounds of text). However,
the cache is created in the child process and that causes the invalid cache
creation does not appear in the crash reports. Therefore, let's try to
crash as soon as possible if `ContentCache` has invalid data.
Note that this does not detect all of the invalid cases because it's hard to
(re-)understand the edge cases. Therefore, this tries to detect the cases
checked in `ContentCacheInParent::HandleQueryContentEvent` (*1) and some other
obvious odd cases.
1. https://searchfox.org/mozilla-central/rev/0ffaecaa075887ab07bf4c607c61ea2faa81b172/widget/ContentCache.cpp#776-778
Differential Revision: https://phabricator.services.mozilla.com/D176747
It's designed for caching content information of focused editor. However, at
sending focus notification to the main process, the editor may have already
been blurred but `IMEContentObserver` may have not known it yet. In this edge
case, only query selection succeeds since IMEContentObserver still has a cache,
but query the others failed because of root content node check of
`IMEContentObserver::HandleQueryContentEvent`. If a content process meets this
case, it should not send focus notification and stop storing the content since
IME shouldn't get focus nor query non-editable content.
On the other hand, the reported testcase reproduces this with a fuzzing API
called **in** the content process. Therefore, I have no idea how to reproduce
it without the API. That's the reason why this patch does not contain new tests.
Differential Revision: https://phabricator.services.mozilla.com/D141821
Unfortunately, the adding test cannot reproduce the crash, but I have no idea
how to make `mSelection` being `Nothing` until next native event loop. Anyway,
it's worthwhile to add new tests.
This patch just avoids `ContentCacheInParent::GetTextRect` referring
`mSelection` when there is no text rects and `mSelection` is `Nothing`.
Differential Revision: https://phabricator.services.mozilla.com/D138876
Oddly, in some complicated web apps, `IMEContentObserver` may get selection
change notifications **after** sending a text change notification to IME.
However, I cannot reproduce it with simple testcase. Therefore, the new test
does not test the original issue, but the approach of this patch must avoid
regressions of this special case handling.
Differential Revision: https://phabricator.services.mozilla.com/D137522
Currently, `mSelection` is cached only when `mText` is some, text rects are
cached only when `mSelection` is cached. However, as far as widget can work
with IME, it should cache content as far as possible. Therefore, this patch
makes it keep querying content data even after something fails.
Differential Revision: https://phabricator.services.mozilla.com/D137431
Even after applying this patch, it still returns error when queried with a
relative offset from selection and only when there is no composition string.
However, this shouldn't cause any problem because in this case, widget should
not try to query content with relative offset.
Differential Revision: https://phabricator.services.mozilla.com/D137430
It's intended to indicate whether the selection is collapsed or not, but it can
be referred by other members, there is no reasonable user and the name makes
developers confused.
Differential Revision: https://phabricator.services.mozilla.com/D137422
Sorry for this big patch.
This makes `WidgetQueryContentEvent::Reply` is stored with `Maybe` to get
rid of `WidgetQueryContentEvent`. And `Reply` stores offset and string
with `Maybe` and ``OffsetAndData<uint32_t>`, and also tentative caret offset
with `Maybe`. Then, we can get rid of `WidgetQueryContentEvent::NOT_FOUND`.
Note that I tried to make `OffsetAndData` have a method to create `NSRange`
for cocoa widget. However, it causes the column limit`to 100 or longer
and that causes unrelated changes in `TextEvents.h` and `IMEData.h`.
Therefore, I create an inline function in `TextInputHandler.mm` instead.
Differential Revision: https://phabricator.services.mozilla.com/D98264
Usually, IME sets selection and considers candidate list position at starting
new composition. However, Apple Japanese IME sometimes consider the candidate
list position at retrieving the character rects before setting selection.
Therefore, we need to store last commit string's character rects, but don't
need to store it in long time because Kakutei-Undo is supported by Japanese
IMEs and they work only immediately after committing a composition. E.g.,
after moving caret, it won't be available.
Depends on D97838
Differential Revision: https://phabricator.services.mozilla.com/D97839
Currently, it manages the composition start offset with `uint32_t` and setting
it to `UINT32_MAX` when there is no composition. But this is now rewritable
with `Maybe<uint32_t>` for easier to read.
Differential Revision: https://phabricator.services.mozilla.com/D97838
Immediately after committing composition, i.e., still remote content is
handling the commit, `ContentCacheInParent` does not think that it still
has composition, but `TSFTextStore::GetTextExt()` tries a query whose offset
is relative offset from the last composition start offset and then,
`ContentCacheInParent` solves it with selection start (typically, the last
composition end offset). Therefore, this may cause returning error from
`GetTextExt()` and some TIP may fail to do something for next typing.
This patch makes `TSFTextStore::GetTextExt()` consider whether it'll query
content with relative offset from last composition start or selection start,
from `TextEventDispatcher::IsComposing()` result rather than
`TextEventDispatcher::IsHandlingComposition()` since the former means whether
there is composition in the chrome process, but the latter is there is
composition in focused process, and `ContentCacheInParent` state matches the
former.
Depends on D97270
Differential Revision: https://phabricator.services.mozilla.com/D97392
Sylvestre Ledru