Closed
Bug 1349255
Opened 8 years ago
Closed 7 years ago
sending NotifyIMEFocus IPC messages can jank the browser
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: froydnj, Assigned: freesamael)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
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?
Flags: needinfo?(ehsan)
Comment 1•8 years ago
|
||
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...
Blocks: SyncIPC
Flags: needinfo?(ehsan) → needinfo?(mstange)
Comment 2•8 years ago
|
||
I haven't seen this particular variant before, and I'm puzzled by it. Nathan, if you see this again, please file a bug.
Flags: needinfo?(mstange)
Comment 3•8 years ago
|
||
Masayuki, please take a look at this or delegate it when you have time. Thank you.
Flags: needinfo?(masayuki)
Comment 4•8 years ago
|
||
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.
Flags: needinfo?(masayuki)
Updated•8 years ago
|
Priority: -- → P2
Comment 5•8 years ago
|
||
Is this profile helpful? https://perfht.ml/2oIdimx It seems to have correct stacks.
Updated•8 years ago
|
Whiteboard: [qf]
Updated•8 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 6•8 years ago
|
||
(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.
Flags: needinfo?(masayuki)
Updated•7 years ago
|
Assignee: nobody → hchang
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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.
Flags: needinfo?(masayuki)
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8864779 -
Flags: review?(masayuki)
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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.
Attachment #8864779 -
Flags: review?(masayuki) → review-
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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).
Attachment #8864779 -
Flags: review?(masayuki) → review-
Comment 15•7 years ago
|
||
(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•7 years ago
|
||
(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?
Flags: needinfo?(masayuki)
Comment 17•7 years ago
|
||
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.)
Flags: needinfo?(masayuki)
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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!
Attachment #8864779 -
Flags: review?(masayuki) → review+
Comment 22•7 years ago
|
||
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.
No longer depends on: 1364857
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
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 28•7 years ago
|
||
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
Flags: needinfo?(kchen)
Comment 29•7 years ago
|
||
(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.
Flags: needinfo?(kchen)
Updated•7 years ago
|
Blocks: Speedometer_V2
Comment 30•7 years ago
|
||
(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•7 years ago
|
||
Henry, could you try this patch? https://treeherder.mozilla.org/#/jobs?repo=try&revision=4516f86851eee4c5449ce5ea0b17fc66e95dd9ec
Attachment #8871388 -
Flags: feedback?(hchang)
Comment 32•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=844785c986679443d0ae04ec5477cbc71e08983f
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
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
Flags: needinfo?(masayuki)
Comment 40•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
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?
Flags: needinfo?(masayuki) → needinfo?(jhofmann)
Comment 43•7 years ago
|
||
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.
Flags: needinfo?(jhofmann)
Comment 45•7 years ago
|
||
(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
Updated•7 years ago
|
Flags: needinfo?(masayuki)
Comment 46•7 years ago
|
||
> 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?
Flags: needinfo?(masayuki)
Comment 49•7 years ago
|
||
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•7 years ago
|
||
(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!
Flags: needinfo?(masayuki)
Comment 51•7 years ago
|
||
I meant that those issues which I found were already fixed by bug 1377672.
Flags: needinfo?(masayuki)
Comment 52•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
(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.
Assignee | ||
Comment 56•7 years ago
|
||
I'll try to continue the investigation on top of Henry's effort.
Assignee: hchang → sawang
Comment 57•7 years ago
|
||
(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!
Assignee | ||
Comment 58•7 years ago
|
||
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
Assignee | ||
Comment 59•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8898722 -
Flags: review?(masayuki)
Attachment #8898723 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 62•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
Comment on attachment 8898723 [details] Bug 1349255 - Part 2: Wait for DOMModalDialogClosed after clicked cancel button. https://reviewboard.mozilla.org/r/170138/#review175344 Thanks
Attachment #8898723 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 64•7 years ago
|
||
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 66•7 years ago
|
||
mozreview-review |
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 |(|.
Attachment #8898722 -
Flags: review?(masayuki) → review-
Comment 67•7 years ago
|
||
(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.
Assignee | ||
Comment 68•7 years ago
|
||
(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.
Assignee | ||
Comment 69•7 years ago
|
||
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.
Attachment #8871388 -
Flags: feedback?(hchang)
Assignee | ||
Comment 70•7 years ago
|
||
(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)`.
Flags: needinfo?(masayuki)
Comment 71•7 years ago
|
||
(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.
Flags: needinfo?(masayuki)
Assignee | ||
Updated•7 years ago
|
Attachment #8864779 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8871388 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 74•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
Comment on attachment 8898722 [details] Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank. https://reviewboard.mozilla.org/r/170136/#review176740
Attachment #8898722 -
Flags: review?(masayuki) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 79•7 years ago
|
||
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.
Flags: needinfo?(sawang)
Keywords: checkin-needed
Comment 80•7 years ago
|
||
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
Updated•7 years ago
|
Comment 81•7 years ago
|
||
mozreview-review |
Comment on attachment 8898722 [details] Bug 1349255 - Part 1: Make PBrowser.NotifyIMEFocus async to avoid UI jank. https://reviewboard.mozilla.org/r/170136/#review177968
Attachment #8898722 -
Flags: review+
Comment 82•7 years ago
|
||
Sigh, you need a IPC peer to give r+ but I'm not sure how to do that in mozreview
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 85•7 years ago
|
||
Updated commit message. That should work. Thanks Kanru!
Flags: needinfo?(sawang)
Keywords: checkin-needed
Comment 86•7 years ago
|
||
Pushed by ryanvm@gmail.com: 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
Keywords: checkin-needed
Comment 87•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e03ce515252 https://hg.mozilla.org/mozilla-central/rev/0c78ece467a8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•5 years ago
|
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•