Closed Bug 808287 Opened 12 years ago Closed 12 years ago

Intermittent test_bug386782.html | Editing failed. - got <p>contentEditabEDITED le</p>, expected EDITED <br><p>contentEditable</p> (and 3 more)

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: RyanVM, Assigned: jchen)

References

Details

(Keywords: intermittent-failure)

Attachments

(7 files, 4 obsolete files)

Fix out-of-order IME events during focus change (v1)
6.74 KB, patch
cpeterson
: review+
masayuki
: feedback+
Details | Diff | Splinter Review
Bug 805766 nsFocusManager should change IME state before dispatching focus event at activating different document (for checkin)
7.83 KB, patch
Details | Diff | Splinter Review
Bug 808287 nsTextStateManager shouldn't notify widget of selection changes and text changes after IME loses focus (for checkin)
5.81 KB, patch
Details | Diff | Splinter Review
Follow-up to combine IME enums, etc (v1)
6.05 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
Use counter instead of boolean to mask events after focus change (v1)
3.15 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
Make sure focus and blur IME notifications always match (v2)
5.33 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Add test for IME focus/blur mismatch condition (v2)
3.79 KB, patch
jchen
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #493381 +++ https://tbpl.mozilla.org/php/getParsedLog.php?id=16720469&tree=Mozilla-Inbound 60 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Editing failed. - got <p>contentEditabEDITED le</p>, expected EDITED <br><p>contentEditable</p> 61 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Edited contents still correct? - got <p>contentEditabEDITED le</p>, expected EDITED <br><p>contentEditable</p> 63 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Can we redo? - got <p>contentEditabEDITED le</p>, expected EDITED <br><p>contentEditable</p> 64 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Can we still edit? - got <p>contentEditabEDITED TWICE le</p>, expected EDITED TWICE <br><p>contentEditable</p>
As test author, please may you take a look at why this has recently regressed - I'm keen to minimise our Android oranges so that people start to take the platform seriously again. If you don't have the cycles to look now, that's fine - just let me know and I can disable just on Android. Cheers :-)
Flags: needinfo?(ehsan)
(In reply to Ed Morley [:edmorley UTC+0] from comment #22) > As test author, please may you take a look at why this has recently > regressed - I'm keen to minimise our Android oranges so that people start to > take the platform seriously again. If you don't have the cycles to look now, > that's fine - just let me know and I can disable just on Android. > > Cheers :-) Hmm, this is pretty serious. There have been no recent editor changes but I'm testing the stuff which landed before it to see which one is at fault.
Flags: needinfo?(ehsan)
I retriggered a bunch of tests on the revisions before the first revision on which we've seen this, and so far it seems like this is a regression from bug 805766. I'll back out that patch to see if it helps.
Blocks: 805766
Backed out bug 805766.
Hmm, it's odd... The patch of bug 805766 just changes the IME state before the first focus event. And only on Android??
Summary: Intermittent test_bug386782.html Editing failed | Edited contents still correct? | Can we redo? | Can we still edit? → Intermittent test_bug386782.html | Editing failed. - got <p>contentEditabEDITED le</p>, expected EDITED <br><p>contentEditable</p> (and 3 more)
When it failed, the caret is moved to next to the 13th character. That is the same length as "EDITED TWICE ". nsTextStateFrame notifies nsWindow of selection change and text change on Android. This could be done asynchronously. http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#768 http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#802 And then, nsWindow for Android notifies native IME of the notified changes. And then, Android IME might set selection again. These might be also done asynchronously, but I'm not sure.
Oops. So, I guess that setting selection from Android's widget may be done after the new window is available.
Jim: Is the last paragraph of comment 40 correct? I.e., Android's IME might set selection on different window if the old widget has already lost focus? If that is NOT correct, we can fix this bug by checking if the widget has already been gone. Otherwise, I have no idea. I think that we need to disable the test on Android or give enough delay before calling sendString() in the test.
Note that nsTextStateManager is used only on Android (in default settings).
Yes. It's possible (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #42) > Jim: > > Is the last paragraph of comment 40 correct? I.e., Android's IME might set > selection on different window if the old widget has already lost focus? Yes, notification to native IME on Android is asynchronous, so I think it is possible for it to set selection on a different target. In that case, this is an Android side bug and I will fix it. > If that is NOT correct, we can fix this bug by checking if the widget has > already been gone. Otherwise, I have no idea. I think that we need to > disable the test on Android or give enough delay before calling sendString() > in the test.
Thank you, Jim. I'll confirm if my investigation is correct.
Okay, I confirmed that my investigation must be right. I removed two characters from input characters after go back. https://hg.mozilla.org/try/rev/28b2a515b171 Then, I got this result: https://tbpl.mozilla.org/php/getParsedLog.php?id=16846737&tree=Try > 82 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Editing failed. - got <p>contentEditEDITED able</p>, expected EDITED <br><p>contentEditable</p> I'll write a test patch for nsTextStateManager but I guess that we need something in Android widget too.
Ah, but it should be done after bug 806996.
This decreases the frequency of this orange. However, it doesn't become zero. So, I think Android widget also has similar issue as I mentioned. # This patch depends on the patches for bug 806996.
(In reply to Jim Chen [:jchen :nchen] from comment #44) > Yes. It's possible (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) > from comment #42) > > Jim: > > > > Is the last paragraph of comment 40 correct? I.e., Android's IME might set > > selection on different window if the old widget has already lost focus? > > Yes, notification to native IME on Android is asynchronous, so I think it is > possible for it to set selection on a different target. In that case, this > is an Android side bug and I will fix it. How long do you need for that? If it's long, we should disable the test on Android since the backed out patch is important for Android too. The random orange means that nsTextStateManager starts working with designMode editor on Android. So, without the backed out patch, Android widget isn't received the text change notification and selection change notification on designMode editor.
This patch should work. It's on the try server here, https://tbpl.mozilla.org/?tree=Try&rev=c971627af38e. But how do I know it fixed the problem? I ran mochitest-3 a few times; is that enough?
Attachment #680260 - Flags: feedback?(masayuki)
I think that 10 or 20 times is enough. I triggered them.
Ah, did you include the backed out patch? I guess you didn't so. I pushed all patches. https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=cca92e09d2cc
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #55) > Ah, did you include the backed out patch? I guess you didn't so. I pushed > all patches. > > https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=cca92e09d2cc Good call! And it seems to be working on try (the one orange is not due to this bug). I think we should get these patches reviewed soon so you can push the designMode patches again.
Attachment #680260 - Flags: feedback?(masayuki) → feedback+
Comment on attachment 679607 [details] [diff] [review] nsTextStateManager shouldn't notify widget of selection changes and text changes after IME loses focus SelectionChangeEvent and TextChangeEvent shouldn't notify widget if Destroy() of nsTextStateManager which created them has already been called. In other words, if IME is already notified of losing focus, IME shouldn't receive the change notification.
Attachment #679607 - Flags: review?(bugs)
Attachment #679607 - Flags: review?(bugs) → review+
Comment on attachment 680260 [details] [diff] [review] Fix out-of-order IME events during focus change (v1) This patch disables IME event handling in Gecko during a focus change. If not disabled, events from Java to Gecko may be in mid-flight when focus changes to a new editor. The new behavior is as soon as Gecko is notified of a blur, we disable events handling. Then on focus, Gecko notifies Java. Java first synchronizes, and then sends an acknowledgement back to Gecko. Gecko then finally re-enables events handling. This way, any events in-flight during the blur are discarded.
Attachment #680260 - Flags: review?(cpeterson)
Comment on attachment 680260 [details] [diff] [review] Fix out-of-order IME events during focus change (v1) Review of attachment 680260 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: mobile/android/base/GeckoEditable.java @@ +52,5 @@ > > private static final boolean DEBUG = false; > private static final String LOGTAG = "GeckoEditable"; > private static final int NOTIFY_IME_REPLY_EVENT = 1; > + private static final int NOTIFY_IME_FOCUSCHANGE = 3; I see that GeckoEditable and GeckoInputConnection are both defining their own NOTIFY_IME enums to mirror those in AndroidBridge.cpp. We should probably consolidate our Java NOTIFY_IME enum in one class or the other. @@ +464,5 @@ > geckoPostToUI(new Runnable() { > public void run() { > // Make sure there are no other things going on > mActionQueue.syncWithGecko(); > + if (type == NOTIFY_IME_FOCUSCHANGE && state != 0) { Should we compare state to GeckoInputConnection.IME_STATE_DISABLED (0) instead of the magic number 0? ::: widget/android/nsWindow.cpp @@ +164,5 @@ > mIMEComposing(false), > mIMEMaskSelectionUpdate(false), > + mIMEMaskTextUpdate(false), > + // Mask IME events initially because there is no focused editors yet > + mIMEMaskEvents(true) I think having the comment nestled within the ctor's initialization list is confusing. I wasn't sure whether the comment was commenting on mIMEMastTextUpdate or mIMEMaskEvents. Consider squeezing the comment on the same line as mIMEMaskEvents or moving it within the ctor body if you want a longer explanation. @@ +1885,5 @@ > + > + if (ae->Action() == AndroidGeckoEvent::IME_ACKNOWLEDGE_FOCUS) { > + mIMEMaskEvents = false; > + return; > + } else if (mIMEMaskEvents) { You don't need this `else` because the previous `if` will return. @@ +1888,5 @@ > + return; > + } else if (mIMEMaskEvents) { > + // Still reply to events, but don't do anything else > + if (ae->Action() == AndroidGeckoEvent::IME_SYNCHRONIZE || > + ae->Action() == AndroidGeckoEvent::IME_REPLACE_TEXT) { Fix indentation so both `ae->Action()` line up. ::: widget/android/nsWindow.h @@ +183,5 @@ > nsCOMPtr<nsIIdleServiceInternal> mIdleService; > > bool mIMEComposing; > bool mIMEMaskSelectionUpdate, mIMEMaskTextUpdate; > + bool mIMEMaskEvents; Food for thought: as the number of boolean state flags increasing, I think we should consider formalizing these states into a proper IME state machine. If we know the current state, our threads can assert that the messages or state transitions they receive are expected. For example, if we are in an "unfocused" state, then we should not receive any IME text or selection events.
Attachment #680260 - Flags: review?(cpeterson) → review+
What do you think about combining the enums into GeckoEditableListener? Seems to be the logical place since the notify methods are defined there.
Attachment #681038 - Flags: review?(cpeterson)
Comment on attachment 681038 [details] [diff] [review] Follow-up to combine IME enums, etc (v1) Review of attachment 681038 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #681038 - Flags: review?(cpeterson) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Turns out my fix for the Java side was not complete. I will have a better fix soon.
The old patch had one rarer race condition where there is a focus and blur on the Gecko side before the Java side could acknowledge the focus. To Gecko, it looks like this: Focus (mask events = true) Blur (mask events => true) Acknowledge (mask events => false) <-- should be true because we don't have focus at this point Therefore, to Gecko, it looks like there was a second focus, but in fact the acknowledgement was for the first focus. This new patch uses a counter to make sure that we don't run into this: Focus (mask events = 1) Blur (mask events => 2) Acknowledge (mask events => 1) <-- stilled masked
Attachment #682062 - Flags: review?(cpeterson)
I moved the OnIMEFocusChange(true) call to outside of the constructor because of a scenario like this, > CreateTextStateManager() > \- nsTextStateManager() > \- OnIMEFocusChange() > \- <IME query events> > \- Editor::PostCreate() > \- <...> > |- DestroyTextStateManager() > |- <...> > |- CreateTextStateManager() When DestroyTextStateManager() is called, because nsTextStateManager() has not returned yet, sTextStateObserver is still null, and DestroyTextStateManager() simply returns. This is bad because CreateTextStateManager() is called next and it creates a new text state manager. But because we are already in CreateTextStateManager() before, two nsTextStateManager are created, and one of them is leaked. Also this causes a mismatch in OnIMEFocusChange(true) and OnIMEFocusChange(false) calls.
Attachment #682072 - Flags: review?(masayuki)
Comment on attachment 682062 [details] [diff] [review] Use counter instead of boolean to mask events after focus change (v1) Review of attachment 682062 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. When I was reviewing your previous mIMEMaskEvents patch, my "Spidey sense" was tingling about the use of a boolean flag. :)
Attachment #682062 - Flags: review?(cpeterson) → review+
(In reply to Jim Chen [:jchen :nchen] from comment #70) > Created attachment 682072 [details] [diff] [review] > Make sure focus and blur IME notifications always match (v1) > > I moved the OnIMEFocusChange(true) call to outside of the constructor > because of a scenario like this, > > > CreateTextStateManager() > > \- nsTextStateManager() > > \- OnIMEFocusChange() > > \- <IME query events> > > \- Editor::PostCreate() > > \- <...> > > |- DestroyTextStateManager() > > |- <...> > > |- CreateTextStateManager() > > When DestroyTextStateManager() is called, because nsTextStateManager() has > not returned yet, sTextStateObserver is still null, and > DestroyTextStateManager() simply returns. This is bad because > CreateTextStateManager() is called next and it creates a new text state > manager. But because we are already in CreateTextStateManager() before, two > nsTextStateManager are created, and one of them is leaked. Also this causes > a mismatch in OnIMEFocusChange(true) and OnIMEFocusChange(false) calls. Are you sure this case? CreateTextStateManager() is called after the editor receives focus event. So, nsEditor::PostCreate() must be called at least once. nsEditor::PostCreate() can be called two or more times, I'm sure the situation, but I confirmed it. So, it could be possible for your scenario. However, I'm not sure if that happens by query event.
Comment on attachment 682072 [details] [diff] [review] Make sure focus and blur IME notifications always match (v1) Anyway, your scenario could be possible. >diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp >index 4e0f7af..dc37179 100644 >--- a/content/events/src/nsIMEStateManager.cpp >+++ b/content/events/src/nsIMEStateManager.cpp >@@ -734,18 +734,16 @@ nsTextStateManager::nsTextStateManager(nsIWidget* aWidget, > NS_ENSURE_TRUE_VOID(mRootContent); > > if (nsIMEStateManager::sIsTestingIME) { > nsIDocument* doc = aPresContext->Document(); > (new nsAsyncDOMEvent(doc, NS_LITERAL_STRING("MozIMEFocusIn"), > false, false))->RunDOMEventWhenSafe(); > } > >- mWidget->OnIMEFocusChange(true); >- Move the "MozIMEFocusIn" event dispatcher too but see following comment. > if (mWidget->GetIMEUpdatePreference().mWantUpdates) { > ObserveEditableNode(); > } PuppetWidget's GetIMEUpdatePreference() returns cached value which is reset at OnIMEFocusChange(true) called. So, this must be called after that. Okay, we should rename the nsTextStateManager::nsTextStateManager() with nsTextStateManager::Init() again. And its constructor should just reset mObserving. And then, you don't need to check mRootContent before calling OnIMEFocusChange(true) since it's already been done. However, after calling OnIMEFocusChange(true), you should do: // OnIMEFocusChange(true) might cause recreating nsTextStateManager // instance via nsIMEStateManager::UpdateIMEState(). So, this // instance might already have been destroyed, check it. if (!mRootContent) { return; } And nsIMEStateManager::CreateTextStateManager() should do: sTextStateObserver = new nsTextStateManager(); NS_ADDREF(sTextStateObserver); // nsTextStateManager::Init() might create another nsTextStateManager // instance. So, sTextStateObserver would be replaced with new one. // We should hold the current instance here. nsRefPtr<nsTextStateManager> kungFuDeathGrip(sTextStateObserver); sTextStateObserver->Init(...); >@@ -769,17 +767,21 @@ nsTextStateManager::ObserveEditableNode() > void > nsTextStateManager::Destroy(void) > { > if (nsIMEStateManager::sIsTestingIME && mEditableNode) { > nsIDocument* doc = mEditableNode->OwnerDoc(); > (new nsAsyncDOMEvent(doc, NS_LITERAL_STRING("MozIMEFocusOut"), > false, false))->RunDOMEventWhenSafe(); > } >- mWidget->OnIMEFocusChange(false); >+ // If CreateTextStateManager failed, mRootContent will be null, >+ // and we should not call OnIMEFocusChange >+ if (mRootContent) { >+ mWidget->OnIMEFocusChange(false); >+ } Move the MozIMEFocusOut event dispatcher into the if block too.
And if it's possible, please add an automated test for that.
Addressed review comments. I am working on a test that I will post soon. Here's a stack of the problem occurring. You can see OnIMEFocusChange called in frame #0 and frame #27 (OnIMEFocusChange calls OnIMETextChange on Android to inform the native IME of the current text) > #0 nsWindow::OnIMEFocusChange (this=0x5dd82600, aFocus=true) > at /home/nchen/central/widget/android/nsWindow.cpp:2178 > #1 0x6234ba44 in nsTextStateManager::Init (this=0x5fd6ee60, aWidget=0x5dd82600, > aPresContext=<optimized out>, aContent=<optimized out>) > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:746 > #2 0x6234bb32 in CreateTextStateManager () > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:1072 > #3 nsIMEStateManager::CreateTextStateManager () > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:1041 > #4 0x6234bd64 in nsIMEStateManager::UpdateIMEState (aNewIMEState=..., aContent=0x608bec20) > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:415 > #5 0x624b3344 in SetFlags (this=<optimized out>, aFlags=<optimized out>) > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:501 > #6 nsEditor::SetFlags (this=<optimized out>, aFlags=259) > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:463 > #7 0x624bad06 in nsEditorEventListener::SpellCheckIfNeeded (this=0x64f55800) > at /home/nchen/central/editor/libeditor/base/nsEditorEventListener.cpp:980 > #8 0x624b74c8 in nsEditor::PostCreate (this=0x613bd2c0) > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:322 > #9 0x62360ce8 in PrepareEditor (aValue=<optimized out>, this=0x644556a0) > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1377 > #10 nsTextEditorState::PrepareEditor (this=0x644556a0, aValue=<optimized out>) > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1150 > #11 0x62360d9c in PrepareEditorEvent::Run (this=0x617a87c0) > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1033 > #12 0x622c2d2e in nsContentUtils::RemoveScriptBlocker () > at /home/nchen/central/content/base/src/nsContentUtils.cpp:5012 > #13 0x621db616 in ~nsAutoScriptBlocker (this=<optimized out>, __in_chrg=<optimized out>) > at ../../dist/include/nsContentUtils.h:2348 > #14 PresShell::FlushPendingNotifications (this=0x6195ea00, aType=Flush_Layout) > at /home/nchen/central/layout/base/nsPresShell.cpp:3874 > #15 0x6234c6e2 in nsContentEventHandler::InitCommon (this=0x5e5fdd70) > at /home/nchen/central/content/events/src/nsContentEventHandler.cpp:52 > #16 0x6234c7ae in nsContentEventHandler::Init (this=0x5e5fdd70, aEvent=0x5e5fe0d0) > at /home/nchen/central/content/events/src/nsContentEventHandler.cpp:88 > #17 0x6234da28 in nsContentEventHandler::OnQueryTextContent (this=0x5e5fdd70, aEvent=0x5e5fe0d0) > at /home/nchen/central/content/events/src/nsContentEventHandler.cpp:506 > #18 0x623409ee in nsEventStateManager::PreHandleEvent (this=0x6170d3e0, > aPresContext=<optimized out>, aEvent=0x5e5fe0d0, aTargetFrame=0x64f88140, aStatus=0x5e5fe098) > at /home/nchen/central/content/events/src/nsEventStateManager.cpp:1149 > #19 0x621dc266 in PresShell::HandleEventInternal (this=0x6195ea00, aEvent=0x5e5fe0d0, > aStatus=0x5e5fe098) at /home/nchen/central/layout/base/nsPresShell.cpp:6534 > #20 0x621dd39c in PresShell::HandleEvent (this=0x6195ea00, aFrame=<optimized out>, > aEvent=0x5e5fe0d0, aDontRetargetEvents=<optimized out>, aEventStatus=0x5e5fe098) > at /home/nchen/central/layout/base/nsPresShell.cpp:6200 > #21 0x621dc7b6 in PresShell::HandleEvent (this=<optimized out>, aFrame=<optimized out>, > aEvent=0x5e5fe0d0, aDontRetargetEvents=<optimized out>, aEventStatus=0x5e5fe098) > at /home/nchen/central/layout/base/nsPresShell.cpp:5770 > #22 0x62409f3e in nsViewManager::DispatchEvent (this=<optimized out>, aEvent=0x5e5fe0d0, > aView=<optimized out>, aStatus=0x5e5fe098) > at /home/nchen/central/view/src/nsViewManager.cpp:766 > #23 0x624081bc in nsView::HandleEvent (this=<optimized out>, aEvent=0x5e5fe0d0, > aUseAttachedEvents=<optimized out>) at /home/nchen/central/view/src/nsView.cpp:1069 > #24 0x6272acaa in nsWindow::DispatchEvent (this=0x5dd82600, aEvent=0x5e5fe0d0) > at /home/nchen/central/widget/android/nsWindow.cpp:641 > #25 0x6272b266 in OnIMETextChange (aNewEnd=2147483647, aOldEnd=2147483647, aStart=0, > this=0x5dd82600) at /home/nchen/central/widget/android/nsWindow.cpp:2211 > #26 nsWindow::OnIMETextChange (this=0x5dd82600, aStart=0, aOldEnd=2147483647, aNewEnd=2147483647) > at /home/nchen/central/widget/android/nsWindow.cpp:2198 > #27 0x6272a5a8 in nsWindow::OnIMEFocusChange (this=0x5dd82600, aFocus=<optimized out>) > at /home/nchen/central/widget/android/nsWindow.cpp:2182 > #28 0x6234ba44 in nsTextStateManager::Init (this=0x6195a6e0, aWidget=0x5dd82600, > aPresContext=<optimized out>, aContent=<optimized out>) > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:746 > #29 0x6234bb32 in CreateTextStateManager () > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:1072 > #30 nsIMEStateManager::CreateTextStateManager () > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:1041 > #31 0x624bae4c in Focus (aEvent=<optimized out>, this=<optimized out>) > at /home/nchen/central/editor/libeditor/base/nsEditorEventListener.cpp:908 > #32 nsEditorEventListener::Focus (this=<optimized out>, aEvent=<optimized out>) > at /home/nchen/central/editor/libeditor/base/nsEditorEventListener.cpp:856 > #33 0x624bb2e4 in nsEditorEventListener::HandleEvent (this=0x64f55800, aEvent=0x5fd9e7c0) > at /home/nchen/central/editor/libeditor/base/nsEditorEventListener.cpp:325 > #34 0x6233904a in nsEventListenerManager::HandleEventSubType (this=<optimized out>, > aListenerStruct=<optimized out>, aListener=0x64f55800, aDOMEvent=0x5fd9e7c0, > aCurrentTarget=0x608bec20, aPhaseFlags=6, aPusher=0x5e5fe3f0) > at /home/nchen/central/content/events/src/nsEventListenerManager.cpp:896 > #35 0x6233917c in nsEventListenerManager::HandleEventInternal (this=0x64f89060, > aPresContext=0x61d4fc00, aEvent=0x5e5fe470, aDOMEvent=0x5e5fe400, aCurrentTarget=0x608bec20, > aFlags=6, aEventStatus=0x5e5fe404, aPusher=0x5e5fe3f0) > at /home/nchen/central/content/events/src/nsEventListenerManager.cpp:969 > #36 0x62349b50 in HandleEvent (aEventStatus=0x5e5fe404, aCurrentTarget=0x608bec20, > aDOMEvent=0x5e5fe400, aEvent=<optimized out>, aPresContext=<optimized out>, > this=<optimized out>, aPusher=0x5e5fe3f0, aFlags=6) > at /home/nchen/central/content/events/src/nsEventListenerManager.h:153 > #37 HandleEvent (aPusher=0x5e5fe3f0, aFlags=6, aVisitor=..., this=<optimized out>, > aMayHaveNewListenerManagers=<optimized out>) > at /home/nchen/central/content/events/src/nsEventDispatcher.cpp:184 > #38 nsEventTargetChainItem::HandleEvent (this=<optimized out>, aVisitor=..., aFlags=6, > aMayHaveNewListenerManagers=<optimized out>, aPusher=0x5e5fe3f0) > at /home/nchen/central/content/events/src/nsEventDispatcher.cpp:160 > #39 0x62349d42 in nsEventTargetChainItem::HandleEventTargetChain (this=0x60f310a0, aVisitor=..., > aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=false, aPusher=0x5e5fe3f0) > at /home/nchen/central/content/events/src/nsEventDispatcher.cpp:316 > #40 0x6234a302 in nsEventDispatcher::Dispatch (aTarget=<optimized out>, aPresContext=0x61d4fc00, > aEvent=0x5e5fe470, aDOMEvent=0x0, aEventStatus=0x0, aCallback=0x0, aTargets=0x0) > at /home/nchen/central/content/events/src/nsEventDispatcher.cpp:634 > #41 0x62414bdc in FocusBlurEvent::Run (this=<optimized out>) > at /home/nchen/central/dom/base/nsFocusManager.cpp:1878 > #42 0x622c292c in nsContentUtils::AddScriptRunner (aRunnable=<optimized out>) > at /home/nchen/central/content/base/src/nsContentUtils.cpp:5033 > #43 0x62417432 in nsFocusManager::SendFocusOrBlurEvent (this=<optimized out>, aType=1300, > aPresShell=<optimized out>, aDocument=0x61d4f400, aTarget=0x608bec20, aFocusMethod=4096, > aWindowRaised=false, aIsRefocus=false) at /home/nchen/central/dom/base/nsFocusManager.cpp:1937 > #44 0x6241777c in Focus (aAdjustWidgets=true, aWindowRaised=false, aFocusChanged=true, > aIsNewDocument=false, aFlags=4098, aContent=0x608bec20, aWindow=<optimized out>, > this=0x5fd7f7c0) at /home/nchen/central/dom/base/nsFocusManager.cpp:1816 > #45 nsFocusManager::Focus (this=0x5fd7f7c0, aWindow=<optimized out>, aContent=0x608bec20, > aFlags=4098, aIsNewDocument=false, aFocusChanged=true, aWindowRaised=false, > aAdjustWidgets=true) at /home/nchen/central/dom/base/nsFocusManager.cpp:1655
Attachment #682072 - Attachment is obsolete: true
Attachment #682072 - Flags: review?(masayuki)
Attachment #682322 - Flags: review?(masayuki)
Comment on attachment 682322 [details] [diff] [review] Make sure focus and blur IME notifications always match (v2) > class nsTextStateManager MOZ_FINAL : public nsISelectionListener, > public nsStubMutationObserver > { > public: >- nsTextStateManager(nsIWidget* aWidget, >- nsPresContext* aPresContext, >- nsIContent* aContent); >+ nsTextStateManager() : mObserving(false) >+ { >+ } nit: nsTextStateManager() : mObserving(false) { }
Attachment #682322 - Flags: review?(masayuki) → review+
Thank you for the stack! > > #4 0x6234bd64 in nsIMEStateManager::UpdateIMEState (aNewIMEState=..., aContent=0x608bec20) > > at /home/nchen/central/content/events/src/nsIMEStateManager.cpp:415 > > #5 0x624b3344 in SetFlags (this=<optimized out>, aFlags=<optimized out>) > > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:501 > > #6 nsEditor::SetFlags (this=<optimized out>, aFlags=259) > > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:463 > > #7 0x624bad06 in nsEditorEventListener::SpellCheckIfNeeded (this=0x64f55800) > > at /home/nchen/central/editor/libeditor/base/nsEditorEventListener.cpp:980 > > #8 0x624b74c8 in nsEditor::PostCreate (this=0x613bd2c0) > > at /home/nchen/central/editor/libeditor/base/nsEditor.cpp:322 > > #9 0x62360ce8 in PrepareEditor (aValue=<optimized out>, this=0x644556a0) > > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1377 > > #10 nsTextEditorState::PrepareEditor (this=0x644556a0, aValue=<optimized out>) > > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1150 > > #11 0x62360d9c in PrepareEditorEvent::Run (this=0x617a87c0) > > at /home/nchen/central/content/html/content/src/nsTextEditorState.cpp:1033 > > #12 0x622c2d2e in nsContentUtils::RemoveScriptBlocker () > > at /home/nchen/central/content/base/src/nsContentUtils.cpp:5012 > > #13 0x621db616 in ~nsAutoScriptBlocker (this=<optimized out>, __in_chrg=<optimized out>) > > at ../../dist/include/nsContentUtils.h:2348 > > #14 PresShell::FlushPendingNotifications (this=0x6195ea00, aType=Flush_Layout) > > at /home/nchen/central/layout/base/nsPresShell.cpp:3874 > > #15 0x6234c6e2 in nsContentEventHandler::InitCommon (this=0x5e5fdd70) > > at /home/nchen/central/content/events/src/nsContentEventHandler.cpp:52 Is seems that #11 might be unsafe. I'll investigate it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4e1735a647 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b8756142f7 Still working on the test; leaving open until the test gets done.
Whiteboard: [orange] → [orange][leave open]
Comment on attachment 682559 [details] [diff] [review] Add test for IME focus/blur mismatch condition (v1) mozIMEFocusIn and mozIMEFocusOut should be reset before your test. And also please check IMEHasFocus too (before your test, please reset this too). Thank you!
Attachment #682559 - Flags: review?(masayuki) → review+
Addressed review comments. Carried over r+.
Attachment #682559 - Attachment is obsolete: true
Attachment #682821 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/131a7ee2362c
Flags: in-testsuite+
Whiteboard: [orange][leave open] → [orange]
https://hg.mozilla.org/mozilla-central/rev/131a7ee2362c
Assignee: nobody → nchen
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
Depends on: 819446
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: