mozilla :: #content

9 Aug 2017
09:24jgrahambz_dinner: Well I'm around now. I assume you aren't though
14:05smaughmm, bug 1355746 should be in Nightly. Time to update.
14:05firebothttps://bugzil.la/1355746 FIXED, hchang@mozilla.com Parser should use idle dispatch and not a timer for background tabs
14:28froydnjmystor: ping
14:28mystorfroydnj: hey
14:28froydnjmystor: I have permaoranges from bug 1380619 that are blocking some work =/
14:29firebothttps://bugzil.la/1380619 NEW, nobody@mozilla.org Intermittent LeakSanitizer | leak at mozilla::SchedulerGroup::LabeledDispatch, Dispatch, Dispatch, m
14:29mystor:(
14:29mystorfroydnj: I'm one patch review away from landing bug 1380081 I think
14:29firebothttps://bugzil.la/1380081 NEW, michael@thelayzells.com Update BHR ping format
14:29mystorfroydnj: I think that will make that go away
14:29* mystor hopes
14:33froydnjmystor: ugh
14:34mystorfroydnj: I can try to fix that bug specifically too
14:34mystorfroydnj: I just don't even know what that might look like
14:34mystorfroydnj: If it would help unblock you, you can disable http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/xpcom/threads/BackgroundHangMonitor.cpp#616-645
14:35mystorI'
14:35mystorI'm just going to delete the code soon anyways, and I don't think anyone in tree uses it yet
14:35froydnjmystor: o.O
14:35froydnjmystor: looking at those comments, I don't know why we don't just shutdown BHR at xpcom-shutdown, like everything else
14:36mystorfroydnj: Yeah, I want to change that soon too
14:36mystorfroydnj: But yeah, those comments are the cause of the problem
14:36mystorfroydnj: The problem specifically is that SystemGroup::Dispatch actually creates another runnable internally which leaks even with my hacks
14:36mystorfroydnj: (Didn't you review this code?)
14:37froydnjmystor: fortunately I did not review the patches that added those comments =D
14:37mystorfroydnj: heh. I guess you were on PTO or something
14:37froydnjI probably reviewd the SystemGroup::Dispatch code and didn't think through the implications
14:37mystorfroydnj: Anyways, I'm deleting all of that ASAP
14:37mystorfroydnj: 'cause it's crap code
14:38froydnjwe could make SchedulerGroup cooperate with shutdown, I think...probably have to, to fix intermittent leaks and whatnot
14:38mystoryeah
14:38mystorthe problem is just that we can't just free the runnable off main thread
14:38mystor(well we can in this case, but can't in the general case)
14:39froydnjright
14:41* froydnj attempts to write a patch
14:41mystor\o/
14:41mystorfroydnj: lmk if you figure something out
14:59smaughmm, back to beta, since Chatzilla is broken on Nightly
15:00smaugAny irccloud users?
15:00smaughow can I use that?
15:00smaugmana instructions don't work
15:01froydnjsmaug: using irccloud here
15:02smaugfroydnj: instructions say I should load https://irccloud.mozilla.com/ and login
15:02smaugit only gives me okta login
15:02smaugbut then "Sorry, you can't access IRCCloud because you are not assigned this app in Okta."
15:03* smaug tries #moc
15:04froydnjI think I ordered an irccloud subscription (?) in servicenow
15:04froydnjand then things just worked
15:04padenotsmaug, #servicedesk, this should be quick
15:12froydnjmystor: https://hg.mozilla.org/try/rev/98b5a247fab54156c7e604226eaf9a7b597c605f is my attempt at addressing this, wdyt?
15:12mystorfroydnj: looking
15:16mystorfroydnj: I'd not use an overload of UnlabeledDispatch, but rather a different method like InternalUnlabeledDispatch
15:16mystorfroydnj: But the general strategy is good I think
15:17froydnjmystor: fair enough
15:17mystorfroydnj: The fact that the name of the internal runnable wrapper is "Runnable" doesn't help with clarity ^.^
15:18froydnjmystor: globally-unique names would be nice sometimes
15:18froydnjmystor: have noticed this same problem in Rust =/
15:19mystorfroydnj: I'd also probably reword the comment on NS_DispatchToCurrentThread to say something like "NS_DispatchToCurrentThread will not leak the passed in runnable when it fails, so we don't need to handle it specifically"
15:19mystorfroydnj: I think it's a problem in general with namespaces. Sometimes they're great because they let you use generic names for internal things, but then you sometimes end up in a situation where it becomes confusing
15:20mystorfroydnj: I'm not the biggest fan of the `Result` type alias pattern
15:20mystor(io::Result and co are very confusing)
15:20froydnjmystor: *nod*, the Result thing really bugs me
15:21* mystor assumed thats what you were referring to
15:21froydnjmystor: but then you have the same problem in reverse without namespaces, PackagePrefixForMyClass sort of thing
15:21mystoryeah. There's something to be said about the strategy of always requiring at least one component of the namespace to import
15:21froydnjmystor: I was thinking of it because Cargo has several different Package types, I think, but Result has the same issues too, which definitely confused me
15:21mystorI think that's what go does? not sure
15:22mystorfroydnj: Yeah, I can see that.
15:22froydnjI don't mind package:: prefixes so much, but then I would also advocate for this-> everything, so >.>
15:22mystorfroydnj: I mean rust requires self.everything, so :P
15:22mystorfroydnj: There's precident
15:22froydnjmystor: same thing with Python
15:23mystorfroydnj: I think this->everything is better, but m-prefixes work too and is what we do here, so *shrug*
15:23* mystor doesn't care enough about style
15:23froydnjmystor: and you actually need this-> prefixing in some template cases, so consistency!
15:23mystorwhee!
15:23froydnjmystor: atm I know not to wade into style discussions ;)
15:24mystor;)
16:20* froydnj grumbles about a bunch of accessibility stuff leaking
16:21* froydnj wonders if he's just getting extraordinarily unlucky with some of these oranges
16:25mystorfroydnj: I mean, sounds about right
16:26froydnjlike, I don't see how these changes could materially affect how accessibility stuff gets freed
16:27mystorfroydnj: I don'
16:27mystort think they would?
16:30froydnjmystor: one would think not, but apparently they are
16:31froydnjunless my m-c base is old and these tests have been disabled or something
16:31mystorfroydnj: Are the leaks in a parent or child process?
16:31froydnjmystor: child
16:34mystorfroydnj: Yeah, reading your patch again there's literally no way this should affect that
16:34* froydnj waits for hundreds of asan retriggers to come back
16:34froydnjmystor: well, technically I'm seeing it here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b5a247fab54156c7e604226eaf9a7b597c605f
16:35froydnjbut none of those patches come anywhere near accessibilty
16:38mystorfroydnj: Yeah, still can't see how that would cause leaks
16:38mystorThe stuff being leaked doesn't even include Runnables
17:45mystorsmaug: ping
17:45smaugmystor: pong
17:45mystorsmaug: For the BHR annotation which notes if there is a pending IPC message, what messages would you want me to check for?
17:46smaugmystor: let me check... I think we have some helper method for that...
17:46mystorsmaug: That sounds handy
17:46mystorsmaug: http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/widget/PuppetWidget.cpp#1418?
17:46smaugmystor: something like http://searchfox.org/mozilla-central/source/widget/PuppetWidget.cpp#1397
17:47mystorsmaug: Cool, I'll prob. pull that switch into a nsContentUtils method and then use it in this place too if that's cool with you
17:47smaugsounds good
17:48mystorsmaug: One last thing - PContent is the toplevel protocol for PBrowser, right?
17:49smaugwhatever that means. PContentBridge can be also its manager
17:50mystorsmaug: PBrowser can have multiple different manager types?
17:50mystorsmaug: (I need to hook in at the toplevel protocol)
17:56smaugmystor: PContentBridge is used with sibling processes http://searchfox.org/mozilla-central/source/dom/ipc/PContentBridge.ipdl#29
17:57mystorsmaug: OK, that makes sense.
17:57smaugbut maybe you could care only the normal cases when manager is PContent
17:57mystorsmaug: I'll probably start out with that. Shouldn't be hard to handle the other case
17:59mystorsmaug: If I keep the count of pending input events on the ContentChild singleton, then I can just make ContentBridgeChild poke that
19:05mystorWe never shutdown XPCOM in the content process in release builds, right?
19:13mystorsmaug: ^?
19:32billmmstange: ping
19:36smaugmystor: that I don't recall
19:39mystorsmaug: ok
19:40smaugmystor: mrbkap might recall
19:47mstangebillm: pong
19:47billmmstange: do you think you can get to that appshell needinfo today? I'd like to move forward with that bug soon.
19:48mstangebillm: I'll try
19:48billmmstange: thanks
19:48billmmstange: it's really important for quantum dom
19:48mstangeok
20:02mstangebillm: I replied
20:03billmmstange: thanks
21:04asuthbkelly: so P6 basically moots P4, yeah? Which is good because I don't need to figure out the cleanest way to plumb the event through to the HttpChannelChild's GetNeckoTarget(), including whether it really should get wrapped into an event handled by its ChannelEventQueue.
21:05asuthBecause the system group definitely did not seem sufficiently sound.
21:05bkellyasuth: I don't think it completely gets rid of P4... we still start and copy early
21:05bkellywe used to start copying very early.... and then when done start/finish at once
21:05asuthbkelly: Oh, yeah, that part is fine, it was just the system group thing that concerned me.
21:06bkellyasuth: yea, the ordering issue is no longer there after P6
21:07asuthbkelly: can I review P5/P6 or to you want me to wait until you've got a green try?
21:07asuth(or just explicitly ask for review)
21:07bkellyasuth: maybe wait... I'm not sure this will fix the crash I was getting... still just a theory
21:08bkellyI can ask for review on P5 I guess
21:08asuthokie dokie
21:17bkellyasuth: hmm... I just remembered to try testing with non-e10s... and it doesn't stream the data
21:17bkellyasuth: probably because of the cache entry business
21:18asuthbkelly: it's okay, we don't have tests for that anymore. badum bum!
21:18bkellywell, android
21:19asuthIs joke.
21:19bkellyasuth: unfortunately this means we lose the streaming with parent intercept, right?
21:21asuthbkelly: InterceptedChannelChrome just needs some love, AFAICT
21:21bkellyasuth: how so?
21:27asuthbkelly: The synthesized cache entry can totally handle having stuff streamed into it. For your bug right now, you just need to make InterceptedChannelChrome make sure it supports doing that.
21:28bkellyasuth: if you have any hints on what I'm doing wrong, I would appreciate the pointers
21:39smaugmystor: sorry, I'm not familiar with that part of IPC
21:40smaugI'm surprised to see methods named that way, assuming I guess when they are called
21:40smaugis OnChannelReceivedMessage called when we receive a message and OnMessageReceived when it has been handled?
22:08asuthbkelly: Looks like we need to invoke SetValid() on the synthetic cache entry to stop the channel from waiting for the cache entry.
22:43smaugmccr8: I'm not too familiar with about:memory. Where does it get the "active" window status?
22:44mccr8smaug: the code comes from around here: http://searchfox.org/mozilla-central/source/dom/base/nsWindowMemoryReporter.cpp#284
22:44mccr8it looks like "active" means GetTopInternal() returns a non null value
22:54smaugmccr8: just thinking aloud. Do we initially only hide the tab in the UI
22:54smaugand not remove it
22:55smaugthat would keep the child process side alive...
22:55smaugmconley might immediately say, no, we do remove it immediately
22:57smaugmccr8: can you reproduce the bug?
22:59mccr8smaug: yes. well, when I did it, the window was around, and active. for the reporter, it was a ghost window.
23:07mconleysmaug: once the tab close animation ends (if there was an animation) the browser element is removed from the DOM
23:07mconleyIf there's no animation, the removal is, I believe, either synchronous, or on a setTimeout(0)
23:09mystorsmaug: the on channel method is called on the Io thread before we submit the runnable to the target event handler, and the other one is usually implemented by PContentChild and performs method deserialization followed by calling the recv method
23:11smaugmystor: why you needed three methods
23:11* smaug should look at the patch again
23:12smaugwhat is ContentChild::OnMessageReceived(const Message& aMsg) and what is ContentChild::OnMessageReceived(const Message& aMsg, Message*& aReply) ?
23:12smaugis the latter for rpc kind of stuff?
23:15mystorsmaug: The second one is only implemented because otherwise it gets shadowed which we don't want. You can't override an overload without overriding the other overloads due to shadowing and - Werror
23:15mystorYeah it's used by rpc messages
23:17smaug mystor: so OnMessageReceived is per top level protocol?
23:18mystorsmaug: its implemented on every protocol. The onchannelrecieved one is per top-level
23:18mystorsmaug: OnMessageRecieved for PContent dispatches to PBrowser
23:18smaugaha, that way
23:19mystorsmaug: in its implementation
23:22smaugmystor: oh, do we really go through this http://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PContentChild.cpp#5137 ?
23:22mystorLooking
23:22mystor(sorry I'm on AA phone right now)
23:23froydnj"AA"?
23:23mystorTyping on phones is hard froydnj
23:23mystorsmaug: to my understanding yes
23:23mystorsmaug: its probably a good idea to have someone who knows ipc code look over it though
23:24smaugmystor: surprising, I'd say. Something which could be statically generated and being called all the time ... and we use a hashtable
23:29mystorsmaug: we also do unnecessary virtual function calls for everything. Iunno.
23:30mystorsmaug: I think each ipdl file is compiled separately so we don't have the global knowledge in the current compiler required to route correctly
23:33froydnjprobably not too hard to implement...but is that routing actually causing pain?
23:34smaugnot sure. Something to profile.
10 Aug 2017
No messages
   
Last message: 6 days and 13 hours ago