mozilla :: #content

12 Jul 2017
11:00kanrumccr8: is 1378374 related to 1378281?
13:36bkellysmaug: is it possible for browser chrome js to run on threads that are not the main thread and not a dom/workers thread?
13:36bkellysmaug: I'm trying to understand nsIThreadManager.currentThread
13:37bkellyhttp://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#231
13:37smaugbkelly: doesn't mozreview comments end up to bugs too
13:37bkellysmaug: just the comments... not the whole patch AFAIK
13:37smaugah, right
13:39bkellysmaug: I'm trying to figure out if the browser chrome call sites using nsIThreadManager.currentThread would reasonably have an nsIGlobalObject... I assume they would have to!
13:40smaugbkelly: I'm thinking if there is some other omt js... is that one thing necko uses, what is it called....
13:40* bkelly is about to learn something new...
13:42smaugbkelly: but that wouldn't anyhow use nsIThreadManager
13:42smaugsince it would need xpconnect
13:42smaugand xpconnect is main thread only
13:45smaugbkelly: I think I'm thinking proxy.pac stuff
13:45bkellyeww
13:48smaughttp://searchfox.org/mozilla-central/source/netwerk/base/ProxyAutoConfig.cpp#553
13:50smaugbkelly: but I guess that JS isn't really chrome JS
13:50smaugit is just JS
13:50smaugwith very limited API surface
14:17bkellysmaug: so it can't access nsIThreadManager... that works for me
14:17smaugaccessing nsIThreadManager would need xpconnect
14:17bkellysmaug: trying to suggest maybe we should have a self.eventTargetFor(TaskCategory.other) in chrome code: https://bugzilla.mozilla.org/show_bug.cgi?id=1376840#c38
14:17firebotBug 1376840 NEW, btseng@mozilla.com Label users of pipes (nsPipeInputStream)
14:18bkellyand then replace most (all?) uses of threadManager.currentThread
14:18smaugwhat is self there
14:18bkellysmaug: the global
14:19bkellysmaug: which I guess I don't know what it is in a jsm
14:21smaugdon't have a strong opinion
14:21bkellyok
14:21bkellysmaug: I mean, it could be on something else that has a ref to the nsiGLobalObject
14:21bkellya service just probably isn't the right answer
14:33bkellybaku: is there any chance you can review bug 1379243 today or tomorrow? I'd just like to land before going on PTO
14:33firebothttps://bugzil.la/1379243 ASSIGNED, bkelly@mozilla.com make WorkerGlobalScope::EventTargetFor() continue to work after worker shutdown is started
14:33bkellyleaving for PTO friday
15:22mystorIf I dispatch a runnable to the main thread during shutdown, the runnable is leaked. Is there a way for me to get the runnable freed instead? My runnable is threadsafe and it's causing asan failures :-/
15:22mystorsmaug: ^
15:29smaugmystor: don't you get error result back from Dispatch if it fails
15:32overholthttps://bugzilla.mozilla.org/show_bug.cgi?id=1378124
15:32firebotBug 1378124 NEW, nobody@mozilla.org Make it possible to check if a Window belongs to a pinned tab
15:37bakubkelly: ok
15:45mystorsmaug: I pass an already_AddRefed into Dispatch, so I'd have to do sketchy stuff where I stash a reference and do an extra free in the error case :-/
15:45mystorsmaug: That sounds crappy
15:46smaugcan you use the non already_AddRefed version?
15:46mystorI think that just addrefs and then forwards to the already_AddRefed version
15:46mystorbut I'll check
15:46smaugyeah, can't recall
15:46* mystor is calling SystemGroup::Dispatch which I think might only have an already_AddRefed version
15:46smaugnever understood why we move to already_Addrefed form
15:46smaugaha
15:47billmsmaug: https://bugzilla.mozilla.org/attachment.cgi?id=8885545&action=edit
15:50smaugaha, vidyo crashed
15:52smaugbillm: anyhow, I guess using system group is ok there
15:59smaugnm that
16:33rhuntsmaug: if you have time, could I get your opinion on https://bugzilla.mozilla.org/show_bug.cgi?id=1379280#c2
16:33firebotBug 1379280 NEW, rhunt@eqrion.net Typing a space in an input field causes page-down scrolling with apz.keyboard.enabled=true
16:33smaugrhunt: looking
16:34smaugrhunt: oh, I thought we'd do apz scrolling only when focus is on document element or body element
16:34smaugis that not the case?
16:36rhuntsmaug: that was the original idea, but I found that just getting it working for the root scrollable would get us most of the way there for any scrollable
16:37rhuntsmaug: we can restrict it to just the root scrollable if event handlers become more of a problem
16:37rhuntsmaug: oh, so the patches are set up to work for any scrollable
16:39smaugrhunt: if we want to support all kinds of scrollable areas, we need something new...
16:40smaugdirty but quite simple hack would be to add virtual bool APZKeyScrollable() to Element and then by default return true
16:40rhuntsmaug: why is that?
16:41smaugrhunt: though, I think I like your approach more
16:41smaugrhunt: to check the event listeners (also system group ones) on focus target
16:41smaugthinking...
16:42smaugdoes that work always
16:44smaugrhunt: I don't understand the xul:browser issue
16:44smaugwouldn't we do this event listener check only in child process
16:46rhuntwe do the check in both
16:48smaugrhunt: but I mean for this bug
16:48smaugwhy would we need to check xul stuff
16:50rhuntsmaug: doesn't a key press event get sent through xul:browser before it goes to content?
16:50smaugyes, so?
16:51rhuntwe do the check for all event targets on the way to the focused element because they could preventDefault/change focus
16:51smaugrhunt: I guess I would special case remote xul:browser, since it acts as event forwarder
16:51smaugrhunt: oh, now I don't understand what you proposed then
16:51smaugI thought only on target
16:52smaugbut indeed you talk about XBL binding.
16:53smaug<input> and <textarea> don&#39;t really have XBL bindings
16:53smaugthey just get event listeners from XBL
16:54smaugrhunt: maybe explicit virtual bool APZKeyScrollable() check wouldn&#39;t be too bad?
16:54smaugon Element
16:55rhuntsmaug: with the default returning true, and HTMLInputElement, HTMLTextAreaElement, etc. returning false?
16:55smaugright
16:55smaugwould need to go through all the element types and think whether scrolling is ok
16:56smaugis that too ugly? We don&#39;t really have any very nice solutions
16:57rhuntsmaug: yeah, I think that would work. It&#39;d just take some auditing
16:58rhuntsmaug: im not sure if it&#39;s too ugly, i don&#39;t really know what&#39;s acceptable or not in DOM code :)
17:00smaugnot so much about DOM as such, but about maintainability
17:01rhuntyeah, it&#39;d be something you&#39;d have to think about whenever you add a key listener
17:02smaugrhunt: oh, kats proposed something else
17:02rhuntsmaug: i think kats proposal is equivalent to adding a virtual to Element
17:03smaugrhunt: well his proposal was to force everyone to call mozDisableApzKeyScroll
17:04rhuntsmaug: it&#39;d be nice to be able to assume every key event should disable apz key scrolling, and then specially annotate just a couple key listeners (like the one in browser.xml)
17:06smaughmm, right
17:06smaugwe&#39;d need some new flag on add*EventListener
17:08rhuntI don&#39;t remember how many key listeners would need to be annotated, I&#39;m going to look into that
17:45bkellymystor: you should listen for shutdown and dispatch your main thread runnable sooner?
17:46mystorbkelly: This is a runnable being fired asynchronously by the background hang monitor
17:46bkellymystor: there is also a semi-private way to tell what stage of shutdown you are in... you could check that before dispatching... probably not terribly thread safe, though
17:47bkellymystor: you can do something like this... http://searchfox.org/mozilla-central/source/xpcom/threads/ThrottledEventQueue.cpp#218
17:47mystorbkelly: I&#39;m not on the main thread though
17:49bkellymystor: have main thread shutdown set an atomic boolean somewhere you can check before doing the dispatch
17:49mystorbkelly: There&#39;s still a race there though?
17:49mystorbkelly: Because it could become during shutdown between me reading the atomic and starting to dispatch
17:50jdmugh
17:50jdmI bet nsDocument::StartDocumntLoad isn&#39;t called until we get a network response
17:50mystorlike, this leak doesn&#39;t matter
17:50mystor(I&#39;m leaking during shutdown. boo hoo)
17:50bkellymystor: there is some way to mark allowed leaks I think
17:50mystorbut it totally fails tests
17:51mystorbkelly: How would I mark this leak as allowed?
17:52bkellymystor: can make an extra nsCOMPtr<nsIRunable> so you still own it after dispatch? call Release() an extra time if Dispatch fails...
17:52bkellyor do you have to guarantee its destroyed on main thread?
17:52bkellymystor: not sure how to mark an allowed leak... maybe mccr8 would know?
17:52mystorbkelly: That&#39;s what my WIP patch does right now
17:52bkellyok
17:52mystorand no it can be destroyed anywhere
17:52mystorThe runnable only holds a string and an integer
17:52bkellymystor: I guess I would recommend adding a big comment about why you can&#39;t just stop doing this stuff before main thread shutdown like other code
17:53mystormk
17:54smaugjdm: because you don&#39;t know whether the request should be handled as a document, or whether to download or open some external program...
17:54smaug...before actually getting the response
17:54jdmthis cookie project is the worst
17:55jdmI feel like the solution we&#39;re trying to use is severely hampering our ability to just solve the sync IPC problem that motivated it
17:56mccr8mystor: yeah runnable shutdown leak stuff is a big pain. Can you check if the dispatch failed?
17:56mccr8I don&#39;t really remember what the state of the art is for that.
17:57mystormccr8: Yeah, it returns an NS_ERROR_...
17:57mccr8mystor: can you just detect an error and destroy the object?
17:57mystormccr8: I&#39;m just holding onto a weak pointer in my latest patch which I&#39;m NS_RELEASE(...) ing if I get an error back from Dispatch
17:58mystorI think that&#39;s my best bet
17:58bkellyI don&#39;t know why we can&#39;t pass some flag in that says &quot;its fine to free me on failure, thanks&quot;
17:59mystorbkelly: I would add that, but it would be annoying
17:59mccr8Hmm yeah it looks like there&#39;s no way to get ownership back on failure. Weird.
17:59mystorbkelly: Obviously I should add a `nsIAnyThreadReleaseRunnable` interface which NS_DispatchToMainThread QIs to
18:00mystorand if the QI succeeds it releases it
18:00mystor(maybe `nsIThreadSafeRelease`?)
18:00mystor^,^
18:02bkellybaku: what is the status of the webidl streams review from bz? is there a spec issue?
18:03bakubkelly: I don&#39;t know if something has changed after the SFO meeting, but at that time, ReadableStreams were not covered by the WebIDL spec.
18:03bzbkelly: I need to look at baku&#39;s responses/updates; working on that today
18:04bzBut we definitely need spec issues; I recall asking for some
18:07bkellyoverholt: do we have any stats on how many DOM reviews are done in splinter vs mozreview?
18:07overholtbkelly: I&#39;ll ask Hsin-Yi but I don&#39;t think that&#39;s in the data
18:35bkellycheck out the lovely jank in this profile: https://perfht.ml/2ufbF4Y
18:36bkelly3317.81ms event processing delay on Content (4 of 4)
18:37bkellywow... a lot of time in nsRange::IsNodeSelected(nsINode *,unsigned int,unsigned int)
18:38bkellysmaug: that is the profile for bug 1380414 ^^^
18:38firebothttps://bugzil.la/1380414 NEW, nobody@mozilla.org Janky typing after pasting crash backtrace into bugzilla comment
18:38bkellyhash tables in nsRange?
18:38smaugI just got a profile too. Not that it is janky on windows either
18:39bkellyits way janky for me
18:39bkellymaybe the universe hates me
18:40bzmystor: ping
18:40mystorbz: yes?
18:42smaugbkelly: I think that is a dup
18:42bkellyoh?
18:43smaugbkelly: I mean, I think I&#39;ve seen some similar complain
18:43smaugI wonder why I didn&#39;t get the jank
18:44bzmystor: so about this static analysis....
18:44bkellysmaug: how did you scroll to the top of the text box? I used mouse wheel
18:44mystoryes?
18:44bzmystor: I&#39;d like to get some idea of what&#39;s feasible here
18:44bzmystor: For example.. can we annotate a function with MUST_HOLD_STRONG_THIS
18:44smaugbkelly: wheel
18:44bzmystor: and then ensure that calls to it are either coming via &quot;this&quot; in other thus-annotate functions
18:45bzmystor: or via a strong ptr
18:45bzmystor: or via bindings in some form?
18:45bkellysmaug: I assume you have default spell checking prefs?
18:45smaugit could be a bit better on the slow windows laptop, but I wouldn&#39;t call it extremely janky
18:45smaugyes
18:46bkellysmaug: and you&#39;re not expanding the text field or anything?
18:46mystorbz: Sure
18:46bzmystor: How would we implement the &quot;via bindings&quot; bit?
18:46smaugbkelly: no
18:47mystorbz: Easiest way would be a dummy smart pointer which bindings use
18:47bkellyweird
18:47mystorbz: So that the analysis think you&#39;re holding a strong reference
18:47bzmystor: analysis would not be restricted to things we know are owning pointers?
18:47* bz thinks it should be....
18:47mystorbz: Other than that, we could have a MOZ_STRONG_THIS_EXEMPT_FUNCTION which you can put on things to exempt the function from the analysis or something
18:47bzOtherwise calls via a NonNull would be allowed
18:48mystorbz: Yeah, we&#39;d have a MOZ_IS_SMART_POINTER or something like that annotation
18:48mystorbz: But we could put it on a dummy type private to bindings potentially
18:48mystorbz: like `class MOZ_IS_SMART_POINTER NotActuallySmartPointer {}`
18:49bzI see
18:49bzok
18:49mystorThat&#39;s just me thinking from the easiest to implement POV
18:49bzsure
18:50mystorI imagine that Andi or his intern Tristan would probably implement this
18:50mystor(so they would probably make the call on the easiest thing to do)
18:50bkellysmaug: disabling &quot;check spelling&quot; in the right-click context menu gets rid of the jank
18:50smaugbkelly: you&#39;re using nightly?
18:50bkellysmaug: are you getting a lot of mispelled words in the stack trace?
18:50bkellysmaug: yes
18:50bkellyjust updated
18:51bkellymaybe you added a bunch of stack trace words to the dictionary?
18:51smaugI do see lots spellchecker red lines
18:51smaugno
18:52bkellyhmm
18:54bzhttps://bugzilla.mozilla.org/show_bug.cgi?id=1380423
18:54firebotBug 1380423 NEW, nobody@mozilla.org Add static analysis for &quot;caller must hold strong refs to function args&quot;
19:01mystorbz: Thanks
19:01bkellysmaug: I took a screen recording of exactly what I am doing here: https://bugzilla.mozilla.org/show_bug.cgi?id=1380414#c5
19:01firebotBug 1380414 NEW, nobody@mozilla.org Janky typing after pasting crash backtrace into bugzilla comment
19:02smaugbkelly: I did the same
19:03bkellyhmm
19:03bkellysmaug: maybe you type so slow you don&#39;t notice the jank? :-)
19:03smaugexcept, I used keyboard to select the stack
19:03smauglet me try
19:03smaugbkelly: I was just typing alsdkfasd asdfl kajsdfla skdjasdf &#39;
19:03smaugand it felt quite ok
19:04smaugstill ok
19:04bkellyit must be your fancy &quot;o&quot; character
19:04smaugcould be better, but definitely not extremely janky
19:05smaugyes,
19:05smaug(though, the reference laptop has English keyboard)
19:08smaugbkelly: like, in your video there is bad jank after &#39;this i&#39;
19:08smaugI don&#39;t see anything like that
19:08bkellysmaug: yea... even hitting enter is a bit janky... but it gets much worse as I type faster
20:58bkellybz: do you think we care if our Timeout::mNestingLevel rolls over once every 49 days?
20:58bkellyassuming it fires continuiously at 4ms intervals
20:59bkellyI guess I could just cap it
21:10bzbkelly: probably not, or we could just make it uint64_t, right?
21:10bkellybz: we don&#39;t care about values above 5.... so I&#39;m just going to cap it there and shrink this member variable to uint8_t
22:23bzsmaug: are you around?
22:23smaugbz: yup
22:23bzsmaug: https://bugzilla.mozilla.org/show_bug.cgi?id=1377131#c14 seems to be a broken link....
22:23firebotBug 1377131 NEW, bugs@pettay.fi Try to trigger collector slices at times which disturb page js less (at least with iframes loaded af
22:23smaughmm
22:23smauguh
22:23smaugcrap
22:24bz(presumably because it&#39;s in an earlier day....)
22:24* bz tried doing http://logs.glob.uno/?c=mozilla%23content&s=5+Jul+2017&e=12+Jul+2017#c451100 but that doesn&#39;t seem to find it either
22:24smauglet me try to find if
22:24smaugbz: I guess http://logs.glob.uno/?c=mozilla%23content&s=3+Jul+2017&e=3+Jul+2017#c451100
22:24bzAha
22:25* bz did not go far enough back in time
22:26bzAlso, which of the patches in that bug should I be looking at?
22:26smaugthe last one
22:26bzThe &quot;approach 2 v5&quot; one, or something else?
22:26bzAnd also also, whichever patch is involved needs a commit message explaining what it&#39;s trying to do, exactly.
22:28* bz is not sure whether chrome process ends up having to talk to content process still after OpenURI and whether the collection would therefore slow down pageloads
22:28bzI guess collections are pretty free if there is no new stuff in the purple buffer
22:29bzThe bit in parser onstartrequest worries me a lot more....
22:29* bz ponders
22:29smaugI&#39;m just compiling a patch to try if we could trigger collectors there only if the collector timer is about to fire. That might not be too effective
22:30smaugor it might
22:30bzSo in the benchmark, we don&#39;t have enough &quot;natural&quot; idle time in pageload to run the collector?
22:30bzBut we do have pageloads?
22:30bzAnd it just happens they don&#39;t get timed?
22:30* bz trying to make sure he understands parameters here
22:30smaugwe load pages in iframe as fast as possible
22:31smaugand iframes do the benchmarking
22:31bzok
22:31bzAnd loading time is not considered idle?
22:31smaugthere is some idle time occasionally
22:31bzok
22:31smaugno
22:31* bz wonders whether it should be, maybe
22:31smaugidle is based on refreshdriver and timers and main event loop being otherwise empty
22:32smauglooks like I could call collectors only if their timer should have timed out
22:32smaugmccr8: does that sounds better?
22:33bzSo if the timer should have timed out
22:33bzbut has not
22:33bzwhat does that mean?
22:34smaugbz: if main thread is too busy to handle timer callbacks
22:34smaugI mean, the runnable to handle them is somewhere in queue
22:35bzok
22:35bzBut if main thread is busy....
22:35bzWhy is that a good time to CC?
22:35bz&quot;busy&quot; == &quot;event loop is backlogged&quot;
22:35bzFrom a user, not benchmark, pov
22:35smaugwhy is that not a good time? I mean, we would do it anyhow when the timer runnable gets chance to run
22:35bzhmm
22:36smaugoh, I would still use the check for user activity
22:36smaugif user hasn&#39;t been active
22:36bzaha
22:36bzThat seems pretty good to me, then
22:36smaugyet main thread is busy and we should have run a collector
22:36smaugok, let me try this one
22:36smaugthis is a small change to the v5 patch
22:37smaugnot as effective
22:37smaugbut might still be good for benchmarks
22:45bzsmaug: Is that all you neeed info on in the bug? As in, can I clear the flag?
22:45smaugbz: how do you feel like about the docshell and parser bits
22:46smaugor can you think of better places to try something like this
22:46bzThose bits seemed fine, more or less
22:46bzThe docshell bit is definitely fine
22:47bzThe parser bit is a bit more worrisome, in that we might end up not servicing the parser&#39;s prefetch bits and DOM node creation bits while we CC
22:47bzI would be interested in having telemetry on how long these CCs end up taking in the wild
22:48bzSo we can get some idea of whether that&#39;s a problem in practice
22:48smaugah, yeah, I could add some telemetry about these &quot;sync&quot; CC/GC runs
13 Jul 2017
No messages
   
Last message: 14 days ago