mozilla :: #content

13 Jul 2017
00:01jfmdo we have any workarounds for background threads that need to deal with URLs serialized over IPDL?
00:01* jfm thinks this might torpedo the background cookie thread idea
00:02jfmif the cookie code only cares about URLs that are backed by native code, can we force it to be parsed on the background thread?
00:04* jfm wonders if we should just serialize the individual pieces instead
00:14jfmnever trust someone who says "that doesn't look too hard" when they have not looked closely at the code in question
00:38bkellyjfm: you could look at what catalinb is trying to do for URL() on worker threads
00:39jfmmmm
00:41bkellyjfm: see bug 1344751
00:41firebothttps://bugzil.la/1344751 NEW, catalin.badea392@gmail.com optimize URL() read access in Worker threads for common URL schemes
00:50catalinbbkelly: ping
00:50bkellycatalinb: hi
00:51catalinbbkelly: hey, are you familiar with the FetchConsumer code? I have a quick question
00:51bkellycatalinb: I reviewed it... so in theory I should know something about it... baku|away wrote it
00:52catalinbbkelly: so if the worker gets closed while a FetchBodyConsumer is on the main thread
00:53catalinbwe get notified in FetchBodyWorkerHolder which does the cleanup
00:53catalinbbut shouldn't we have a mutex here?
00:53catalinbcause we can run code on the main thread that touches the same data
00:54catalinbit's similar to what we had for promise resolves
00:55bkellycatalinb: which data in particular is shared between threads?
00:56catalinbbkelly: BeginConsumeBodyRunnable holding a mFetchBodyConsumer on the main thread
00:57catalinbbkelly: and then, while running http://searchfox.org/mozilla-central/source/dom/fetch/FetchConsumer.cpp#68
00:57catalinbwe get a Notify that the worker is shutting down
00:59catalinbbkelly: I guess I can test this by writing a quick patch for it since it happens quite often on tryrun
01:00bkellycatalinb: there have been various crashes in this code, so it would not surprise me
01:09bkellycatalinb: I guess its still not jumping out at me which fields on FetchBodyConsumer are accessed by both of those methods?
01:09catalinbbkelly: mBody
01:10catalinbin BeginConsumeBody we have a bunch of mBody-> calls
01:11catalinband moBody is set to null in the Notify call
01:13catalinbbkelly: FetchBodyHolder::Notify -> ContinueConsumeBody -> ReleaseObject()
01:14bkellycatalinb: hmm... I guess it should wait for this to complete before clearing mBody?
01:14bkellyhttp://searchfox.org/mozilla-central/source/dom/fetch/FetchConsumer.cpp#677
01:15bkellylike bounce another runnable back to indicate the main thread stuff is stopped
01:21catalinbbkelly: oh, that's a sync runnable
01:23bkellycatalinb: oh, right... I forgot that
01:23bkellycatalinb: maybe there is some extra runnable coming in after the pump is canceled
01:24bkellycatalinb: maybe we just need to check mShuttingDown in a few more places before accessing mBody
01:31catalinbbkelly: oh, I know
01:32catalinbbkelly: the dispatch takes a minimum worker state
01:32catalinband we pass Terminating
01:33catalinbmeaning if we call this from a Notify for Terminating the dispatch won't happen
01:33catalinbbkelly: http://searchfox.org/mozilla-central/source/dom/workers/WorkerRunnable.h#404
01:34bkellycatalinb: yea, that should probably be Killing
01:34bkellygood catch
09:20bakucatalinb: ping
10:02catalinbbaku: pong
10:19bakucatalinb: file a bug for that issue
10:19bakucatalinb: I've just seen the chat between you and bkelly
10:22catalinbbaku: ok, I think the dispatch flag might be it. Try run didn't crash with the flag changed (and the rest of my URL patches), but I'll rerun that job a couple of times to be sure.
10:37catalinbbaku: ok, so it looks like it's not crashing anymore after changing the flag. Quick review pretty please? bug 1380604?
10:37firebothttps://bugzil.la/1380604 NEW, nobody@mozilla.org Race in FetchConsumer
10:43bakuit's just me or I cannot submit a public a review using splinter?
10:48bakuit seems that localStorage is broken in release.
10:48bakusmaug: have you seen this?
10:48smauglocalStorage broken in release?
10:49bakuhttps://pastebin.mozilla.org/9026996
10:49smaug( I know nothing about splinter. )
10:49bakuI guess this is not about splinter.
10:49smaugis that some database corruption perhaps?
11:02bakuI had to restart firefox.
11:03smauginteresting. I wonder what could have caused the issue
11:05bakuI also tried to run FF using the same profile but that works.
11:06bakuthere was not corruption... it was probably something else, but no debug information, e no log :/
13:02NarrowWeaselSince the web app store has been discontinued I'm wondering if there is a proper way to get some JS code fingerprint checked. The reason I am asking is that some people (e.g. proton mail) like to do crypto in JS for e2e security but that kind of thing requires some sort of vality check for the JS to prevent the server from sending malicious code.
13:02NarrowWeaselSo... should these things be firefox extensions or is there some other trick I could use to get verified JS?
13:14bkellybaku: some multi-process localStorage breakage? maybe asuth would know?
13:15bakubkelly: hard to say. I didn't have log or access to the stderr. Restarting FF, everything was OK again.
13:15bkellybaku: is this on mac?
13:15bakubkelly: linux :/
13:16bakuoh well
13:16bkellyok
13:16bakuright. maybe I have the log. 2 secs.
13:16bkellyI thought maybe it was more weird breakage from the sandbox change on mac recently... although I guess thats only nightly right now
13:16bakuyeah nothing more than line 1119: NS_ERROR_FAILURE
14:06bkellysmaug: so I was reading a bit about how chrome canary keeps things running smoothly during my timer-flood test
14:06bkellysmaug: it sounds like they have something in their nsThread equivalent which lets you assign "percent CPU" for a given "task source"
14:07bkellysmaug: so they can weight painting, rAF, animations, etc higher than other sources like timers
14:07bkellysounds like its not a pure priority queue, but more a weighting of time allowed per frame, etc
14:08bkellysmaug: I wonder if this approach could solve some of the problems you saw with trying to prioritize vsync runnables, etc
14:08smaugwell, painting/raf/animations are vsync for us
14:09smaugbut yes, that is another option to solve the issue I had with vsync
14:09smaugnot sure which one is better
14:10bkellythe thing I like about it is it provides a single configuration point to tune things... a mapping of "source=% weight"...
14:10bkellyanyway, just something interesting i thought I would mention
16:49bzmccr8: ping
16:49mccr8bz: hey
16:49bzmccr8: Does https://bugzilla.mozilla.org/show_bug.cgi?id=1380393#c9 help?
16:50firebotBug 1380393 NEW, bzbarsky@mit.edu Kill off unused offset* bits of nsIDOMHTMLElement
16:50bzmccr8: basically, nothing in script depends on these nsIDOM* xpidl interfaces (which are not scriptable anyway)
16:50mccr8bz: ah, ok, so even with a QI or whatever, JS can never get at the XPIDL interface. And all of these properties are just things from the WebIDL interface. Interesting.
16:51mccr8Thanks for the explanation.
16:51bzYep, exactly
16:51bzNo problem
16:52bzBasically, we fake all the xpidl-like bits hard
16:52mccr8That sounds fun.
16:55* bz needs to spend some more time randomly removing one DOM xpidl interface a day or something
17:24bkellybz: just change your bugzilla description to "automatic r- if the patch does not include a commit message and also removes a DOM xpidl interface"
17:36bzbkelly: lol
17:52* jdm tries to reason about whether nsString::Assign copies or shares
17:53jdmtrying to understand the string code implementation is... an experience
19:30bzjdm: The answer is that it depends
19:30bzjdm: On whether the thing is shareable
19:30jdmit's ok, I made progress since then
19:30jdmthanks anyways
20:02smaugI wonder how much, if any, DOM performance would improve by removing nsIDOM* interfaces. It would shrink the size of many elements
20:02smaugmore elements would be 128 bytes
20:20bzsmaug: I would really like us to do some of that post-57
21:33* smaug mumbles something about Phabricator change and not having chance to give any feedback.
21:52bkellybut they talked to senior engineers...
21:58qDotTOP. MEN.
22:00qDotMeanwhile, I can't submit patches to mozreview because at one point they contained someone that now has no-review flags on.
22:00qDotBack to splinter it is.
22:06mccr8top... man[ager]s.
22:07mccr8qDot: I think you can resubmit a patch with somebody who doesn't have no-review flags on? maybe? I hit a similar issue and filed a bug about it. (which was WONTFIXed...)
22:08mccr8maybe this will fix it: https://bugzilla.mozilla.org/show_bug.cgi?id=1346550#c3
22:08firebotBug 1346550 WONTFIX, pzalewa@mozilla.com Can't push review request after attempting to request review from somebody who had it blocked
22:09qDotmccr8: Ah, ok, thanks.
22:10smaugbz: this readystate handling. Does it work with document.write?
22:11smaugsay, you first load a page, then document.write("foo"); after load event and then go back and forward, or something like that
22:11smaug(no document.close())
22:18qDotWow that is so weird. Cleared the invalid reviewer in the UI and things work now, but I would not even have known that UI existed otherwise.
23:08mccr8Yeah the mozreview UI is baffling at times. Like you are lost in some kind of bramble maze.
23:09mccr8I have thrown down my defense of having DOM peers! https://www.oxymoronical.com/blog/2017/07/Thoughts-on-the-module-system
23:24smaughuh
23:24smaug++mccr8
23:25mccr8smaug: I was going to add that a positive thing about this idea is that it means it'll be easier for you to review 100% of Firefox patches, but I decided against it. ;)
23:25smaugha
23:26smaugmccr8: you know, I learnt yesterday that I'm a firefox module peer! https://wiki.mozilla.org/Modules/Firefox
23:26smaugno idea how that happened
23:26mccr8smaug: hah!
23:29mccr8smaug: "added more peers" in Feb 2017. https://wiki.mozilla.org/index.php?title=Modules%2FFirefox&diff=1163663&oldid=1163614
23:30mccr8njn and janv were added, too.
14 Jul 2017
No messages
   
Last message: 7 days and 11 hours ago