In bug 1057898 I implemented a feature where if the user focus a new input from another input, we will consume the blur message before the new focus message. The original intention is to allow keyboard app (and System) to react the actual focus as fast as possible. However, it turned out our impl can never consume the blur message if the focus changes from one input to another input in another process. Think of tapping rocket bar input from the message app for example. Given that, a safeguard in input mgmt and keyboard app in Gaia can never be removed -- they will always have to properly handle the focus message during blur message is being handled (in the middle of transitioning out, cleaning up etc.) Such safeguard is in https://github.com/mozilla-b2g/gaia/blob/6fac9625103575beb7c87e9b08192c85cb5ddae1/apps/system/js/input_window_manager.js#L195-L201 and https://github.com/mozilla-b2g/gaia/blob/6fac9625103575beb7c87e9b08192c85cb5ddae1/apps/keyboard/js/keyboard/state_manager.js (by always putting the action follows into a queue) I therefore think it make sense to rollback what's done in bug 1057898 to make forms.js simpler. It will not be a complete revert because many of the test fixes has been improved since that fix and I want to keep them.

We can try to do so in Keyboard.jsm in another bug but let's not worry about that for now.

ulei or Masayuki, can either of you review this patch? Thanks! https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a3e39416389

Comment on attachment 8602519 [details] [diff] [review] Patch v1.0 This is not in my area...

I will need to fix bug 1162383 first before landing this, and also confirm the problem with reftest report by try.

Masayuki, It looks like this JS patch in forms.js will crash nsIEditor at NotifyEditorObservers in file https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug612271-1.html?from=bug612271-1.html&case=true#1 According to https://treeherder.mozilla.org/logviewer.html#?job_id=7301768&repo=try Could you advice me what to do next to fix this? Thanks!

A quick on MDN reveals that we should be able to re-impl bug 1057898 if the |relatedTarget| property is filled on the blur event. With that we no longer need waitForNextTick() to tell whether blur event is blur to non-input or to another input. The bug is already filed at bug 962251.

So I've build the ICS emulator to run the test however it crashes at the same place even without my patch... completely stuck here. :'(

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #7) > So I've build the ICS emulator to run the test however it crashes at the > same place even without my patch... completely stuck here. :'( It turned out I would need to |./build.sh| instead of |./build.sh gecko| to push Gecko changes into the Emulator.

I can now confirm the crash is caused by the patch, when we access element.selectionStart and element.selectionEnd in the function loop of EditAction. https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#994-995 If I comment out that two lines the tests would continue without crash. Without it Gecko will crash when EditAction returns. The next thing to look is probably HTMLInputElement::GetSelectionStart ?

Removing the size change on the onkeydown call https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug612271-1.html?from=bug612271-1.html&case=true#6 would prevent the crash too. Obviously the test will fail.

Masayuki, Could you review this patch? Thanks! So the root cause of the crash is because the ranged for loop will produce a null observer for us to call EditAction() on. The operations in forms.js will cause one instance of IMEContentObserver to be destroyed and be replaced with a new one. By using a traditional loop, we will be able to call the EditAction() on the newly added one instead of the destroyed one. This is by far the correct implementation, as this pattern itself can behave incorrectly if we are not adding/removing the observer after ourselves. However, the "correct" behavior is not defined. If we ought to allow observers to cause observers to be removed or added, Cervantes (cyu@mozilla.com) suggests we should adopt a while-loop-and-pop pattern to ensure all of the observers are called (and exists when called). If you agree I can work on that with his help.

BTW, the loop was changed back in https://hg.mozilla.org/mozilla-central/rev/dea5a5323d5c

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11) > This is by far the correct implementation, as this pattern itself can behave > incorrectly if we are not adding/removing the observer after ourselves. > However, the "correct" behavior is not defined. If we ought to allow > observers to cause observers to be removed or added, Cervantes > (cyu@mozilla.com) suggests we should adopt a while-loop-and-pop pattern to > ensure all of the observers are called (and exists when called). If you > agree I can work on that with his help. But maybe in another bug though, since the two are not really related and the first patch has been blocked for too long. i.e. land the patch here to fix the crash first and work on a proper fix in another bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d7f7648829b https://treeherder.mozilla.org/logviewer.html#?job_id=7542381&repo=try There is a Gip error. (I didn't successfully invoke Gip in previous Try runs because of bug 1152865)

