After fixing bug 1420849, remote process started to ignore composition events
after it synthesizes eCompositionCommit event after requesting to commit
composition. However, remote process still keeps returning composition events
when it receives from the main process. So, if the main process has already
sent eCompositionCommit(AsIs) event when it's requested to commit composition
from the remote process, ContentCacheInParent::OnEventNeedingAckHandled()
receives both eCompositionCommitRequestHandled and eCompositionCommit(AsIs)
events for *a* composition. Therefore, mPendingCompositionCount may be
decremented twice for a composition. This causes hitting MOZ_DIAGNOSTIC_ASSERT.
So, ContentCacheInParent need to manage if sent composition events are ignored
or not. Then, ContentCacheInParent::OnEventNeedingAckHandled() stops
decrementing mPendingCompositionCount when it receives eCompositionCommit(AsIs)
events which are ignored by the remote process.
This patch manages it with |mIsChildIgnoringCompositionEvents| and changes
|bool mIsPendingLastCommitEvent| to |uint8_t mPendingCommitCount| for
making ContentCache be able to manage multiple pending commit events if
its remote process is too busy.
MozReview-Commit-ID: CYQDeZXl7TJ
When ContentCacheInParent::RequestIMEToCommitComposition() returns true,
the remote process will synthesize eCompositionCommit event. This causes
destroying TextComposition event in the remote process (by
IMEStateManager::DispatchCompositionEvent()). Then,
IMEStateManager::DispatchCompositionEvent() will recreate new TextComposition
instance when it receives new composition event. Then, it may request
to commit composition to the main process via PuppetWidget accidentally.
So, after PuppetWidget::RequestIMEToCommitComposition() synthesizes
eCompositionCommit, PuppetWidget should discard following composition events
until it receives eCompositionStart since they are unnecessary for the content
and they are for the old composition, i.e., outdated events.
MozReview-Commit-ID: BNRcoYxABpd
ContentCacheInParent::mPendingCommitLength is never initialized until it
receives eCompositionCommit(AsIs) event from widget or receives the latest
content from the remote process when there is a composition.
The bug is, immediately after dispatching eCompositionStart and first
eCompositionChange event, MS Pinyin tries to query the character at caret,
but ContentCacheInParent::HandleQueryContentEvent() tries to resolve
related position of an eQueryTextRect event with the uninitialized
mPendingCommitLength. Therefore, the query almost always fails and MS Pinyin
gives up to show its candidate window.
This patch just initializes the member with 0.
MozReview-Commit-ID: JyYNqi8hoTa
For protecting main process, we should stop crashing main process in release
build even when we detect our bug. However, we should keep crashing with
MOZ_DIAGNOSTIC_ASSER which is enabled only on Night and Developer Edition.
MozReview-Commit-ID: 5BQ46IFzXXj
Now, TextComposition::RequestToCommit() manages if it has already requested
IME to commit or cancel composition and this is important for redundant
requests. Therefore, ContentCacheInParent::RequestIMEToCommitComposition()
shouldn't call nsIWidget::NotifyIME() directly.
MozReview-Commit-ID: 69VpgyK9Jk5
This is a simple bug of ContentCacheInParent. When
ContentCacheInParent::RequestIMEToCommitComposition() returns true,
PuppetWidget::RequestIMEToCommitComposition() will send
eCompositionCommitRequestHandled pseudo event message back to the main process.
This causes counting down mPendingEventsNeedingAck in
ContentCacheInParent::OnEventNeedingAckHandled(). Therefore, in the normal
path, ContentCacheInParent::OnCompositionEvent() increments it for receiving
the pseudo event message.
However, if the tab parent has already lost focus,
RequestIMEToCommitComposition() returns true without requesting native IME to
commit composition. So, ContentCacheInParent::OnCompositionEvent() cannot
increment mPendingEventsNeedingAck for coming
eCompositionCommitRequestHandled. Therefore, RequestIMEToCommitComposition()
needs to increment mPendingEventsNeedingAck by itself when it won't request
IME to commit composition but it returns true.
MozReview-Commit-ID: 4Alwfy8avB
This is a follow up patch of bug 1408086. The previous patch starts to append
log of 2 sets of composition events to app notes of crash report when
ContentCacheInParent::OnEventNeedingAckHandled() meets unexpected state and
crash itself. However, now, we know the unexpected state occurs when TabParent
receives eCompositionCommitRequestHandled message from its remote process.
The event comes when ContentCacheInParent::RequestIMEToCommitComposition()
returns true. So, we need to know what occurs in the method before the crash.
This patch defines each case of RequestIMEToCommitComposition() with an enum
class, RequestIMEToCommitCompositionResult and make
RequestIMEToCommitComposition() append one of its value to the array.
Then, ContentCacheInParent discards unnecessary log of this when it discards
log of old composition events. Finally, appends the log to the app notes of
crash report.
MozReview-Commit-ID: 9sJyl4SvUXu
We have a lot of crash reports in OnEventNeedingAckHandled() due to unexpected
state (hit MOZ_RELEASE_ASSERT). However, it's unclear what occurs and we're not
sure there are how many cases to crash because the stack trace is too short
because the method is called when TabParent receives event handled message from
the remote process. I.e., it doesn't show what happens immediately before the
crash.
This patch puts 2 sets of composition events to app notes of crash report when
it needs to crash. This *might* make damage to the performance. If so, after
fixing the crashes, we should back this out. Fortunately, we have a lot of
reports from either Nightly or Beta.
MozReview-Commit-ID: 9tDrEIf72MG
We have a minimum requirement of VS 2015 for Windows builds, which supports
the z length modifier for format specifiers. So we don't need SizePrintfMacros.h
any more, and can just use %zu and friends directly everywhere.
MozReview-Commit-ID: 6s78RvPFMzv
If ContentCacheInParent::RequestIMEToCommitComposition() returns true after its TabParent has already sent eCompositionCommit(AsIs) event, ContentCacheInParent::OnEventNeedingAckHandled() will receive both eCompositionCommit(AsIs) and eCompositionCommitRequestHandled for a composition. Then, that causes making mPendingCompositionCount decreased twice. That causes the crash.
So, even if its TabParent already lost focus, it should return false after its TabParent sent eCompositionCommit(AsIs).
MozReview-Commit-ID: 6OylQtc8kgC
Due to the fix of bug 1376424, ContentCacheInParent::FlushPendingNotifications() may be called when aWidget is nullptr. In this case, it doesn't need to do anything. So, it should ignore the call.
MozReview-Commit-ID: Fj3J76v6Xuk
Requests to commit/cancel composition came from remote process with sync message. So, it may be too late. E.g.,
* If the process already sent new composition start but is not handled by the remote process yet.
* If the process already send commit message but it's not handled by the remote process yet.
* If focus was already moved to different process.
In the former 2 cases, the remote process should wait eCompositionCommit(AsIs) events for clearing TextComposition. Therefore, the requested should be treated as it's handled asynchronously.
In the last case, the remote process should commit composition with latest composition string in the main process because if the remote process commits composition with "current" composition string in it, user may lost some inputted text.
MozReview-Commit-ID: 18BUoZZq7HS
IME should receive notifications and requests only from proper process. E.g., IME shouldn't commit composition by a request which came from previous focused process.
This patch makes that IMEStateManager::NotifyIME() takes pointer to TabParent optionally. If the request or notification came from remote process, it should be non-nullptr. Then, this makes it ignore notifications and requests from unexpected process.
Note that this patch also touches some gfx headers because they use |ipc::| but compiler is confused at the ambiguousness between |mozilla::ipc::| and |mozilla::dom::ipc::|.
Finally, this patch changes the NS_ASSERTION in IMEHandler::OnDestroyWindow() to MOZ_ASSERT because the orange caused by the NS_ASSERTION was not realized since there was already an intermittent orange bug caused by different NS_ASSERTION.
MozReview-Commit-ID: 9CgKXQRJWmN
When IME commits composition and restarts composition immediately, query event with relative offset doesn't work as expected until the commit is handled in the remote process.
Therefore, this patch makes ContentCacheInParent store the last commit string length until it's handled in the content. Note that ContentCache doesn't need to store all commit string lengthes which have not been handled in the remote process yet because it's important and possible to return next character of commited composition only when there are not 2 or more pending compositions. Even if there are, i.e., the remote process is too busy, ContentCacheInParent doesn't have necessary character rect.
MozReview-Commit-ID: 8gBr8kO4JcF
The problem is, only when requesting IME to commit or cancel composition is handled synchronously, TabParent does not send the dispatched eCompositionCommit(AsIs) event to the remote process. Therefore, TabParent (and ContentCacheInParent) never receives the message from the remote process.
This patch makes TabChild notifies TabParent of eCompositionCommitRequestHandled special event message after TabChild dispatches eCompositionCommit into the DOM tree. Then, ContentCacheInParent should decrease mPendingCompositionCount and mPendingEventsNeedingAck as usual composition event messages.
MozReview-Commit-ID: 7ec5HPiE687
TextComposition in the main process is destroyed when the main process sends eCompositionCommit(AsIs) to focused remote process. Therefore, ContentCacheInParent::mCompositionPendingCount is never 2 or more now.
It may cause ContentCacheInParent::Assign() setting older composition's start offset to current composition's start offset in the main process.
For making uplift the following patch easier, the wrong patch should be backed out first.
MozReview-Commit-ID: IHWc7qZBQtc
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
When ContentCacheInParent receives eCompositionStart, it temporarily sets mCompositionStart to selection start offset. However, if there is a composition in the remote process, the selection start is caret position in the composition string. Therefore, it's not useful information. Instead, the composition start offset should be used because around there are a lot of information.
For that, ContentCacheInParent should always store compostion start offset in the remote process with mCompositionStartInChild even if mWidgetHasComposition is false. And when it receives eCompositionStart, mCompositionStart should be set to mCompositionStartInChild.
MozReview-Commit-ID: DksPNEsi6Ec
ContentCacheInParent::mCompositionStart was set to ContentCacheInChild::mCompositionStart without any check. However, that's clearly wrong approach. For example, when the remote process handles some composition events after eCompositionCommit(AsIs) in the parent process, mCompositionStart is valid offset even after mWidgetHasComposition is set to false. Similarly, even after parent process sends eCompositionStart, the remote process may send invalid offset for mCompositionStart due to no composition in it.
For solving this issue, ContentCacheInParent should check mWidgetHasComposition.
If it's true and coming offset is valid, let's use it (even if mPendingCompositionCount is 2 or bigger since widget shouldn't use WidgetQueryContentEvent when there are some pending events).
If the coming offset is invalid but mWidgetHasComposition is false, let's use selection start instead because HandleQueryContentEvent() can work around selection start offset only.
Otherwise, i.e., mWidgetHasComposition is false, we should always set mCompositionStart to invalid offset.
MozReview-Commit-ID: IONU0Cbhpil
When ContentCacheInParent receives content information from the remote process, it notifies TextComposition of the latest composition start offset in the remote process. However, the information may be older composition's, i.e., the composition was already committed in the process but is still being handled by the remote process. TextComposition shouldn't work with such obsolete information.
Note that TextComposition instance is created and destroyed when WidgetCompostionEvent is handled by IMEStateManager. Then, TextComposition instance guarantees that all following composition events for a composition are sent to same EventTarget (including TabParent). So, TextComposition is always synced with a composition in widget.
MozReview-Commit-ID: 78NuvpE2rPx
If the remote process is busy or user restarts composition too quickly, there could be 2 or more compositions in ContentCache. For managing such case, ContentCacheInParent should manage the pending composition count which is increased at dispatching eCompositionStart event to the remote process and decreased at receiving eCompositionCommit(AsIs) event from the remote process.
MozReview-Commit-ID: KbTsK20NEZD
For making the meaning of ContentCacheInParent::mIsComposing clearer, let's rename it to mWidgetHasComposition. It becomes true when the parent process sends eCompositionStart to the remote process and false when the parent process sends eCompositionCommit(AsIs). So, it represents if the widget (i.e., the native IME handler in the chrome process) has composition.
MozReview-Commit-ID: 5k05IXMgJxw
This change avoids lots of false positives for Coverity's CHECKED_RETURN
warning, caused by NS_WARN_IF's current use in both statement-style and
expression-style.
In the case where the code within the NS_WARN_IF has side-effects, I made the
following change.
> NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));
> -->
> Unused << NS_WARN_IF(NS_FAILED(FunctionWithSideEffects()));
In the case where the code within the NS_WARN_IF lacks side-effects, I made the
following change.
> NS_WARN_IF(!condWithoutSideEffects);
> -->
> NS_WARNING_ASSERTION(condWithoutSideEffects, "msg");
This has two improvements.
- The condition is not evaluated in non-debug builds.
- The sense of the condition is inverted to the familiar "this condition should
be true" sense used in assertions.
A common variation on the side-effect-free case is the following.
> nsresult rv = Fn();
> NS_WARN_IF_(NS_FAILED(rv));
> -->
> DebugOnly<nsresult rv> = Fn();
> NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Fn failed");
ContentCache::TextRectArray::GetUnionRectAsFarAsPossible() should avoid crash by itself even if it's caller's bug. This makes parent process more stable, that is what one of the purpose of e10s is.
MozReview-Commit-ID: qKAfvm6eZw
This patch makes ContentCache store previous character's rect of selection anchor and selection focus because if caret is at end of a line, IME may query the last character of the line.
MozReview-Commit-ID: 5X1K8KtrYfl
This must be able to reproduce with some IMEs which creates 0 length composition string. In such case, mTextRectArray isn't available, but mTextRectArray.GetUnionRectAsFarAsPossible() always assumes that it's valid and has at least one rect. Therefore, it can meet this crash.
Therefore, this patch makes that ContentCacheInParent::GetUnionTextRects() not use mTextRectArray when it's not valid nor doesn't have rects.
MozReview-Commit-ID: 2yLMo4lxI3Z
To optimize copy of text rect array, we should use mozilla::Move. Also, after using it, we should mark is invalid result into SetEventResult
MozReview-Commit-ID: HH9H7DhK12
ContentCache may store composition string's rects which are inserted by one or more older composition event. Even in such case, native IME, especially TIP of TSF, expects non-empty rects.
Therefore, if native IME handler uses "insertion point relative query", ContentCache should return non-empty rect as far as possible. For example, even if query offset or range is not in its rect array of composition string, ContentCache should adjust the offset into the stored range.
MozReview-Commit-ID: 7hcIqxOWFk2
In e10s mode and during focus is in a remote process, ContentCache handles WidgetQueryContentEvent instead of ContentEventHandler. Therefore, for supporting selection relative WidgetQueryContentEvent in e10s mode, we need to support it in ContentCache too.
MozReview-Commit-ID: L5d5ilmpetG
It's not clear to me what NOTIFY_IME_OF_COMPOSITION_UPDATE means only from the name. For making the name clearer, this patch renames it to NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED and add some explanation to the definition.
MozReview-Commit-ID: 8ySYCNJ1Ytz
The bulk of this commit was generated with a script, executed at the top
level of a typical source code checkout. The only non-machine-generated
part was modifying MFBT's moz.build to reflect the new naming.
CLOSED TREE makes big refactorings like this a piece of cake.
# The main substitution.
find . -name '*.cpp' -o -name '*.cc' -o -name '*.h' -o -name '*.mm' -o -name '*.idl'| \
xargs perl -p -i -e '
s/nsRefPtr\.h/RefPtr\.h/g; # handle includes
s/nsRefPtr ?</RefPtr</g; # handle declarations and variables
'
# Handle a special friend declaration in gfx/layers/AtomicRefCountedWithFinalize.h.
perl -p -i -e 's/::nsRefPtr;/::RefPtr;/' gfx/layers/AtomicRefCountedWithFinalize.h
# Handle nsRefPtr.h itself, a couple places that define constructors
# from nsRefPtr, and code generators specially. We do this here, rather
# than indiscriminantly s/nsRefPtr/RefPtr/, because that would rename
# things like nsRefPtrHashtable.
perl -p -i -e 's/nsRefPtr/RefPtr/g' \
mfbt/nsRefPtr.h \
xpcom/glue/nsCOMPtr.h \
xpcom/base/OwningNonNull.h \
ipc/ipdl/ipdl/lower.py \
ipc/ipdl/ipdl/builtin.py \
dom/bindings/Codegen.py \
python/lldbutils/lldbutils/utils.py
# In our indiscriminate substitution above, we renamed
# nsRefPtrGetterAddRefs, the class behind getter_AddRefs. Fix that up.
find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \
xargs perl -p -i -e 's/nsRefPtrGetterAddRefs/RefPtrGetterAddRefs/g'
if [ -d .git ]; then
git mv mfbt/nsRefPtr.h mfbt/RefPtr.h
else
hg mv mfbt/nsRefPtr.h mfbt/RefPtr.h
fi
