1349255 - sending NotifyIMEFocus IPC messages can jank the browser

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Nathan Froyd [:froydnj]]

https://perfht.ml/2n457Ak shows a profile which has multiple Msg_NotifyIMEFocus hangs in it (~3s, ~1.5s, ~600ms).  Unfortunately, the stacks are complete garbage, for reasons that I do not understand--I think it has something to do with pending updates, though.  Is this a known sore spot for IPC messages?

Comment 1

[(no longer active)]

It's in the telemetry data but I don't think we have a bug on file for it, thanks for filing it.

The stacks are really surprising.  Markus, have you seen something like this?  We may wanna spin off the stack walking issue int a separate bug...

Comment 2

[Markus Stange [:mstange]]

I haven't seen this particular variant before, and I'm puzzled by it. Nathan, if you see this again, please file a bug.

Comment 3

[Andrew Overholt [:overholt]]

Masayuki, please take a look at this or delegate it when you have time. Thank you.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, I should do this. But I have an urgent bugs, one is a security bug and the others are improving performance issue of IMEContentObserver. I could take this after them.

However, I'm not sure making it async works without any regressions because user may type soon after moving focus, then, IME state may mismatch with the visual result.

Comment 5

[Florian Quèze [:florian]]

Is this profile helpful? https://perfht.ml/2oIdimx
It seems to have correct stacks.

Comment 6

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #4)
> Yeah, I should do this. But I have an urgent bugs, one is a security bug and
> the others are improving performance issue of IMEContentObserver. I could
> take this after them.
> 
> However, I'm not sure making it async works without any regressions because
> user may type soon after moving focus, then, IME state may mismatch with the
> visual result.

Hi Masayuki,

This bug seems to be clear enough for me to help you out. 

Regarding the mismatch issue, do you think if it would help to have a flag
to indicate if the result of "SendNotifyIMEFocus" has asynchronously
come out? If the result hasn't out, we then (unfortunately) call a 
sync function [1] to get the result and ignore the one on the way.

[1] The sync function may have to be something like "idempotent" (?).
    That is, calling SendNotifyIMEFocusAsync followed by SendNotifyIMEFocusSync
    should be equivalent to calling only SendNotifyIMEFocusAsync only.

Comment 7

[Henry Chang [:hchang]]

Attachment: Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review commit: https://reviewboard.mozilla.org/r/136444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136444/

Comment 8

[Henry Chang [:hchang]]

I submitted a patch which addresses this issue in two aspects:

1) Add PBrowser.AsyncNotifyIMEFocus, which is totally identical to 
   PBrowser.NotifyIMEFocus except that it returns the result
   asynchronously.

2) In order to avoid the mismatch issue, the result (mIMENotificationRequestsOfParent)
   will be obtained with PBrowser.NotifyIMEFocus again if the previous
   PBrowser.AsyncNotifyIMEFocus hasn't return the result.

Masayuki, 

Could you give me some feedback about my patch on the reviewboard?
I tested my patch basically and it seemed working fine. I've quickly
changed the editable focus and there seemed be no mismatch. But, I
am not sure if there is still anything that I wasn't aware since I am
not very familiar with this function.

Thanks :)

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm still thinking about bug, though, perhaps, the fix of bug 1217700 must help this bug.

After bug 1217700, IMEContentObserver started to observe any changes in focused editor. When it receives an change, it checks if the widget needs which notifications at that timing. So, perhaps, now, the IPC message doesn't need to be sync anymore.

When PuppetWidget::NotifyIMEOfFocusChange() is called, PuppetWidget should request all notifications until it receives actual notification requests of parent process. If parent process receives unnecessary notifications, it should just ignore them.

Comment 10

[Henry Chang [:hchang]]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136444/diff/1-2/

Comment 11

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #9)
> I'm still thinking about bug, though, perhaps, the fix of bug 1217700 must
> help this bug.
> 
> After bug 1217700, IMEContentObserver started to observe any changes in
> focused editor. When it receives an change, it checks if the widget needs
> which notifications at that timing. So, perhaps, now, the IPC message
> doesn't need to be sync anymore.
> 
> When PuppetWidget::NotifyIMEOfFocusChange() is called, PuppetWidget should
> request all notifications until it receives actual notification requests of
> parent process. If parent process receives unnecessary notifications, it
> should just ignore them.

Hi Masayuki-san,

If I am not misunderstanding your explanation, the patch I just asked for your 
review is supposed to work perfectly without the mismatch issue you mentioned.
Could you please help review that patch? Do you think if I should add any test
case to ensure the change doesn't screw up anything?

Thanks!

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/136444/#review140458

I'd like to check next version, r- until then.

::: dom/ipc/PBrowser.ipdl:228
(Diff revision 2)
> -    nested(inside_cpow) sync NotifyIMEFocus(ContentCache contentCache,
> +    async NotifyIMEFocus(ContentCache contentCache,
> -                                            IMENotification notification)
> +                              IMENotification notification)

nit: fix the indent if it's not a bug of reviewboard.

::: widget/PuppetWidget.cpp:847
(Diff revision 2)
>    } else {
>      // When IME loses focus, we don't need to store anything.
>      mContentCache.Clear();
>    }
>  
>    mIMENotificationRequestsOfParent = IMENotificationRequests();

mIMENotificationRequestsOfParent should request all notifications.

