Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 12 years ago

Now, nsTextStore dispatches DOM events (composition events and set selection event) during document locked. However, it may cause document is changed by DOM event handlers.

We should dispatch such DOM events after RequestLock() called OnLockGranted().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=976385460d6d

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 12 years ago

The part.3 patch does NOT work fine without this patch. They're separated for easier review.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 34 • 12 years ago

First of all, let's move composition data into new struct. This makes each member meaning clearer.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 35 • 12 years ago

Note: mLastDispatchedTextEvent will be moved into another struct, don't mind about that.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 36 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35)
> Note: mLastDispatchedTextEvent will be moved into another struct, don't mind
> about that.

Oops, not moved, it will be removed. Sorry for the bug spam.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 37 • 12 years ago

These new methods of nsTextStore::Composition make the code simple.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 38 • 12 years ago

This patch is the first important part of this bug.

We should just recode the action during document lock and need to flush the recorded actions at unlocking the document.

Note that On(Start|Update|End)Composition() are even called without document lock. Therefore, we need the FlushPendingActionsIfNotLocked class.

Note that only with this patch, all Get*() methods return information which are not changed by pending actions. This issue will be fixed by part.6.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 39 • 12 years ago

Just refines logging code for the document lock.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 40 • 12 years ago

Selection can be changed by SetSelection() and InsertTextAtSelection(). So, now, we should store the latest selection information which is also modified by pending actions outside of nsTextStore::Composition.

CurrentSelection() synchronizes the mSelection with actual selection of the focused content. So, before referencing the mSelection, the clients need to call CurrentSelection() first and they should use the result which references mSelection.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 41 • 12 years ago

Similar to the Selection, we should store the content text and modify it during the document locked for Get*() methods returning the latest content and selection which may be modified by SetSelection(), SetText() or InsertTextAtSelection().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 42 • 12 years ago

By the part.6, GetText() don't need to emulate the latest content and Composition::mLength is always same as Composition.mString.Length(). So, Composition::mLength is not necessary. We can remove it now.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 43 • 12 years ago

Now, UpdateCompositionExtent() just does restarting composition if it's necessary. So, let's rename it with RestartCompositionIfNecessary().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 44 • 12 years ago

OnStartCompositionInternal() isn't good name because it's also called by RestartCompositionIfNecessary().

Now, the method records compositionstart action. So, it should be RecordCompositionStartAction().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 45 • 12 years ago

Similar to part.9, let's make RecordCompositionEndAction().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 46 • 12 years ago

By these changes, no DOM events fired during document lock being granted. That means the content is actually locked from content script.

Now, changes which are caused by flushing pending actions are not notified to TSF via OnTextChange() or OnSelectionChange(). This is the requirements of TSF application.

However, even if content script changes the content at handling composition events or something, it should be notified by OnTextChange() and/or OnSelectionChange(). I have no idea how to implement this behavior. But this isn't a new bug, let's put off this issue.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 47 • 12 years ago

FYI: test build will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b6d0a7c9914a

	Masatoshi Kimura [:emk]
	Comment 48 • 12 years ago

Comment on attachment 706831 [details] [diff] [review]
part.2 Make some useful methods on nsTextStore::Composition

