mozilla :: #e10s

15 Mar 2017
00:36bkellymconley: are there known issues with e10s and ABP? notice that guy on twitter has ABP installed
00:37mconleyMaybe if it's out of date. Recent versions should be okay.
00:37mconleybkelly: --^
00:37bkellyhmm
15:44spohlhi, in e10s, this call is no longer blocking: https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#5468
15:45spohlI was told that I could set mWantReplyFromContentProcess to true for the event
15:45spohlthis was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=862519
15:45firebotBug 862519 FIXED, wmccloskey@mozilla.com Events handled by both content and chrome are not supported well in electrolysis
15:45spohlbut this also doesn't seem to be synchronous
15:46spohlinstead, the key event seems to be redispatched from the content process to the parent process
15:46spohlwhat would be the proper way to detect whether or not the event was handled by the content process here?
16:09bsmedbergspohl, you want smaug
16:10smaugblocking?
16:11smaugspohl: there was never blocking parent->child calls
16:13spohlsmaug: right, but in non-e10s, we would know whether or not the event was handled
16:13smaugtrue
16:13spohlin e10s, we no longer seem to have a way to know whether or not the child process handled the event
16:14smaugspohl: which event are you talking about?
16:14spohland the mWantReplyFromContentProcess flag seemed promising, but I wasn't sure if I had to register some kind of listener for the event that gets "replied" by the child process
16:15spohlso, the native event is https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#5468
16:15smaugLooks like mWantReplyFromContentProcess is for key events only, etm
16:15smaugatm
16:15spohlwhich then gets dispatched here: https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/TextInputHandler.mm#1645
16:15smaughttp://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#1975-1977
16:15smaugok, you do deal key events
16:15smaugso you should get another event dispatched
16:16spohlyou mean from child back to parent, yes?
16:17smaugwell, child sends a message to parent, and parent should dispatch a new event
16:17smaugBy this code http://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1998-2019
16:18spohlyes, ok, that seems to match my understanding. what I've been struggling with is how to detect that this was the response from the child for the earlier event, and basically relay this info back to nsChildView
16:19smaugspohl: check that mNoCrossProcessBoundaryForwarding is true in the event listener?
16:21spohlsmaug: but couldn't there be several different events that request a reply from the content process? how do I know that it's the reply from the event that I dispatched here in the first place?
16:21smaugoh, you want to know it is exactly your event
16:22smaugI guess you need to add some ID to the WidgetEvent or so
16:22spohlright, I basically need to get back to a point where I can decide whether or not to dispatch the event further, to the native application menu for example
16:24spohli.e. which is why we used to have the |handled| flag, to decide whether or not to dispatch the event to the application menu here: https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/nsChildView.mm#5474
16:26spohlsmaug: also, what happens if there is no child process? is there any reply at all?
16:27spohlsmaug: and if we add an ID to WidgetEvent to wait for a response, do we need to maintain a list of IDs with callbacks? Do we need to purge the list after a certain amount of time has passed?
16:27* smaug looks at the XBL code...
16:28blasseyjimm-lunch: can you join #planning?
16:28smaugXBL code explicitly checks if the target is remote browser, and if not, doesn't set mWantReplyFromContentProcess
16:29smaugspohl: I still don't quite understand what kind of event you're dispatching
16:29* smaug reads cocoa code
16:30spohlso, it's ultimately a WidgetKeyboardEvent
16:30spohlbut one event that causes issues right now is Cmd+V to paste
16:31spohlIn bug 429824, I'm trying to support custom shortcuts for the application menu
16:31firebothttps://bugzil.la/429824 REOPENED, spohl.mozilla.bugs@gmail.com User-defined (custom, System Preferences) shortcuts don't work
16:31spohlthis works perfectly in non-e10s, because |handled| gets set properly here: https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/nsChildView.mm#5468
16:31spohlhowever, in e10s, we don't know that content has already processed Cmd+V and pasted into a text field
16:32spohlso then we dispatch Cmd+V again to the application menu, which by default has a shortcut key of Cmd+V set for pasting
16:32spohlwhich results in content being pasted twice in text fields
16:33smaugwait what? e10s on OSX pastes twice?
16:33smaugor only if web page explicitly handles cmd+v?
16:34spohlit pastes twice when we land this patch, which is to support custom shortcuts on OSX: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=429824&attachment=8829951
16:34spohlwe had to back it out for that reason
16:36spohland this is true for all web pages, not just ones that explicitly handle cmd+v
16:38smaugspohl: is performKeyEquivalent some OS provided method?
16:38smaugdo we know which key events OS is interested in?
16:39spohlsmaug: yes, this is a native method. and no, we can't be 100% which ones the OS is interested.
16:39spohlwe have an initial set of default shortcuts, but these don't account for custom shortcuts that a user sets in system preferences
16:39spohlwhich is what we're trying to support here
16:40smaugok, sounds very different to what we use mWantReplyFromContentProcess currently
16:41smaugbasically you want reply for all the key events?
16:42spohlright, I basically need for the parent to have a way to know whether an event that it dispatched to the child was actually handled by the child or not
16:43spohlI'm actually quite surprised that we didn't need this so far
16:43smaugI can't really think of any reason why we'd need it elsewhere
16:43smaugthe XBL stuff is different
16:44smaugthere we know whether we have handler for the particular key event
16:45spohlyes, the initial list of shortcuts is in XBL too. the problem I ran into with custom shortcuts is to update this list on the fly when the user sets custom shortcuts
16:46smaugsounds like the setup needs to change then
16:46spohlso we agreed not to do this and instead let the OS handle events if XBL didn't handle it
16:46smaugI wonder how badly this will affect to performance
16:46spohlbut in e10s we don't know whether or not XBL handled it
16:46smaughow so?
16:47smaugoh, I see
16:47smaugyou still need to wait for the reply
16:47spohlright
16:47smaugwell, then wait for the reply?
16:48smaugdoes the ID even matter?
16:48spohlyou mean using an ID on WidgetEvent like you suggested earlier?
16:48smaugI mean the hypothetical ID, does that even matter
16:48spohlwell, I guess it does matter to make sure we're looking at the reply for the event we care about
16:49smaugbut I thought we don't know which events we care about
16:50spohlwe care about all the events that get dispatched here: https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/nsChildView.mm#5468
16:50smaugok, now I'm lost then
16:50spohlwhich gets translated to WidgetKeyboardEvents and dispatched here: https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/TextInputHandler.mm#1649
16:51smaug"so we agreed not to do this and instead let the OS handle events if XBL didn't handle it"
16:51spohloh, I see. let me rephrase:
16:52spohlwe have an initial default set of shortcuts in XBL
16:52spohlour thinking at first was to update this list whenever a user sets a custom shortcut in system preferences
16:53spohlbut there are numerous hurdles to this. one huge issue was translating the shortcut from the native representation to what we want in XBL
16:54spohlwhen we looked at all the obstacles, we (mstange and I) agreed that this was too complex and too hard to maintain.
16:54smaug(sounds surprising but ok)
16:54spohlso we agreed that we wouldn't try to update this default list of shortcuts and instead simply dispatch the event like we do now and if it hasn't been handled by XBL, we would dispatch it to the native menu bar
16:55smaugok, so you don't care about content
16:55smaugonly XBL
16:56spohlI'm thinking content might be able to set the status of the WidgetKeyboardEvent to nsEventStatus_eConsumeNoDefault here too, no? https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/TextInputHandler.mm#1649
16:56spohlif yes, then we'd be interested in that too
16:57smaugyes, content can call preventDefault()
16:57smaugso, you don't want "so we agreed that we wouldn't try to update this default list of shortcuts and instead simply dispatch the event like we do now and if it hasn't been handled by XBL, we would dispatch it to the native menu bar" after all?
16:57smaugbut s/XBL/any DOM/
16:58smaugmeaning that you want reply for all the events then
16:59smaugwhich is of course doable. I still don't understand why you care about the ID though. You just pass all the non-handled events to native menubar
16:59spohlyes, I think that would be the expected behavior here
16:59smaugone thing unclear to me is how much having reply for all the events affect performance
16:59spohlsmaug: oh, yes, if we know that the events haven't been handled we can just dispatch them to the native menubar
17:00spohlI may have misread this, but I thought we currently request replies for all key events, no?
17:00smaugno we don't
17:01smaughttp://searchfox.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#528-530,534
17:02spohlah ok, I see.
17:06smaugso, you'll need to change the setup we have now
17:06smaugand then also measure performance a bit
17:06smaugthough key events happen relatively rarely, so I'm not too worried about them
17:09spohlso I would want to dispatch the event to the native menu bar if mNoCrossProcessBoundaryForwarding is true here, correct? https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#517
17:09spohlsmaug: and I would still need to handle the case when we don't have a child process and don't get a reply
17:10smaugright. You could check after event dispatch what was the originalTarget
17:10smaugif it wasn't remote browser, then nothing was forwarded to child process
17:11smaugor EventStateManager could even set some flag on the event whether it was forwarded to child process
17:11spohland I can do that directly after dispatch here? https://dxr.mozilla.org/mozilla-central/rev/8dd496fd015a2b6e99573070279d9d1593836ea9/widget/cocoa/TextInputHandler.mm#1649
17:13spohlsmaug: checking the originalTarget, that is
17:13smaugright, after dispatch the event has gone through EventStateManager
17:13smaugright
17:13spohlokay, great
17:14spohlphew. looks like there's finally light at the end of the tunnel.
17:14spohlthank you, smaug!
17:42zombiehey, anyone knows the order of magnitude of data we send to the child process on creation, for things like prefs, initiaalProcessData and similar
18:49felipemrbkap: is there some sort of helper in BrowserTestUtils to force tabs to load in difference processes? To write e10s-multi tests?
18:59blasseymconley: in my room when you're ready
19:00mconleyomw
19:04mconleyblassey: https://mzl.la/2nbhRaL
19:09mconleyblassey: https://d3uepj124s5rcx.cloudfront.net/items/1u072q0u1n0u380Z1q1v/charts2.PNG
20:33wlachdoes an e10s argument make any sense at all in the context of xpcshell?
20:33wlachhttp://searchfox.org/mozilla-central/source/testing/xpcshell/mach_test_package_commands.py#25
20:33wlachI am thinking the answer is no, and this is just some kind of copypaste mistake
20:34mrbkapfelipe: not that I know of.
20:34mrbkapasuth: do you know of anything like that?
20:40felipemrbkap: this might be handy.. florian just wrote a test that needed two tabs from two different processes and it was ugly
20:41felipeis there still that JS api to define which process to load? maybe he could have used that, but he didn't
20:42felipeit would be nice to have something like: Browser.loadMultipleProcesses( [...tabs in process1], [...tabs in process2])
20:42felipes/Browser/BrowserTestUtils/
20:53billmblassey: ping
20:55blasseybillm: pong
20:56billmblassey: hey, I can meet whenever you want
20:56blasseyI'm in my room
21:22asuthfelipe: :mystor filed https://bugzilla.mozilla.org/show_bug.cgi?id=1345990 on a helper like that. :gabor had talked about adding something like that at one point, particularly in response to my hacky approach in http://searchfox.org/mozilla-central/source/dom/tests/browser/browser_localStorage_e10s.js#221
21:22firebotBug 1345990 NEW, nobody@mozilla.org Mechanism to ensure tab is loaded in brand new process for mochitest-browser
21:24felipenice!
21:24blasseybillm: https://bugzilla.mozilla.org/buglist.cgi?list_id=13490807&resolution=---&status_whiteboard_type=allwordssubstr&query_format=advanced&status_whiteboard=e10s-multi%20MemShrink
21:46mrbkapone thing we can do now is replace the process allocator with one that always returns a new process.
21:46mrbkapasuth, felipe: though we need to be careful about perf.
21:46mrbkapso that might make it easier.
21:47mrbkapugh, billm: My patch to kill that shutdown warning only works in the parent process -- the child still gives off a ton of those !mMainThread warnings.
21:47mrbkapfelipe: are you still around?
21:47felipemrbkap: yeah
21:48mrbkapfelipe: Hey, do you know if we do something special with focus the first time a tab is raised?
21:49feliperaised in what sense?
21:49felipewhen it was a background tab?
21:49mrbkapfelipe: I was looking at an intermittent test and if I have `tab1 = BTU.openNewForegroundTab(...); tab2 = BTU.openNewForegroundTab(...); gBrowser.selectedTab = tab1;` then when we do `gBrowser.selectedTab = tab2;` later, focus stays in the URL bar.
21:50mrbkapfelipe: but if instead I do `tab1 = BTU.openNewForegroundTab(...); tab2 = gBrowser.addTab(...);` then when we set selectedTab to tab2 later, focus goes to the content.
21:51felipemrbkap: hmm strange that openNewForegroundTab basically does the same
21:52mrbkapfelipe: the only difference I can think of is that we special-case the first time we focus a tab.
21:52mrbkapmaybe somewhere
21:52* mrbkap doesn't care enough to actually debug it.
21:52felipemrbkap: which URL is being opened?
21:52mrbkapfelipe: I'll link you to the test, 1 minute.
21:53felipeI&#39;ll note that about:home for example has <input autofocus>, which might steal focus to the content `if it&#39;s in the foreground when it loads`
21:53felipeso it could be a case of something like that happening
21:54mrbkapfelipe: so, this patch works: https://pastebin.mozilla.org/8982139
21:55mrbkapfelipe: but if I apply https://pastebin.mozilla.org/8982140 on top of that, we get stuck waiting for a focus event after we set gBrowser.selectedTab = tab2.
21:57felipei don&#39;t see a gBrowser.selectedTab = tab2
21:57billmmrbkap: ok, I was a little worried it might not be enough. I think I know how to fix it.
21:58mrbkapfelipe: see `case 6:` in the message listener for Test:BackgroundColorChanged
21:59felipeah, I thought it was on the diff
21:59feliperight
21:59felipeyeah, hmm
22:00mrbkapfelipe: It isn&#39;t a big deal, just wondering if you could think of anything off the top of your head.
22:01felipemrbkap: yeah sure... sorry, nothing obvious comes to mind, except the obvious &quot;focus is a mess :P&quot;
22:01mrbkapfelipe: heh, ok. Thanks :)
22:01mrbkapfelipe: and thanks for those reviews!
22:02felipenp, thanks for working on this stuff! out of curiosity, are there still many tests using shims?
22:03mrbkapfelipe: I need to push to try again to get a number.
22:03mrbkapfelipe: there are still a fair amount.
22:06mconleyblassey: hey - my content process is hung at start-up on my Win 10 machine. The main thread is stuck trying to load freebl3.dll... is that the deadlock you were talking about before in our 1:1?
22:07mconleythe active thread is also stuck at MessagePumpForIO::GetIOItem
22:08mconleyin the chromium IPC stuff
22:08mconleyis there a bug for this hang?
22:09blasseymconley: unclear. Is ContentParent on the stack?
22:09mrbkapfelipe: I think there should be ~40 tests left.
22:10mconleyblassey: no - this is a content process hang
22:10mconleyso I guess this isn&#39;t the same thing
22:15blasseyNo, this was a chrome process hang
16 Mar 2017
No messages
   
Last message: 101 days and 16 hours ago