I think that IMENotificationRequests::Notifications should have NOTIFY_ALL whose value is (NOTIFY_TEXT_CHANGE | NOTIFY_POSITION_CHANGE | NOTIFY_MOUSE_BUTTON_EVENT_ON_CHAR). (NOTIFY_DURING_DEACTIVE isn't necessary even on Windows, perhaps.) And it should set to mIMENotificationRequestsOfParent before calling SendNotifyIMEFocus(). Then, it's guaranteed that all notifications are sent to parent process until receiving actual requests.

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136444/diff/2-3/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/136444/#review140576

::: widget/PuppetWidget.cpp:847
(Diff revision 3)
> -  mIMENotificationRequestsOfParent = IMENotificationRequests();
> -  if (!mTabChild->SendNotifyIMEFocus(mContentCache, aIMENotification,
> -                                     &mIMENotificationRequestsOfParent)) {
> +  auto NOTIFY_ALL = IMENotificationRequests::NOTIFY_TEXT_CHANGE |
> +                    IMENotificationRequests::NOTIFY_POSITION_CHANGE |
> +                    IMENotificationRequests::NOTIFY_MOUSE_BUTTON_EVENT_ON_CHAR;

No, I meant that you should add NOTIFY_ALL to the enum (declared in IMEData.h). Then, when the others add new notifications, they will modify NOTIFY_ALL too (perhaps).

Comment 15

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #14)
> Comment on attachment 8864779 [details]
> Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.
> 
> https://reviewboard.mozilla.org/r/136444/#review140576
> 
> ::: widget/PuppetWidget.cpp:847
> (Diff revision 3)
> > -  mIMENotificationRequestsOfParent = IMENotificationRequests();
> > -  if (!mTabChild->SendNotifyIMEFocus(mContentCache, aIMENotification,
> > -                                     &mIMENotificationRequestsOfParent)) {
> > +  auto NOTIFY_ALL = IMENotificationRequests::NOTIFY_TEXT_CHANGE |
> > +                    IMENotificationRequests::NOTIFY_POSITION_CHANGE |
> > +                    IMENotificationRequests::NOTIFY_MOUSE_BUTTON_EVENT_ON_CHAR;
> 
> No, I meant that you should add NOTIFY_ALL to the enum (declared in
> IMEData.h). Then, when the others add new notifications, they will modify
> NOTIFY_ALL too (perhaps).

Got it! I'll do it tomorrow.

BTW, I just saw a bunch of testing failures from the try result 
and about to drop the review :p

Thanks :)

Comment 16

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #14)
> Comment on attachment 8864779 [details]
> Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.
> 
> https://reviewboard.mozilla.org/r/136444/#review140576
> 
> ::: widget/PuppetWidget.cpp:847
> (Diff revision 3)
> > -  mIMENotificationRequestsOfParent = IMENotificationRequests();
> > -  if (!mTabChild->SendNotifyIMEFocus(mContentCache, aIMENotification,
> > -                                     &mIMENotificationRequestsOfParent)) {
> > +  auto NOTIFY_ALL = IMENotificationRequests::NOTIFY_TEXT_CHANGE |
> > +                    IMENotificationRequests::NOTIFY_POSITION_CHANGE |
> > +                    IMENotificationRequests::NOTIFY_MOUSE_BUTTON_EVENT_ON_CHAR;
> 
> No, I meant that you should add NOTIFY_ALL to the enum (declared in
> IMEData.h). Then, when the others add new notifications, they will modify
> NOTIFY_ALL too (perhaps).

By the way, do you think if we can just set NOTIFY_ALL to 0xFF so that
we would never miss any newly added notifications?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't think so. Like NOTIFY_DURING_DEACTIVE, new flag could have different meaning. (It requests notifications even during the window deactivated, that means that composition should not be committed at inactivating the window, but on some platforms, it should be committed due to native IME behavior's limitation.)

Comment 18

[Henry Chang [:hchang]]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136444/diff/3-4/

Comment 19

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #17)
> I don't think so. Like NOTIFY_DURING_DEACTIVE, new flag could have different
> meaning. (It requests notifications even during the window deactivated, that
> means that composition should not be committed at inactivating the window,
> but on some platforms, it should be committed due to native IME behavior's
> limitation.)

Does that mean NOTIFY_ALL may end up being platform specific?

Sorry I am super unfamiliar with IME stuff and am reading [1].
Besides, do you think if we should update that document when
this bug is done?

Thanks :)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Henry Chang [:hchang] from comment #19)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #17)
> > I don't think so. Like NOTIFY_DURING_DEACTIVE, new flag could have different
> > meaning. (It requests notifications even during the window deactivated, that
> > means that composition should not be committed at inactivating the window,
> > but on some platforms, it should be committed due to native IME behavior's
> > limitation.)
> 
> Does that mean NOTIFY_ALL may end up being platform specific?

Could be, but perhaps, it's not necessary. So, I think that just ignoring NOTIFY_DURING_DEACTIVE is enough for now. (I'll test it after landing your patch.)

> Sorry I am super unfamiliar with IME stuff and am reading [1].
> Besides, do you think if we should update that document when
> this bug is done?
> 
> Thanks :)
> 
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide

Ah, the document is already old. I'll update it when I have much time, don't worry.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/136444/#review141932

Oops, sorry, I forgot to check your review request. Thank you for your work!

Comment 22