I can now reproduce the Gaia UI tests failure locally, however I have not figure out what's wrong with it. The recipient part of the UI in Message app employs a rather complex way of re-focus inputs, which take some time to understand (or find people to ask).

It looks like the test will try to tap on the "To" area again to re-focus the input even when the focus will not get lost after this patch. This is sad -- we might need to land Gecko/Gaia patch at the same time for this...

Comment on attachment 8607370 [details] [gaia] timdream:message-recipient-no-two-taps > mozilla-b2g:master Remove this line will make the Gecko w/ patch passes the test. It passes on the unpatched Gecko locally too. So we should remove it if it really does -- pending verification on [1]. [1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=6bd2f2a6257843272d0b8da730ca04569a7fb0ad

Comment on attachment 8607370 [details] [gaia] timdream:message-recipient-no-two-taps > mozilla-b2g:master :geo, could you review this? Context: The Gecko patch here will change the timing of keyboard display a bit. The tap removed here will blur the focus (instead of an no-op) on the patched Gecko. Message app always re-focus to the new recipient field after the semicolon, so I presume the test was written before Message app start to do that. Since this patch passes on both unpatch Gecko and patched Gecko, I will land this first before the Gecko patch, after your review. Thanks!

Comment on attachment 8607370 [details] [gaia] timdream:message-recipient-no-two-taps > mozilla-b2g:master Looks good. Given what you say re: behavior change, think the test would want to not tap again anyway, since it might hide a bug where something else steals focus. This is a good change.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #19) > Comment on attachment 8607370 [details] > [gaia] timdream:message-recipient-no-two-taps > mozilla-b2g:master Merged this part on gaia master: https://github.com/mozilla-b2g/gaia/commit/01938468cf518009e08262532c6dfc56b6cb2210

Comment on attachment 8605710 [details] [diff] [review] Patch part 2 > + // It possible the observers will be modified when the observers are > + // being executed. Do not change this back to ranged for loop. > + for (uint32_t i = 0; i < mEditorObservers.Length(); i++) { > + mEditorObservers[i]->EditAction(); > } I wonder, according to your comment, this must not work well with following case: 1. mEditorObservers[i]->EditAction() is called. 2. mEditorObservers[i] is removed from it. 3. mEditorObservers[i] is now mEditorObservers[i+1]. 4. But new mEditorObservers[i+1] will be used, i.e., the old [i+1] isn't used. So, I think that nsEditor should have mIsUsingEditorObservers or something as bool. It should be set true before the for loops with a stack class like AutoEditorObserversFlusher. Then, nsEditor::RemoveEditorObserver() should replace the specified observer as nullptr in mEditorObservers if mIsUsingEditorObservers. And when AutoEditorObserversFlusher is destroyed, it should remove the all nullptr item from the array. Then, we can guarantee all observers except some of them which are removed from array during a loop are preformed correctly. # Sorry for the delay to review :-(

Patch updated accordingly. Thanks :kanru for providing C++ help. https://treeherder.mozilla.org/#/jobs?repo=try&revision=507b960f2dbc

Comment on attachment 8608528 [details] [diff] [review] Patch part 2 v2 > NS_IMETHODIMP > nsEditor::RemoveEditorObserver(nsIEditorObserver *aObserver) > { > NS_ENSURE_TRUE(aObserver, NS_ERROR_FAILURE); > >- mEditorObservers.RemoveElement(aObserver); >+ if (mIsUsingEditorObservers) { >+ mEditorObservers.ReplaceElementAt(mEditorObservers.IndexOf(aObserver), nullptr); What occurs if IndexOf() returns NoIndex? I think that you should check it before a call of ReplaceElementAt(). >+ for (uint32_t i = 0; i < mEditorObservers.Length(); i++) { >+ if (mEditorObservers[i] != nullptr) { if (mEditorObservers[i]) { >+ mEditorObservers[i]->EditAction(); >+ } > } > case eNotifyEditorObserversOfBefore: > mIsInEditAction = true; >- for (auto& observer : mEditorObservers) { >- observer->BeforeEditAction(); >+ for (uint32_t i = 0; i < mEditorObservers.Length(); i++) { >+ if (mEditorObservers[i] != nullptr) { ditto. >+ mEditorObservers[i]->BeforeEditAction(); >+ } > } >+ > break; > case eNotifyEditorObserversOfCancel: > mIsInEditAction = false; >- for (auto& observer : mEditorObservers) { >- observer->CancelEditAction(); >+ for (uint32_t i = 0; i < mEditorObservers.Length(); i++) { >+ if (mEditorObservers[i] != nullptr) { ditto. > protected: >+ class AutoEditorObserversFlusher class MOZ_STACK_CLASS AutoEditorObserversFlusher >+ { >+ public: >+ AutoEditorObserversFlusher(nsEditor* aEditor) >+ { >+ mEditor = aEditor; Could you use initializer? i.e., AutoEditorObserversFlusher(nsEditor* aEditor) : mEditor(aEditor) { >+ mEditor->mIsUsingEditorObservers = true; Please save the old value. It may be true already. >+ } >+ ~AutoEditorObserversFlusher() >+ { >+ mEditor->mIsUsingEditorObservers = false; >+ >+ for (int32_t i = mEditor->mEditorObservers.Length() - 1; i >= 0; i--) { >+ if (mEditor->mEditorObservers[i] == nullptr) { if (!mEditor->mEditorObservers[i]) { >+ mEditor->mEditorObservers.RemoveElementAt(i); >+ } >+ } >+ >+ mEditor = nullptr; >+ } >+ >+ private: >+ nsEditor* mEditor; Use nsCOMPtr<nsIEditor>.

Patch updated accordingly except /nsCOMPtr<nsIEditor>/nsRefPtr<nsEditor>/ because we are referencing the instance there. > Please save the old value. It may be true already. I did save the old value with a mWasUsingEditorObservers property, but I don't think the code will behavior correctly when this happens; when the inner loop ends the distructor will remove all nullptr add in the outer loop, and maybe disrupt the outer loop, making it skip some observers. Maybe we should just throw/crash in this case?

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #25) > > Please save the old value. It may be true already. > > I did save the old value with a mWasUsingEditorObservers property, but I > don't think the code will behavior correctly when this happens; when the > inner loop ends the distructor will remove all nullptr add in the outer > loop, and maybe disrupt the outer loop, making it skip some observers. > > Maybe we should just throw/crash in this case? Ah, good point. I think that the destruct should work only when the saved value is false. Otherwise, it shouldn't touch to the array.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #23) > Created attachment 8608528 [details] [diff] [review] > Patch part 2 v2 > > Patch updated accordingly. Thanks :kanru for providing C++ help. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=507b960f2dbc Gip-f14 errors still shown here.

Comment on attachment 8608576 [details] [diff] [review] Patch part 2 v2.1 > NS_IMETHODIMP > nsEditor::RemoveEditorObserver(nsIEditorObserver *aObserver) > { > NS_ENSURE_TRUE(aObserver, NS_ERROR_FAILURE); > >- mEditorObservers.RemoveElement(aObserver); >+ if (mIsUsingEditorObservers) { >+ uint32_t i = mEditorObservers.IndexOf(aObserver); auto may be better, but up to you. > protected: >+ class MOZ_STACK_CLASS AutoEditorObserversFlusher >+ { >+ public: >+ AutoEditorObserversFlusher(nsEditor* aEditor) >+ : mEditor(aEditor) >+ , mWasUsingEditorObservers(mEditor->mIsUsingEditorObservers) >+ { >+ mEditor->mIsUsingEditorObservers = true; >+ } >+ ~AutoEditorObserversFlusher() >+ { >+ mEditor->mIsUsingEditorObservers = mWasUsingEditorObservers; >+ >+ for (int32_t i = mEditor->mEditorObservers.Length() - 1; i >= 0; i--) { >+ if (!mEditor->mEditorObservers[i]) { >+ mEditor->mEditorObservers.RemoveElementAt(i); >+ } >+ } Okay, return before this for if mWasUsingEditorObservers is true. With that change, r=masayuki.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #28) > Comment on attachment 8608576 [details] [diff] [review] > Patch part 2 v2.1 Thanks. (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #27) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=507b960f2dbc > > Gip-f14 errors still shown here. This is now the last blocker from landing the change. Apparently comment 19 is not the cause. Going to investigate tomorrow.

(In reply to Autolander from comment #30) > Created attachment 8608629 [details] > [gaia] timdream:remove-tap_recipient_section > mozilla-b2g:master This is a rather radical change that just don't tap the field altogether (as the keyboard should just show up when the user enters the new message panel). Also pushed to Try since TreeHerder on Gaia pull request doesn't seem to be able to reproduce the bug here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c751cc055255

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #31) > Also pushed to Try since TreeHerder on Gaia pull request doesn't seem to be > able to reproduce the bug here. Sorry, I was meant to say the bug does not seems to be reproducible locally.

Let's try this one... https://treeherder.mozilla.org/#/jobs?repo=try&revision=45a4d320a84b

Comment on attachment 8608629 [details] [gaia] timdream:remove-tap_recipient_section > mozilla-b2g:master Geo, Would you mind reviewing this? I am sorry that the previous patch was not a complete fix. It turned out the keyboard will show up when the user enters the new message panel, so we don't really need to tap the field at all!

Patch to commit uploaded but Gaia patch needs to be reviewed and landed first.

Comment on attachment 8609165 [details] [diff] [review] Patch to commit (part 2) Hi Ehsan, :kanru told me I should ask you for feedback on this, since removing OwningNonNull might not be safe. Could you take a look the nsEditor.h part? Thanks.

Comment on attachment 8609165 [details] [diff] [review] Patch to commit (part 2) Review of attachment 8609165 [details] [diff] [review]: ----------------------------------------------------------------- I have a hard time understanding what you're trying to do here, but this seems clearly wrong. Reading comment 11, you may have hit bug 1166932, can you please try the patches there? If that doesn't fix things, can you please describe the issue more closely, so that we can figure out a better way to fix this? Thanks!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #41) > I have a hard time understanding what you're trying to do here, but this > seems clearly wrong. Reading comment 11, you may have hit bug 1166932, can > you please try the patches there? Hi, could you cc me to the bug?

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #41) > Comment on attachment 8609165 [details] [diff] [review] > Patch to commit (part 2) > > Review of attachment 8609165 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have a hard time understanding what you're trying to do here, but this > seems clearly wrong. Reading comment 11, you may have hit bug 1166932, can > you please try the patches there? I can't view the bug, probably so does Tim. Could you cc us? > If that doesn't fix things, can you please describe the issue more closely, > so that we can figure out a better way to fix this? Thanks! What Tim hit here is that the observer array is changed by the listeners. Ranged based for loop caches the begin and end iterator so that leads to undefined behavior.

CCed you both. I won't discuss the details of the issue here, please ask questions on that bug if the problem and the fix is not clear. :-)

Comment on attachment 8609165 [details] [diff] [review] Patch to commit (part 2) Per discussion in bug 1166932.

Comment on attachment 8608629 [details] [gaia] timdream:remove-tap_recipient_section > mozilla-b2g:master Code looks good, though I'm taking your word on the behavior change. I'd generally prefer for tests that we import only at top of file (the inline in the app object is for that particular use case) but I won't r- for that.

gaia master: https://github.com/mozilla-b2g/gaia/commit/bcb3000dcd8f975c784dcf2ed65a62e8d1fe49e6

Final Try push to make sure everything is fixed; gonna tag checkin-needed when it's ready: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c76ee7d64c

Backed out for suspicion of causing share_activity_test.js permafail. https://treeherder.mozilla.org/logviewer.html#?job_id=10269489&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9430165acc1b

This was also hitting Android timeouts again. https://treeherder.mozilla.org/logviewer.html#?job_id=10266585&repo=mozilla-inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50) > Backed out for suspicion of causing share_activity_test.js permafail. Confirmed green post-backout.

I can't produce the breakage locally because of bug 1170426.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #54) > I can't produce the breakage locally because of bug 1170426. I can run the test now, but not reliably... need some time to figure out where the real issue lies.

I can confirm that the tests fails because it taps the send button while the keyboard is sliding down. Gonna change the way the button is being tapped since it doesn't seem to happen in the real world.

Will push another try when bug 1171387 reaches m-c and require for checkin when we are sure of everything is green.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #48) Although I wonder why this run did not found bug 1171387....

According to the profile at bug 970093 comment 73, this patch will save 35ms. http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&search=handleFocus&selection=0,710,15,972,973,974,975

I can reproduce the errors in comment 60 and strangely, even if the keyboard is clearly not on screen and the activity caller app is not transitioning, we still can't tap() the 'Pick Contact' button -- and change the call to click() would fix it. I think there is something to fix in the tap() itself when a mozpasspointerevents frame is involved.

Hi :kats, I wonder if this symptoms rings any bell. This is somewhere similar like bug 1121033 although intermittently locally but always reproducible on try. Long story short, touch (marionette "singleTap" command) seems sometimes fail to trigger click event handler, while click (marionette "clickElement" command) always do. The patch here simply change the timing of the keyboard showing/hidden and thus trigger all these settle timing issues. I am now trying to dive into the marionette server code to see how touch event. It seems that we took different code path in the code here. https://dxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#799-805 I have not yet fully figure out the entire code path when it fails (the above link is where I am at for now), but I want to ni you first in case you already know where to look. If not, it's fine and I will keep looking and maybe ni you again until I have something concrete. Thanks!

So I don't know off the top of my head what's going on here, but assuming "singleTap" generates touch events, and assuming "clickElement" generates mouse events, it's quite possible that timing issues will affect "singleTap" but not "clickElement". In order for a "tap" to be converted into a "click", the touchdown and touchup have to be within a certain time interval. It's possible that the timing changes are causing the touchup to be delayed resulting in a long-press kind of gesture instead of a click.

Hi James, Do you know how do get $gaia/marionette-mocha-gecko.log from the failed task in comment 60? I can't no longer reproduce it locally so I would need to read the log there. I've tried to run the docker script in the task detail and somehow successful to produce that too, but I can't seem be able to pull out the actual log from the docker runtime. If you could tell me how to do that that would be great too (I am getting a log file with just one line: "Error: cannot open display: :0" when running |docker cp $NAME:/home/worker/gaia/marionette-mocha-gecko.log ./| after seeing the error being reproduced and ^C the runtime).

Also, running that docker image takes 25 min to start from here. Every time. It tries to download gaia-central.tar.gz and the file is 2G ... can't we use git shallow clone there?

After rebase Gecko today I can again reproduce the bug locally. I am now trying to understand where the mouse events are being converted to click. Alternatively, maybe I should bisect between the two rebase instead?

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #66) > Alternatively, maybe I should bisect between the two rebase instead? The current tip is 5787d784485e

I can find the code path of the click event with rebase. I suspect the click event is consumed by browserElementPanningAPZDisabled.js but it's proven to be false. The JS stacktrace I have confirmed the click event come from emitMouseEvent() call that emits mouseup event. I just need to know why the native code decided not to dispatch the click event here. (Here is what I get from installing a click event listener in browserElementPanningAPZDisabled.js) BrowserElementPanningAPZDisabled click cp_setupListenersForPanning/<@chrome://global/content/BrowserElementPanningAPZDisabled.js:47:9 ActionChain.prototype.emitMouseEvent@chrome://marionette/content/actions.js:125:1 ActionChain.prototype.mouseTap@chrome://marionette/content/actions.js:491:3 singleTap@chrome://marionette/content/listener.js:924:5

By inserting an resize event listener, I can now confirm the resize happens between mousedown and mouseup and moved the button away from the touch target, therefore preventing the click event from firing. I previously thinks this shouldn't happen because I thought nsDOMWindowUtils::SendMouseEvent() dispatches the event synchronously, but the order in the log shows otherwise. My assumption in bug 1171387 turned out to be correct*, but I am not convinced this would happen in the real world. Need to figure out how to make Marionette dispatches the event in a more reasonable way. * Still this does not explain comment 61 but I can no longer reproduce that... so maybe there are two issues here.

I've decided to workaround the attach button with the same trick in bug 1174062. I am moving the discussion of Marionette quirk to bug 1171950 comment 2. I hope we could land this soon.

I am actually not sure of how try picks up Gaia, but here is the try push. If this didn't include bug 1174062 I will push a new one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf878505c22

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #71) > I am actually not sure of how try picks up Gaia, but here is the try push. > If this didn't include bug 1174062 I will push a new one. Try picked up Gaia commit 8583025b84cd and did not include bug 1174062, but it failed to produce the error. This is really mysterious...

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #72) > Try picked up Gaia commit 8583025b84cd and did not include bug 1174062, but > it failed to produce the error. This is really mysterious... Oh wait, it did at Gij-7 and Gij-8. I read the full log of Gij-11 wrong...

The Gaia change is represented in m-c as this changeset [1], which this try includes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=855246a10530 [1] https://hg.mozilla.org/integration/b2g-inbound/rev/27edf4123f38