>+++ b/widget/windows/nsTextStore.cpp
>@@ -1265,36 +1266,31 @@ nsTextStore::SendTextEventForComposition
>-  if (mComposition.mSelection.acpStart !=
>-        mComposition.mSelection.acpEnd &&
>-      textRanges.Length() == 1) {
>+  if (mComposition.IsSelectionCollapsed() && textRanges.Length() == 1) {

|!mComposition.IsSelectionCollapsed()|?

>@@ -2486,25 +2471,26 @@ nsTextStore::OnStartCompositionInternal(
>   mWidget->InitEvent(event);
>   mWidget->DispatchWindowEvent(&event);
> 
>+  mComposition.Start(this);
>+
>   PR_LOG(sTextStoreLog, PR_LOG_DEBUG,
>          ("TSF: 0x%p   nsTextStore::OnStartCompositionInternal() succeeded: "

>@@ -2543,18 +2529,16 @@ nsTextStore::OnStartComposition(ITfCompo
>     return hr;
>   }
> 
>-  mComposition.StartLayoutChangeTimer(this);
>-
>   *pfOk = TRUE;
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,

Why did you move the position?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 49 • 12 years ago

(In reply to Masatoshi Kimura [:emk] from comment #48)
> Comment on attachment 706831 [details] [diff] [review]
> part.2 Make some useful methods on nsTextStore::Composition
> 
> >+++ b/widget/windows/nsTextStore.cpp
> >@@ -1265,36 +1266,31 @@ nsTextStore::SendTextEventForComposition
> >-  if (mComposition.mSelection.acpStart !=
> >-        mComposition.mSelection.acpEnd &&
> >-      textRanges.Length() == 1) {
> >+  if (mComposition.IsSelectionCollapsed() && textRanges.Length() == 1) {
> 
> |!mComposition.IsSelectionCollapsed()|?

Good catch! Thanks!

> >@@ -2486,25 +2471,26 @@ nsTextStore::OnStartCompositionInternal(
> >   mWidget->InitEvent(event);
> >   mWidget->DispatchWindowEvent(&event);
> > 
> >+  mComposition.Start(this);
> >+
> >   PR_LOG(sTextStoreLog, PR_LOG_DEBUG,
> >          ("TSF: 0x%p   nsTextStore::OnStartCompositionInternal() succeeded: "
> 
> >@@ -2543,18 +2529,16 @@ nsTextStore::OnStartComposition(ITfCompo
> >     return hr;
> >   }
> > 
> >-  mComposition.StartLayoutChangeTimer(this);
> >-
> >   *pfOk = TRUE;
> >   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
> 
> Why did you move the position?

I don't think it's good to start the timer which is a part of composition start from OnCompositionStart(). The processing in OnCompositionStartInternal() will be moved to FlushPendingActions(). And then, it needs to start the timer in it. So, this change is a preparation of part.3.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 51 • 12 years ago

Updated for the part.2's change.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 52 • 12 years ago

Removing MOZ_ASSERT() from CurrentSelection() since it may be called without document lock. E.g., from On*Composition().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 53 • 12 years ago

Updated for the new part.5.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 54 • 12 years ago

Comment on attachment 706832 [details] [diff] [review]
part.3 Store edit actions during document lock and flush them at unlocking the document

I find a mistake of this patch's design. On*Composition() may be called without document lock. Then, the actions are performed at quitting the called method (by FlushPendingActionsIfNotLocked). However, this can cause OnSelectionChangeInternal(). Then, TSF may request lock for checking the new selection. At this time, FlushPendingActions() call is nested.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 55 • 12 years ago

This patch delays to call mSink->OnChangeSelection() when FlushPendingActions() causes nsTextStore::OnChangeSelectionInternal() without document lock. It will be called at the end of FlushPendingActions().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 58 • 12 years ago

I'll request reviews again after test the patched build.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 59 • 12 years ago

Okay, ready for the review now.

See comment 38 for the detail. And let me know if you have some questions.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 61 • 12 years ago

See comment 40 for the detail.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 62 • 12 years ago

See comment 41 for the detail.

	Masatoshi Kimura [:emk]
	Comment 63 • 12 years ago

Comment on attachment 711852 [details] [diff] [review]
part.3 Store edit actions during document lock and flush them at unlocking the document

>@@ -736,27 +738,46 @@ nsTextStore::RequestLock(DWORD dwLockFla
>        "null phrSession", this));
>     return E_INVALIDARG;
>   }
> 
>   if (!mLock) {
>     // put on lock
>     mLock = dwLockFlags & (~TS_LF_SYNC);
>     PR_LOG(sTextStoreLog, PR_LOG_DEBUG,
>-      ("TSF: 0x%p   nsTextStore::RequestLock() notifying OnLockGranted()...",
>-       this));
>+      ("TSF: 0x%p   Locking (%s) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"
>+       ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"
>+       ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>",
>+       this, GetLockFlagNameStr(mLock).get()));
>     *phrSession = mSink->OnLockGranted(mLock);
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+      ("TSF: 0x%p   Unlocked (%s) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<"
>+       "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<"
>+       "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<",
>+       this, GetLockFlagNameStr(mLock).get()));
>+    if (IsReadWriteLocked()) {
>+      FlushPendingActions();

Does this patch satisfy the condition required by the following MSDN sentence?

http://msdn.microsoft.com/en-us/library/windows/desktop/ms538447%28v=vs.85%29.aspx
> Applications must never modify the document or send change notifications
> using the ITextStoreACPSink::OnTextChange method from within the
> ITextStoreACP::RequestLock method. If the application has pending changes to
> report, the application can only respond to the asynchronous lock request.

In other words, will FlushPendingActions() never modify the document?

>+  class NS_STACK_CLASS FlushPendingActionsIfNotLocked MOZ_FINAL

nit: The class name should be a noun rather than a verb.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 64 • 12 years ago

(In reply to Masatoshi Kimura [:emk] from comment #63)
> Comment on attachment 711852 [details] [diff] [review]
> part.3 Store edit actions during document lock and flush them at unlocking
> the document
> 
> Does this patch satisfy the condition required by the following MSDN
> sentence?
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms538447%28v=vs.
> 85%29.aspx
> > Applications must never modify the document or send change notifications
> > using the ITextStoreACPSink::OnTextChange method from within the
> > ITextStoreACP::RequestLock method. If the application has pending changes to
> > report, the application can only respond to the asynchronous lock request.
> 
> In other words, will FlushPendingActions() never modify the document?

Hmm, FlushPendingAction() modifies the document from RequestLock(), of course. But I feel the document is odd. My understanding is, the document lock is only valid during OnLockGranted() call.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538064%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538403%28v=vs.85%29.aspx
So, at unlocking the document, i.e.. immediately after OnLockGranted() call, we should modify the document (by FlushPendinActions()) but it's still in RequestLock(), therefore, we never call OnTextChange() (since mLock value is still non-zero at FlushPendingActions() call).

If we needed to modify the document asynchronously after returning from RequestLock(), it would be possible with message hack or something. However, I don't guess that works fine because I guess that TIP (the caller) attempts to confirm the document's state after RequestLock() call synchronously.

And the document uses "or", not "and". So, I think that it's safe if we don't call OnTextChange() during RequestLock() call even we modify the document at that timing.

I'd like to know jimm's advice if he has.

	Masatoshi Kimura [:emk]
	Comment 65 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #64)
> My understanding is, the document
> lock is only valid during OnLockGranted() call.
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms538064%28v=vs.
> 85%29.aspx
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms538403%28v=vs.
> 85%29.aspx

I'm confused. According to the document, TSF manager can modify the document only inside a read/write lock. But your patch is doing the exact opposite.

> And the document uses "or", not "and". So, I think that it's safe if we
> don't call OnTextChange() during RequestLock() call even we modify the
> document at that timing.

I think "not A or B" means "not (A or B)" here. (But let's hear it from a native English speaker.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 66 • 12 years ago

(In reply to Masatoshi Kimura [:emk] from comment #65)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #64)
> > My understanding is, the document
> > lock is only valid during OnLockGranted() call.
> > http://msdn.microsoft.com/en-us/library/windows/desktop/ms538064%28v=vs.
> > 85%29.aspx
> > http://msdn.microsoft.com/en-us/library/windows/desktop/ms538403%28v=vs.
> > 85%29.aspx
> 
> I'm confused. According to the document, TSF manager can modify the document
> only inside a read/write lock. But your patch is doing the exact opposite.

Ah, okay. I need to explain the strategy of this bug for you.

Yes, the callers (typically, TIP) can call SetSelection(), SetText() and InsertTextAtSelection() which can modify the document only while OnLockGranted() is called for read-write lock request.

During the document lock, the documents must not be modified by non-callers such as DOM event handler (i.e., web application or chrome). Therefore, we need to *delay* the DOM event dispatch after unlocking the document. That's the main purpose of this bug. For example, if a web application destroys the window during OnLockGranted() call, TIP may be confused. And also shouldn't cause committing composition string. If we allow them, that means the document is NOT locked actually even during OnLockGranted() call.

During the read-write lock, changes caused by the methods called by TIP modify the cached content and the cached selection which are implemented by part.6 and part.5. The cached data is used for result of Get*() method during the document lock. By this change, I realized that the code becomes simpler since a lot of query-content events can be removed and widget-destroyed check isn't necessary.

Note that, ideally, if a DOM event handler changes the content or selection from a DOM composition event handler, we need to call OnTextChange() and OnSelectionChange() for TIP outside the RequestLock() call. However, this mechanism has never been implemented yet because we must not call them if changes are caused by TIP. These patches also still have this bug. But we should put off this issue latter since it needs a lot of other work.

	Masatoshi Kimura [:emk]
	Comment 67 • 12 years ago

Then document modifications by TIPs will be invisible from non-TIPs until the document is unlocked. Is it correct?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 68 • 12 years ago

(In reply to Masatoshi Kimura [:emk] from comment #67)
> Then document modifications by TIPs will be invisible from non-TIPs until
> the document is unlocked. Is it correct?

If the non-TIPs means web apps, Yes. Exactly.

	Masatoshi Kimura [:emk]
	Comment 69 • 12 years ago

Consider the following scenario:
1. A TIP obtains a document lock and makes some modifications to the document. The modifications will be pending.
2. A web app obtains a text via DOM.
3. The TIP unlocked the document. All pending modifications will be committed.
4. The web app makes a modification to the text based on the text obtained in step 2. All modifications made by the TIP will be lost because they were invisible from the web app.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 70 • 12 years ago

No, #2 must not happen. The possible scenario is:

1. A TIP requests read-write lock
2. TIP receives OnLockGranted() call and may modify the document.
3. Immediately after returning from OnLockGranted(), FlushPendingActions() dispatches one or more DOM events. At this time, web application can do anything. But neither OnTextChange() nor OnSelectionChange() are called for TIP (this is a bug, I think).
4. Returning from RequestLock() called for #1.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 71 • 12 years ago

The problem of current code is, during #2, web applications can do anything.

	Masatoshi Kimura [:emk]
	Comment 72 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #70)
> No, #2 must not happen. The possible scenario is:
> 
> 1. A TIP requests read-write lock
> 2. TIP receives OnLockGranted() call and may modify the document.
> 3. Immediately after returning from OnLockGranted(), FlushPendingActions()
> dispatches one or more DOM events. At this time, web application can do
> anything. But neither OnTextChange() nor OnSelectionChange() are called for
> TIP (this is a bug, I think).
> 4. Returning from RequestLock() called for #1.

Will web apps never executed during the step 2. by, for example, setTimeout?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 73 • 12 years ago

I believe so. During calling OnLockGranted(), the thread processes TIP, TSF and nsTextStore. Perhaps, setTimeout() will be executed before getting next native message. Although, I don't know actual setTimeout() implementation. But if your concern can happen, then, anyway, we cannot avoid the problem.

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#288
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#579
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#256
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#209
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#89
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseAppShell.cpp#97

	Jim Mathies [:jimm]
	Comment 74 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #64)
> Hmm, FlushPendingAction() modifies the document from RequestLock(), of
> course. But I feel the document is odd. My understanding is, the document
> lock is only valid during OnLockGranted() call.

"The lock is only valid during the OnLockGranted call. When OnLockGranted returns, the document is considered unlocked."

Seems pretty clear - modifying the doc after OnLockGranted returns seems ok.

	Masatoshi Kimura [:emk]
	Comment 75 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #74)
> "The lock is only valid during the OnLockGranted call. When OnLockGranted
> returns, the document is considered unlocked."
> 
> Seems pretty clear - modifying the doc after OnLockGranted returns seems ok.

When the document is unlocked, *TIPs* cannot change the document (via the TSF manager). But the following sentence is a requirement to the *applications*. No?
> Applications must never modify the document or send change notifications
> using the ITextStoreACPSink::OnTextChange method from within the
> ITextStoreACP::RequestLock method. If the application has pending changes to
> report, the application can only respond to the asynchronous lock request.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 76 • 12 years ago

Hmm, you might be right. However, I think that we have no plan B for now.

First, we never call OnTextChange() at Flushing the pending actions because mLock is still non-zero at that time. This is not problem for your concern.

Next, dispatching composition or text events may cause changing the document by (web) applications. This is what you're concerned. However, even in this time, we don't call OnTextChange() (I mentioned this at the last paragraph in comment 66, but this is NOT a new bug with the patches).

So, I think, ideally, we should NOT dispatch the events into the DOM tree at that time but should modify nsEditor directly instead. And after that, we should dispatch DOM composition events (and other events such as input events).

But this needs a lot of additional work in XP level (not impossible).

Therefore, I think that we should land these patches for now because this patch prevents the change during calling OnLockGranted() and makes accessing contents faster. And then, I should work for making dispatching DOM events asynchronous in another bug.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 77 • 12 years ago

Let's sort out the issues:

1. Current m-c code dispatches DOM events during calling OnLockGranted(). This may cause unexpected result of Get*() methods during OnLockGranted() call. And also, the text store may be destroyed during OnLockGranted() call.

2. Even with the patches of this bug, DOM events are dispatched during RequestLock() call. Then, DOM event handler may modify the content but that doesn't cause OnTextChange() call. I.e., cached content in TIP and actual content may not match at that time.

The patches only fixed #1. In the future, TextComposition class should modify editor directly. And DOM composition event (and other all events which are fixed by the change, e.g., input event, select event) should be dispatched asynchronously after RequestLock() call.

So, please focus #1 for now. I believe these patches really improve the code in nsTextStore and avoid a lot of strange behavior which TIP feels.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 78 • 12 years ago

updated for latest m-c.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 80 • 12 years ago

Kimura-san, if you don't have much time to review them, let me know. If so, I think that jimm is a better reviewer.

	Masatoshi Kimura [:emk]
	Comment 81 • 12 years ago

Sorry, I'm busy for the time being. I have no time to review the substantial patch. Feel free to reassign the reviewer.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 82 • 12 years ago

Thank you, Kimura-san. No problem. Most Japanese people are busy in February and March.

Jimm, so, it's difficult Kimura-san to review these big patches. Could you review them?

See comment 38 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 83 • 12 years ago

Comment on attachment 711855 [details] [diff] [review]
part.5 Store selection even if there is no composition

See comment 40 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 84 • 12 years ago

See comment 41 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 85 • 12 years ago

Comment on attachment 706836 [details] [diff] [review]
part.7 Remove nsTextStore::Composition::mLength

See comment 42 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 86 • 12 years ago

Comment on attachment 706839 [details] [diff] [review]
part.10 Make nsTextStore::RecordCompositionEndAction() for processing composition end action

See comment 45 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 87 • 12 years ago

patched build with the latest m-c will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d9e9a47c4872

	Jim Mathies [:jimm]
	Comment 88 • 12 years ago

Comment on attachment 706839 [details] [diff] [review]
part.10 Make nsTextStore::RecordCompositionEndAction() for processing composition end action

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

::: widget/windows/nsTextStore.cpp
@@ +2672,5 @@
> +           ("TSF: 0x%p   nsTextStore::RecordCompositionEndAction() FAILED due "
> +            "to CurrentContent() failure", this));
> +    return E_FAIL;
> +  }
> +  currentContent.EndComposition(*action);

This seems odd. You've added action to mPendingActions but if you fail due to !currentContent.IsInitialized() we never call EndComposition. Plus the action stays in the pending array?

	Jim Mathies [:jimm]
	Comment 89 • 12 years ago

Comment on attachment 711855 [details] [diff] [review]
part.5 Store selection even if there is no composition

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

::: widget/windows/nsTextStore.cpp
@@ +1133,5 @@
> +  if (mSelection.IsDirty()) {
> +    // If the window has never been available, we should crash since working
> +    // with broken values may make TIP confused.
> +    if (!mWidget || mWidget->Destroyed()) {
> +      MOZ_CRASH();

Crashing the browser is the only solution we have here? This seems rather extreme.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 90 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #89)
> Comment on attachment 711855 [details] [diff] [review]
> part.5 Store selection even if there is no composition
> 
> Review of attachment 711855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsTextStore.cpp
> @@ +1133,5 @@
> > +  if (mSelection.IsDirty()) {
> > +    // If the window has never been available, we should crash since working
> > +    // with broken values may make TIP confused.
> > +    if (!mWidget || mWidget->Destroyed()) {
> > +      MOZ_CRASH();
> 
> Crashing the browser is the only solution we have here? This seems rather
> extreme.

Basically, it should never happen. When mWidget is cleared or destroyed, nsTextStore::Destroy() must be called at that time. So, after that, the link between nsTextStore and TIP should be disconnected. Why I used MOZ_CRASH() is easier to find such critical bug than assertion because assertions are usually not filed as bug :-(

However, I don't mind if you don't like crashing here. Just returning from here is Okay.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 91 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #88)
> Comment on attachment 706839 [details] [diff] [review]
> part.10 Make nsTextStore::RecordCompositionEndAction() for processing
> composition end action
> 
> Review of attachment 706839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsTextStore.cpp
> @@ +2672,5 @@
> > +           ("TSF: 0x%p   nsTextStore::RecordCompositionEndAction() FAILED due "
> > +            "to CurrentContent() failure", this));
> > +    return E_FAIL;
> > +  }
> > +  currentContent.EndComposition(*action);
> 
> This seems odd. You've added action to mPendingActions but if you fail due
> to !currentContent.IsInitialized() we never call EndComposition.

currentContent.EndComposition() is called for synchronizing the cached content as expected by TIP. Therefore, If |!currentContent.IsInitialized()| happened, there is no cached content which should be synchronized.

> Plus the action stays in the pending array?

No. If it's called during document lock, FlushPendingActions() must be called by nsTextStore::RequestLock(). Otherwise, i.e., nsTextStore::OnEndComposition() is called without document lock, AutoPendingActionAndContentFlusher instance in OnEndComposition() will call FlushPendingActions(). (Of course, if somebody forgot to create an auto flusher, it would occur such bug you concerned)

	Jim Mathies [:jimm]
	Comment 92 • 12 years ago

Comment on attachment 711855 [details] [diff] [review]
part.5 Store selection even if there is no composition

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

Well if it is never going to happen. (Although with mozilla I've learned to never say never. ;)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 93 • 12 years ago

Thank you jimm for your review for such difficult patches! (separated patches are sometimes not easy to read)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 94 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9048c032545b
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f12498de97
https://hg.mozilla.org/integration/mozilla-inbound/rev/7001fb7460a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0d1d378577
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb30fa1ffd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/de121608ca36
https://hg.mozilla.org/integration/mozilla-inbound/rev/f11dcd5c542b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0431bc8042d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4641d29cab
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aadc0e21f75

	Ed Morley [:emorley]
	Comment 95 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/9048c032545b
https://hg.mozilla.org/mozilla-central/rev/08f12498de97
https://hg.mozilla.org/mozilla-central/rev/7001fb7460a2
https://hg.mozilla.org/mozilla-central/rev/ed0d1d378577
https://hg.mozilla.org/mozilla-central/rev/fdb30fa1ffd1
https://hg.mozilla.org/mozilla-central/rev/de121608ca36
https://hg.mozilla.org/mozilla-central/rev/f11dcd5c542b
https://hg.mozilla.org/mozilla-central/rev/f0431bc8042d
https://hg.mozilla.org/mozilla-central/rev/3f4641d29cab
https://hg.mozilla.org/mozilla-central/rev/7aadc0e21f75