[Henry Chang [:hchang]]

The patch itself and the test cases look good but they suffer from
the bug where the PuppetWidget might receive the async IPDL Promise
reply even if the actor (mTabChild) has been destroyed. This is the 
fundamental bug of IPDL async returned Promise and I will try to solve
that in bug 1364857.

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136444/diff/4-5/

Comment 24

[Henry Chang [:hchang]]

Even thought bug 1364857 has been fixed, I am still stuck in 

http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/xpcom/threads/MozPromise.h#444

where the runnable for Reject() (on the sender side) will never be executed 
in the shutdown phase for unknown reason. If for some reason the runnable 
cannot be executed, the Promise will never be released. Need to investigate 
what's actually going on behind the scenes. Probably mRespondTarget has
been destroyed to reject new dispatched runnable or something like that.
I am guessing this might happen in the shut down phase.

Comment 25

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=450bb9593f4a

Comment 26

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c40c9182c0

Comment 27

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df6f0b2d0ee

Comment 28

[Henry Chang [:hchang]]

It turns out I ended up failing to solve the following issue:

"GECKO(45210) | ###!!! [Child][DispatchAsyncMessage] Error: PBrowser::Reply_NotifyIMEFocus Route error: message sent to unknown actor ID"

What I expected is

TabParent                           | TabChild
----------------------------------------------------------------------
- RecvNotifyIMEFocus                  |
    NotifyIMEFocusPromise->Resolve    | 
      Reply_NotifyIMEFocus()          |
                                      | Received "Reply_NotifyIMEFocus"
- Destroy()                           |  
    SendDestroy()                     |
                                      | RecvSendDestroy()
                                      |


What it actually is

TabParent                             | TabChild
----------------------------------------------------------------------
- RecvNotifyIMEFocus                  |
    NotifyIMEFocusPromise->Resolve    | 
                                      |
                                      | 
- Destroy()                           |  
    SendDestroy()                     |
                                      | RecvSendDestroy()
  !!!Reply_NotifyIMEFocus()           | 
                                      | Received "Reply_NotifyIMEFocus"

As you can see, even though we didn't *asynchronously* resolve the promise,
it's resolver is still be run asynchronously (because of [1])

I can either 

1) Give up using async IPDL promise
2) Instead of handing over the promise to the handler, we ask the sender
   to set a resolver/rejecter callback, which will be *synchronously* 
   executed. This approach has to be carefully designed to not leave us
   unable to reply the result asynchronously.

Kanru, what do you think of option 2? That might require the IPDL layer
on the receiver side to maintain a list of pending Promise as we do on
the sender side.


[1] http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/xpcom/threads/MozPromise.h#444

Comment 29

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Henry Chang [:hchang] from comment #28)
> It turns out I ended up failing to solve the following issue:
> 
> "GECKO(45210) | ###!!! [Child][DispatchAsyncMessage] Error:
> PBrowser::Reply_NotifyIMEFocus Route error: message sent to unknown actor ID"
> 
> What I expected is
> 
> TabParent                           | TabChild
> ----------------------------------------------------------------------
> - RecvNotifyIMEFocus                  |
>     NotifyIMEFocusPromise->Resolve    | 
>       Reply_NotifyIMEFocus()          |
>                                       | Received "Reply_NotifyIMEFocus"
> - Destroy()                           |  
>     SendDestroy()                     |
>                                       | RecvSendDestroy()
>                                       |
> 
> 
> What it actually is
> 
> TabParent                             | TabChild
> ----------------------------------------------------------------------
> - RecvNotifyIMEFocus                  |
>     NotifyIMEFocusPromise->Resolve    | 
>                                       |
>                                       | 
> - Destroy()                           |  
>     SendDestroy()                     |
>                                       | RecvSendDestroy()
>   !!!Reply_NotifyIMEFocus()           | 
>                                       | Received "Reply_NotifyIMEFocus"
> 
> As you can see, even though we didn't *asynchronously* resolve the promise,
> it's resolver is still be run asynchronously (because of [1])
> 
> I can either 
> 
> 1) Give up using async IPDL promise
> 2) Instead of handing over the promise to the handler, we ask the sender
>    to set a resolver/rejecter callback, which will be *synchronously* 
>    executed. This approach has to be carefully designed to not leave us
>    unable to reply the result asynchronously.
> 
> Kanru, what do you think of option 2? That might require the IPDL layer
> on the receiver side to maintain a list of pending Promise as we do on
> the sender side.
> 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 35b37316149108d53a02fb8498e250ea8a22ab5d/xpcom/threads/MozPromise.h#444

It's expected that resolving a MozPromise will need a trip to the event queue. If MozPromise had microtask semantic then this should work as expected.

I think for now for safety we can require the Actor to inherit from SupportsWeakPtr so we can check if the Actor is still alive.

Comment 30

[(no longer active)]

(This was showing up in one of the inferno speedometer v2 profiles I was looking at, for example see https://perfht.ml/2qkezAq)

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-WIP-Do-not-send-reply-if-actor-is-dead.patch

Henry, could you try this patch?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4516f86851eee4c5449ce5ea0b17fc66e95dd9ec

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #31)
> Created attachment 8871388 [details] [diff] [review]
> 0001-WIP-Do-not-send-reply-if-actor-is-dead.patch
> 
> Henry, could you try this patch?
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4516f86851eee4c5449ce5ea0b17fc66e95dd9ec

Hmm, this is not useful because SendDestroy does not actually destroy the Actor.

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

I'm writing a patch to make the resolver promise a resolver function and remove the ability to reject. I think it actually makes the recv function looks better.

Comment 34

[Kan-Ru Chen [:kanru] (UTC+9)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=844785c986679443d0ae04ec5477cbc71e08983f

Comment 35

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c5f14b6395

Comment 36

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12acd78ed6e7

Comment 37

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1522d0aad4

Comment 38

[Henry Chang [:hchang]]

Comment on attachment 8864779 [details]
Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136444/diff/5-6/

Comment 39

[Henry Chang [:hchang]]

Some updates lately:

1) After I landed bug 1364857, I still got a bunch of try failures due to
   what I mentioned in comment 23. So, I decided not to use Promise but
   a pair of async Notify/Reply IPC messages.

2) However, there's at least one test case (browser_master_password_autocomplete.js)
   which permanently fails. The error message is like: [1]

[task 2017-06-07T06:29:02.292567Z] 06:29:02     INFO - Console message: [JavaScript Error: "1496816612127	Browser.Experiments.Experiments	ERROR	Experiments #0::httpGetRequest::onLoad() - Request to http://127.0.0.1:8888/experiments-dummy/manifest returned status 404" {file: "resource://gre/modules/Log.jsm" line: 748}]
[task 2017-06-07T06:29:02.295813Z] 06:29:02     INFO - App_append@resource://gre/modules/Log.jsm:748:9
[task 2017-06-07T06:29:02.297729Z] 06:29:02     INFO - log@resource://gre/modules/Log.jsm:386:7
[task 2017-06-07T06:29:02.300138Z] 06:29:02     INFO - getLoggerWithMessagePrefix/proxy.log@resource://gre/modules/Log.jsm:501:44
[task 2017-06-07T06:29:02.302777Z] 06:29:02     INFO - Experiments.Experiments/this._log.log@resource:///modules/experiments/Experiments.jsm:320:5
[task 2017-06-07T06:29:02.304637Z] 06:29:02     INFO - error@resource://gre/modules/Log.jsm:394:5
[task 2017-06-07T06:29:02.307061Z] 06:29:02     INFO - _httpGetRequest/</xhr.onload@resource:///modules/experiments/Experiments.jsm:951:11
[task 2017-06-07T06:29:02.310178Z] 06:29:02     INFO - openModalWindow@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:363:5
[task 2017-06-07T06:29:02.312132Z] 06:29:02     INFO - openPrompt@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:553:9
[task 2017-06-07T06:29:02.314393Z] 06:29:02     INFO - nsIPrompt_promptPassword@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:771:9
[task 2017-06-07T06:29:02.317562Z] 06:29:02     INFO - promptPassword@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:584:20
[task 2017-06-07T06:29:02.319712Z] 06:29:02     INFO - decrypt@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/crypto-SDR.js:119:20
[task 2017-06-07T06:29:02.322513Z] 06:29:02     INFO - _decryptLogins@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/storage-json.js:492:26
[task 2017-06-07T06:29:02.326262Z] 06:29:02     INFO - searchLogins@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/storage-json.js:267:14
[task 2017-06-07T06:29:02.328568Z] 06:29:02     INFO - searchLogins@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsLoginManager.js:411:12
[task 2017-06-07T06:29:02.331091Z] 06:29:02     INFO - searchLoginsWithObject@resource://gre/modules/LoginHelper.jsm:171:12
[task 2017-06-07T06:29:02.333214Z] 06:29:02     INFO - _searchAndDedupeLogins@resource://gre/modules/LoginManagerParent.jsm:45:16
[task 2017-06-07T06:29:02.336535Z] 06:29:02     INFO - doAutocompleteSearch@resource://gre/modules/LoginManagerParent.jsm:282:16
[task 2017-06-07T06:29:02.338526Z] 06:29:02     INFO - receiveMessage@resource://gre/modules/LoginManagerParent.jsm:123:9
[task 2017-06-07T06:29:02.342133Z] 06:29:02     INFO - receiveMessage@jar:file:///home/worker/workspace/build/application/firefox/browser/omni.ja!/components/nsBrowserGlue.js:194:15

which is a "404 Not Found" error while loading http://127.0.0.1:8888/experiments-dummy/manifest
I wonder if it's because the child sent unexpected event to parent
in the state. (e.g. after Destroy()'ed)

Hi Masayuki san,

Do you have any suggestion for me to debug this kind of error?
I have been stumbled by this for a couple of days and still
have no clue :( The new approach (the Notify/Reply) has been
submitted to the reviewboard. 

Thanks!


[1] https://public-artifacts.taskcluster.net/Y5lZrzoXQhKN5Sgt3DDEYg/0/public/logs/live_backing.log

Comment 40

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Henry Chang [:hchang] from comment #39)
> Some updates lately:
> 
> 1) After I landed bug 1364857, I still got a bunch of try failures due to
>    what I mentioned in comment 23. So, I decided not to use Promise but
>    a pair of async Notify/Reply IPC messages.

I'm interested to know the difference between a manual Notify/Reply message and the IPC promise approach. In theory they should behave the same because the IPC promise is implemented by a pair of async messages. Did bug 1368031 change anything?

Comment 41

[Henry Chang [:hchang]]

I also think they makes no difference :p I was just trying to isolate any potential
issues first. But sadly I have still been stuck in the try test case failure for
about a week...

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to reply. But I have no idea. I'm not familiar with it.

Let's ask it to the original author of the test.

johannh:

Do you have any ideas about the 404 error mentioned in comment 39?

Comment 43

[Johann Hofmann [:johannh]]

I don't think the 404 error is the source of failure here. That seems like random noise to me (someone should look into this). 

This test is a bit flaky with timing (it's hard to get right, it's testing a timing issue). The test uses .focus() in a time-sensitive way (has to occur faster than 3s). Could that be a problem? You could try increasing these two values to find out if this is about the focus not happening fast enough https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js#34,49

Feel free to hit me up on IRC to figure this out.

Comment 44

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5739852bf3

Comment 45

[Henry Chang [:hchang]]

(In reply to Johann Hofmann [:johannh] from comment #43)
> I don't think the 404 error is the source of failure here. That seems like
> random noise to me (someone should look into this). 
> 
> This test is a bit flaky with timing (it's hard to get right, it's testing a
> timing issue). The test uses .focus() in a time-sensitive way (has to occur
> faster than 3s). Could that be a problem? You could try increasing these two
> values to find out if this is about the focus not happening fast enough
> https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/
> test/browser/browser_master_password_autocomplete.js#34,49
> 
> Feel free to hit me up on IRC to figure this out.

Hi Johnna,

Thanks for your information  :)

I tried have longer timeouts but they are still permanent failures. So I would
think it might be some other fundamental bugs. I am trying to find clues from
other test failures. (yes there is not just this very test failure ^^")

Masayuki san,

I found in TabParent we might need to ignore unexpected events from child [1].
Not sure if they are crucial enough to lead to such many testing failures [2].
What do you think? I have kicked off a try build to see if my findings matters.

Thanks!


[1] http://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1711,1727
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5739852bf3

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I found in TabParent we might need to ignore unexpected events from child [1].

Ah, yes. With your patch, actual request and notifications may mismatch. So, please remove the check.

> Not sure if they are crucial enough to lead to such many testing failures [2].

Looks like that these oranges are high-frequent oranges (I also have some trouble with these oranges now). How about to rebase and retry?

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Hmm, does bug 1375491 end up changing the timing here too?

Comment 48

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00313bee6ff0

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

While I'm investigating bug 1377672, I realized that IMEStateManager in the main process doesn't handle delayed notifications and requests to IME properly. So, it might cause the odd oranges.

Comment 50

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #49)
> While I'm investigating bug 1377672, I realized that IMEStateManager in the
> main process doesn't handle delayed notifications and requests to IME
> properly. So, it might cause the odd oranges.

Hi Masayuki san,

So do have any suggestion for me? Have that issue been fixed by bug 1377672 or
still a thing? Thanks!

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I meant that those issues which I found were already fixed by bug 1377672.

Comment 52

[Henry Chang [:hchang]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #51)
> I meant that those issues which I found were already fixed by bug 1377672.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c0a2adef6683000d98e4d5a68f9317754903248

Have triggered a new try run but those failures are still present.
(plus new failures ^^") 

BTW, according to https://bugzilla.mozilla.org/show_bug.cgi?id=1377672#c41
the intermittent failure that bug 1377672 is trying to resolve is
still very frequent. Am I missing something?

Comment 53

[(no longer active)]

Hi Henry, any updates here?  This is still showing up in Speedometer profiles, it would be nice if we can fix it soon.  Thanks!

Comment 54

[Henry Chang [:hchang]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #53)
> Hi Henry, any updates here?  This is still showing up in Speedometer
> profiles, it would be nice if we can fix it soon.  Thanks!

I still cannot solve the try-only failures :(
(even on top of Masayuki san's fix in bug 1377672)

I might have to investigate IME handling in e10s mode much deeper.
(maybe start from https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide)

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Henry Chang [:hchang] from comment #54)
> I might have to investigate IME handling in e10s mode much deeper.
> (maybe start from
> https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide)

That is perhaps not updated for the latest design, but it must help you to understand the idea of e10s support.

Comment 56

[Samael Wang [:freesamael] (away for now)]

I'll try to continue the investigation on top of Henry's effort.

Comment 57

[Ethan Tseng [:ethan]]

(In reply to Samael Wang [:freesamael] from comment #56)
> I'll try to continue the investigation on top of Henry's effort.

Thank you for taking this, Samael!

Comment 58

[Samael Wang [:freesamael] (away for now)]

Some updates about the failed test case browser_master_password_autocomplete.js:

While I haven't figured out why the visible symptom is that 2 prompts left on screen [1], I've found the main reason the test case fails is that click [2] doesn't make the prompt closed immediately. nsGlobalWindow::ReallyCloseWindow of the 1st prompt can happen after the focus ContentTask [3] gets resolved, and it's because FinalClose [4] would make a runnable scheduled twice before execution, and Henry's patch has made focus() on input elements not being blocked by NotifyIMEFocus. 

The point of master password canceling time being recorded is after the prompt actually closed [5], and since the 4s waiting can start before previous prompt being closed, it easily gets failed on slow machines or on try server (I can easily reproduce it when using --debugger=rr).

I tried a simple fix by waiting for "DOMModalDialogClosed" event, and that runs much better on try.
5 runs with original test case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c167ec8ce6be321e18f8ff1a48cd8c158aaef08e&selectedJob=123488572
20 runs with "DOMModalDialogClosed" fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b145f79b915221415500f541b3c7b741ec5c7b02&group_state=expanded

I'm inclined to make the test case even less time-sensitive and hopefully prevent intermittent failures. I'm still verifying the patch:
https://hg.mozilla.org/try/comparison/278ca9dad347/toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js

[1] https://public-artifacts.taskcluster.net/F_5bfXqiQDynIn62aOyl5g/0/public/test_info//mozilla-test-fail-screenshot_c2NfDM.png
[2] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js#12
[3] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js#43-46
[4] http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/dom/base/nsGlobalWindow.cpp#9444-9447
[5] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/components/passwordmgr/LoginManagerParent.jsm#55

Comment 59

[Samael Wang [:freesamael] (away for now)]

(In reply to Samael Wang [:freesamael] from comment #58)
> Some updates about the failed test case
> browser_master_password_autocomplete.js:
> 
> While I haven't figured out why the visible symptom is that 2 prompts left
> on screen

FWIW, this is what rr told me:

1. The 1st `await dialogShown` clicked cancel button, which eventually called nsCloseEvent::PostCloseEvent.
2. nsCloseEvent::Run executed, which PostCloseEvent again.
3. TabParent::RecvNotifyIMEFocus got called. Probably because the prompt still exists, it fell to !IsSameProcess(aTabParent, sActiveTabParent) [1] and got ignored.
4. Received message "content-task:complete".
5. Received message "RemoteLogins:autoCompleteLogins", which triggered another promt.

The eventual js stack:

(rr) call js::DumpBacktrace((JSContext*)0x7f1db41c8000)
#0   7f1d7d558190 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:363 (7f1da7f9d848 @ 186)
#1   7f1d7d5580b8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:573 (7f1da7f9de38 @ 525)
#2   7f1d7d558020 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:791 (7f1da7fa2550 @ 142)
#3   7f1dad7ebf38 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:604 (7f1da7fa2090 @ 56)
#4   7f1dad7ebe78 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/crypto-SDR.js:119 (7f1d908a7d08 @ 89)
#5   7f1dad7ebdb0 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/storage-json.js:492 (7f1d908a7718 @ 130)
#6   7f1dad7ebce8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/storage-json.js:267 (7f1d9089ff68 @ 435)
#7   7f1dad7ebc58 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsLoginManager.js:409 (7f1d90885a10 @ 219)
#8   7f1dad7ebbb8 i   resource://gre/modules/LoginHelper.jsm:171 (7f1d90892aa8 @ 45)
#9   7f1dad7ebb00 i   resource://gre/modules/LoginManagerParent.jsm:45 (7f1d9c9e8680 @ 71)
#10   7f1dad7eb9f8 i   resource://gre/modules/LoginManagerParent.jsm:282 (7f1d9c9e8a10 @ 475)
#11   7f1dad7eb930 i   resource://gre/modules/LoginManagerParent.jsm:123 (7f1d9c9e87b0 @ 396)
#12   7ffcf7eb94c0 b   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/browser/components/nsBrowserGlue.js:182 (7f1dab21e420 @ 120)
#13   7f1dad7eb868 i   self-hosted:995 (7f1da3797a10 @ 407)
#14   7f1dad7eb7c0 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:363 (7f1da7f9d848 @ 186)
#15   7f1dad7eb6e8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:573 (7f1da7f9de38 @ 525)
#16   7f1dad7eb650 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:791 (7f1da7fa2550 @ 142)
#17   7f1dad7eb5b8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsPrompter.js:604 (7f1da7fa2090 @ 56)
#18   7f1dad7eb4f8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/crypto-SDR.js:119 (7f1d908a7d08 @ 89)
#19   7f1dad7eb430 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/storage-json.js:492 (7f1d908a7718 @ 130)
#20   7f1dad7eb368 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/storage-json.js:267 (7f1d9089ff68 @ 435)
#21   7f1dad7eb2d8 i   file:///home/freesamael/Repos/build/obj-firefox-icecc-disable-optimize/dist/bin/components/nsLoginManager.js:409 (7f1d90885a10 @ 219)
#22   7f1dad7eb238 i   resource://gre/modules/LoginHelper.jsm:171 (7f1d90892aa8 @ 45)
#23   7f1dad7eb180 i   resource://gre/modules/LoginManagerParent.jsm:45 (7f1d9c9e8680 @ 71)
#24   7f1dad7eb0f0 i   resource://gre/modules/LoginManagerParent.jsm:234 (7f1d9c9e88e0 @ 564)
#25   7f1dad7eb020 i   self-hosted:1187 (7f1d909e02f0 @ 173)

[1] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/events/IMEStateManager.cpp#1567

Comment 60

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review commit: https://reviewboard.mozilla.org/r/170136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170136/

Comment 61

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button.

Review commit: https://reviewboard.mozilla.org/r/170138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170138/

Comment 62

[Samael Wang [:freesamael] (away for now)]

Hi Masayuki,

I'm taking over the bug from Henry, but since MozReview wouldn't let me just "inherit" the review, I have to ask for your review again. The patch is exactly the same as Henry's one.

Comment 63

[Matthew N. [:MattN]]

Comment on attachment 8898723 [details]
Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button.

https://reviewboard.mozilla.org/r/170138/#review175344

Thanks

Comment 64

[Samael Wang [:freesamael] (away for now)]

Now I'm hitting this assertion on multiple test cases on try run:

[task 2017-08-18T11:45:26.338366Z] 11:45:26     INFO - [Parent 1015] ###!!! ASSERTION: Don't call Send/RecvNotifyIMETextChange without NOTIFY_TEXT_CHANGE: 'requests.WantTextChange()', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1812
[task 2017-08-18T11:45:26.339503Z] 11:45:26     INFO - #01: mozilla::dom::TabParent::RecvNotifyIMETextChange [dom/ipc/TabParent.cpp:1811]

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to review it. I'll check this tomorrow. (hopefully!)

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/170136/#review176118

I only see small issues in this patch, but according to your comment in bugzilla, this is still not available. So, -'ing for now.

::: commit-message-a6a1f:1
(Diff revision 1)
> +Bug 1349255 - Make PBrowser.NotifyIMEFocus async to avoid UI jank.
> +
> +MozReview-Commit-ID: 15eUwMJ2Q7H

Ah, could you explain the detail of the reason of this change in this commit message?

::: dom/ipc/PBrowser.ipdl:212
(Diff revision 1)
>       *
>       *  contentCache Cache of content
>       *  notification Whole data of the notification
>       *  requests     Requests of notification for IME of the native widget
>       */
> -    nested(inside_cpow) sync NotifyIMEFocus(ContentCache contentCache,
> +    async NotifyIMEFocus(ContentCache contentCache, IMENotification notification);

This line is over 80 characters. Could you break after the |,| like before you change?

::: dom/ipc/TabChild.h:449
(Diff revision 1)
> +  virtual mozilla::ipc::IPCResult
> +  RecvReplyIMENotificationRequests(const IMENotificationRequests& aRequests) override;

Over 80 characters. Please wrap the line after |(|.

::: dom/ipc/TabChild.cpp:2221
(Diff revision 1)
>                       aMessage, false, &data, &cpows, aPrincipal, nullptr);
>    return IPC_OK();
>  }
>  
>  mozilla::ipc::IPCResult
> +TabChild::RecvReplyIMENotificationRequests(const IMENotificationRequests& aRequests)

Over 80 characters, please break this line after the |(|.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Samael Wang [:freesamael] from comment #64)
> Now I'm hitting this assertion on multiple test cases on try run:
> 
> [task 2017-08-18T11:45:26.338366Z] 11:45:26     INFO - [Parent 1015] ###!!!
> ASSERTION: Don't call Send/RecvNotifyIMETextChange without
> NOTIFY_TEXT_CHANGE: 'requests.WantTextChange()', file
> /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1812
> [task 2017-08-18T11:45:26.339503Z] 11:45:26     INFO - #01:
> mozilla::dom::TabParent::RecvNotifyIMETextChange [dom/ipc/TabParent.cpp:1811]

This detects new regression correctly.

That means that when the TabParent receives text change notification (and also other change notifications), IMEStateManager has already sent blur notification to the widget (in the main process). This can occur when focus is moved from a remote process to chrome or another tab. In such case, IMEStateManager in the main process generates blur notification to widget:
https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/events/IMEStateManager.cpp#526-542
In this case, IMEStateManager will call TabParent::SendStopIMEStateManagement() immediately:
https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/events/IMEStateManager.cpp#553-558
I think that TabParent should override SendStopIMEStateManagement() and start ignoring any notifications from the remote process except new focus notification.  Then, you won't hit the assertions.

FYI: IMEStateManager will ignore focus notification which came after the TabParent loses focus. So, perhaps, you don't need to worry about outdated focus notification.

Comment 68

[Samael Wang [:freesamael] (away for now)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #67)
> (In reply to Samael Wang [:freesamael] from comment #64)
> > Now I'm hitting this assertion on multiple test cases on try run:
> > 
> > [task 2017-08-18T11:45:26.338366Z] 11:45:26     INFO - [Parent 1015] ###!!!
> > ASSERTION: Don't call Send/RecvNotifyIMETextChange without
> > NOTIFY_TEXT_CHANGE: 'requests.WantTextChange()', file
> > /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1812
> > [task 2017-08-18T11:45:26.339503Z] 11:45:26     INFO - #01:
> > mozilla::dom::TabParent::RecvNotifyIMETextChange [dom/ipc/TabParent.cpp:1811]
> 
> This detects new regression correctly.
> 
> That means that when the TabParent receives text change notification (and
> also other change notifications), IMEStateManager has already sent blur
> notification to the widget (in the main process). This can occur when focus
> is moved from a remote process to chrome or another tab. In such case,
> IMEStateManager in the main process generates blur notification to widget:
> https://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/dom/events/IMEStateManager.cpp#526-
> 542
> In this case, IMEStateManager will call
> TabParent::SendStopIMEStateManagement() immediately:
> https://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/dom/events/IMEStateManager.cpp#553-
> 558
> I think that TabParent should override SendStopIMEStateManagement() and
> start ignoring any notifications from the remote process except new focus
> notification.  Then, you won't hit the assertions.
> 
> FYI: IMEStateManager will ignore focus notification which came after the
> TabParent loses focus. So, perhaps, you don't need to worry about outdated
> focus notification.

Wasn't this because of initial NOTIFY_ALL? Henry hinted me that this problem had been mentioned in comment 45. I'll take a look to check if this is a real regression.

Comment 69

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8871388 [details] [diff] [review]
0001-WIP-Do-not-send-reply-if-actor-is-dead.patch

This has been landed in bug 1368031. I'm going to try MozPromise again and see how it plays this time.

Comment 70

[Samael Wang [:freesamael] (away for now)]

(In reply to Samael Wang [:freesamael] from comment #68)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #67)
> > That means that when the TabParent receives text change notification (and
> > also other change notifications), IMEStateManager has already sent blur
> > notification to the widget (in the main process). 
> 
> Wasn't this because of initial NOTIFY_ALL? Henry hinted me that this problem
> had been mentioned in comment 45. I'll take a look to check if this is a
> real regression.

I made a try run ignoring all NotifyIME* if `!IMEStateManager::DoesTabParentHaveIMEFocus(this)`:
https://hg.mozilla.org/try/rev/c9d6b729d7457ce8ef8032882e35e0be5493ce84
And that still hits the assertion.

My local build also shows it can happen in the following sequence:
1. TabParent::RecvRequestFocus
2. TabParent::RecvNotifyIMEFocus
3. TabParent::RecvNotifyIMETextChange => hit assertion

So I think it's because of the NOTIFY_ALL, or at least it can be hit in this case.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> > I found in TabParent we might need to ignore unexpected events from child [1].
> 
> Ah, yes. With your patch, actual request and notifications may mismatch. So,
> please remove the check.

Does this suggestion still apply? I'm inclined to remove the assertion, and also ignore notifications if `!IMEStateManager::DoesTabParentHaveIMEFocus(this)`.

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Samael Wang [:freesamael] from comment #70)
> (In reply to Samael Wang [:freesamael] from comment #68)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #67)
> > > That means that when the TabParent receives text change notification (and
> > > also other change notifications), IMEStateManager has already sent blur
> > > notification to the widget (in the main process). 
> > 
> > Wasn't this because of initial NOTIFY_ALL? Henry hinted me that this problem
> > had been mentioned in comment 45. I'll take a look to check if this is a
> > real regression.
> 
> I made a try run ignoring all NotifyIME* if
> `!IMEStateManager::DoesTabParentHaveIMEFocus(this)`:
> https://hg.mozilla.org/try/rev/c9d6b729d7457ce8ef8032882e35e0be5493ce84
> And that still hits the assertion.
> 
> My local build also shows it can happen in the following sequence:
> 1. TabParent::RecvRequestFocus
> 2. TabParent::RecvNotifyIMEFocus
> 3. TabParent::RecvNotifyIMETextChange => hit assertion
> 
> So I think it's because of the NOTIFY_ALL, or at least it can be hit in this
> case.

Ah, I see. Probably, yes, you're right.

Then, we should ignore the unnecessary notifications instead of using MOZ_ASSERT.

> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> > > I found in TabParent we might need to ignore unexpected events from child [1].
> > 
> > Ah, yes. With your patch, actual request and notifications may mismatch. So,
> > please remove the check.
> 
> Does this suggestion still apply? I'm inclined to remove the assertion, and
> also ignore notifications if
> `!IMEStateManager::DoesTabParentHaveIMEFocus(this)`.

Exactly.

Comment 72

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170136/diff/1-2/

Comment 73

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898723 [details]
Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170138/diff/1-2/

Comment 74

[Samael Wang [:freesamael] (away for now)]

I changed NotifyIMEFocus back to the original nested level |nested(inside_cpow)|, and re-adopted mozPromise for slightly better clean code. Try running at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3524ea62820cddcaac7681802481fe4402a9812

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/170136/#review176740

Comment 76

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170136/diff/2-3/

Comment 77

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898723 [details]
Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170138/diff/2-3/

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

freesamael: Why don't you land the patches?

Comment 79

[Samael Wang [:freesamael] (away for now)]

I was waiting for another try run. 

There were quite a few noises on try so judging whether the patch would cause intermittent failures is a bit difficult. Trying to play safe, I rebased the patch on latest m-c, compared it with a before-patch try, and look tho these oranges to see if they're suspicious.

Somehow macOS build machine seems to be extremely busy on try it took a whole day to complete all jobs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b83f99ea939ffb6069523473de82a1391a29b1c&group_state=expanded

It overall looks OK to me, so we can try to land it now.

Comment 80

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 9 changes to 9 files
remote: 
remote: sync-messages.ini altered in changeset 4ff6f5942e82 without IPC peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
remote: This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote

Comment 81

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

https://reviewboard.mozilla.org/r/170136/#review177968

Comment 82

[Kan-Ru Chen [:kanru] (UTC+9)]

Sigh, you need a IPC peer to give r+ but I'm not sure how to do that in mozreview

Comment 83

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898722 [details]
Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170136/diff/3-4/

Comment 84

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8898723 [details]
Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170138/diff/3-4/

Comment 85

[Samael Wang [:freesamael] (away for now)]

Updated commit message. That should work. Thanks Kanru!

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e03ce515252
Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank. r=kanru,masayuki
https://hg.mozilla.org/integration/autoland/rev/0c78ece467a8
Part 2: Wait for DOMModalDialogClosed after clicked cancel button. r=MattN

Comment 87

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8e03ce515252
https://hg.mozilla.org/mozilla-central/rev/0c78ece467a8