mozilla :: #content

8 Sep 2017
00:36bkellyAWFY looks unhappy: https://arewefastyet.com/#machine=35&view=single&suite=speedometer-misc&subtest=score
00:36bkelly50% regression effecting both chrome and firefox
02:28overholtbkelly: https://bugs.webkit.org/show_bug.cgi?id=172968, I think
03:20bzbkelly: The scoring changed
07:43annevkbaku|away: if you have some free time: https://bugzilla.mozilla.org/show_bug.cgi?id=1360190
07:43firebotBug 1360190 NEW, nobody@mozilla.org MessageChannel does not work with SharedArrayBuffer
07:46annevkbkelly: yeah, past behavior of not creating WPT tests for all the things is rather horrid practice in hindsight
07:46annevkbkelly: what's bad today is that not all standards orgs are fully committed to creating tests for their work
07:46annevkbkelly: e.g., IETF and many W3C WGs
08:39smaugannevk: I wonder what kind of implementation we have for SharedArrayBuffer
08:39smaugdoes it support cross-process handling
08:40annevksmaug: pretty sure the answer is no, but lth in #jslang can confirm
08:41annevksmaug: note that even with MessageChannel support cross-process would still fail per spec
08:41annevksmaug: since you're not allowed to go outside of "agent clusters"
08:41smaugannevk: oh, I see. forgot that
08:42smaugso broadcastchannel works only.... to some other windows or workers?
08:42* smaug needs to re-read what agent cluster means
08:43annevksmaug: summary: window <-> dedicated workers; shared/service worker <-> dedicated workers
08:43smaugannevk: and no Broadcasting
08:43annevkI forgot how BroadcastChannel works with this
08:45annevksmaug: I think BroadcastChannel would work, since there&#39;s no transferring involved here
08:45smaughmm
08:45annevksmaug: I don&#39;t see anything in the spec that would obviously make it fail
08:47annevksmaug: what you might have though is that if you have a window, dedicated worker, and a shared worker, your SharedArrayBuffer message gets to two of them and the third gets a failure
08:47smaugannevk: I&#39;m not a bit lost. https://bugzilla.mozilla.org/show_bug.cgi?id=1360190#c2 talks about BC, but BC can be cross-process
08:47firebotBug 1360190 NEW, nobody@mozilla.org MessageChannel does not work with SharedArrayBuffer
08:47smaugs/not//
08:48annevksmaug: whenever you hit cross-process, be it MessageChannel or BroadcastChannel, the cloning fails and the receiving party gets a messageerror event
08:48annevksmaug: that&#39;s also what&#39;s supposed to happen when you postMessage() to a cross-origin <iframe>&#39;s Window
08:49smaughow does that work with broadcasting
08:49smaugwhich really broadcasts possibly to several other participants
08:49smaugone may get the data and some may not
08:49annevksmaug: some participants get a message and some get a messageerror
08:49annevkright
08:49smaugbizarre
08:50annevktotally locking down usage would also be frustrating
08:50annevkthere wasn&#39;t really any good solution given the constraints
08:51annevkI think what we have is reasonable; the only complexity we might want to add at some point is that a messageerror results in some kind of automated special message back to the sender so they know about those failures too
09:21bakuannevk: about that bug, I think we cannot support it.
09:21bakuannevk: MessagePort can be sent to a different process (in theory)
09:22bakuannevk: and we don&#39;t support SharedArrayBuffer cross processes.
09:22smaugbaku: sure, but we just need to check that
09:22annevkbaku: yeah, and at that point deserializing the message would fail
09:22annevkbaku: and you&#39;d fire a messageerror event
09:22bakuannevk: right. oh... I didn&#39;t know we finally have messageerror events when deserializing fails.
09:22bakuannevk: this is great news for MessagePort and any StructuredClone use.
09:23annevkbaku: yeah, I added that with Domenic to support this and also OOM for normal ArrayBuffer
09:23bakuannevk: do you know if we support it already? talking about OOM for normal arrayBuffer?
09:24annevkbaku: I think we support throwing RangeError, but we don&#39;t support messageerror afaik
09:24bakuk
09:24bakuright. we should file a bug.
09:24* baku doing it
09:25annevkbaku: there&#39;s a bug on messageerror already
09:25annevkbaku: https://bugzilla.mozilla.org/show_bug.cgi?id=1359017
09:25firebotBug 1359017 NEW, nobody@mozilla.org Implement messageerror event
09:25bakuannevk: cool. thanks
09:57emiliosmaug: could I ask you a question about XBL and its interaction with the flattened tree?
09:58emiliosmaug: is it expected that an element who is in a composed doc is not a flattened tree descendant of that document?
10:07smaugemilio: I&#39;d say not expected
10:07smaugwhat is your testcase
10:08emiliosmaug: bug 1397983
10:08firebothttps://bugzil.la/1397983 NEW, emilio@crisal.io stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::Conten
10:08emiliosmaug: in particular, I have an element who claims to be in the composed doc, but whose binding parent claims not to be
10:09smaugI can&#39;t immediately think of cases where that could happen
10:09smaugexpect perhaps during unlinking or while removing something from document or such
10:10emiliosmaug: I&#39;m equally confused :). This is during a normal refresh driver tick
10:10smaugthe bug doesn&#39;t have good stack
10:10smaugI mean, it is cutting all the interesting bits
10:10smaugor, hmm
10:10smaugmaybe not
10:10smaugthat is just a tick
10:11emiliosmaug: I got back with rr to a point where down
10:12emiliosmaug: err, where the initial &quot;restyle root&quot; was set, and the situation doesn&#39;t change, fwiw
10:14smaugemilio: so when the root is set, binding parent is still in document I assuem
10:14smaugassume
10:14smaugcould you track when it&#39;s UnbindFromTree is called?
10:14emiliosmaug: nope
10:14emiliosmaug: (it&#39;s not in the document already, though the child claims to be)
10:14smaugwhat, binding parent isn&#39;t ever in document?
10:15smaugwhen do we then set the restyle root
10:15emiliohttps://www.irccloud.com/pastebin/UeMm3crT/
10:15smaugis this about XBL or about native anonymous content?
10:16smaugah, this could be about both
10:16smaug<video>
10:16emiliosmaug: the binding parent is a xul:videocontrols fwiw
10:17smaugso, we end up calling UnbindFromTree for xul:videocontrols at some point. That element is native anonymous
10:18emiliosmaug: should I try to track that UnbindFromTree call down?
10:19smaughttp://searchfox.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#180 should get called
10:19smaugbut does it not propagate to the xbl part
10:20smaugemilio: my guess is this http://searchfox.org/mozilla-central/source/dom/base/Element.cpp#1936
10:20smaugjust a guess
10:20smaugthat runnable does http://searchfox.org/mozilla-central/source/dom/base/Element.cpp#1810
10:21smaugbut it is not exactly synchronous
10:22emiliosmaug: hmm... I see. I&#39;ll try to poke at it a bit more, let&#39;s see. Thanks!
10:22smaugso we call http://searchfox.org/mozilla-central/source/dom/xbl/nsXBLBinding.cpp#825 and there unbind, but using a script runner
10:26emiliosmaug: you&#39;re right
10:26smaugperhaps we should unbind xbl content synchronously
10:26smaugbut just do rest of the xbl uninstallation using script runner
10:36emiliosmaug: hmm... so the old style root, which is what is failing, is _not_ native anonymous, and all the runnables have run by the point we hit the assertion
10:36emiliosmaug: so something else is going on I think
10:36smaugwhat is the old style root?
10:36smaugthe xul element?
10:36smaugthat sure should be nac
10:36emiliosmaug: is a <div>, whose binding parent is the xul element
10:37emiliosmaug: and the binding parent is unbound already
10:37smaughmm
10:37smaugsome really racy case
10:41emilioso the <xul> element gets unbound from nsVideoFrame::DestroyFrom, which makes sense
10:45emilioThat posts the runnable...
10:47emilioThat runnable runs...
10:49emiliosmaug: so that runnable runs, but it claims that binding->mContent is null
10:49emiliosmaug: (that binding is just the result from calling `content->GetXBLBinding()`
10:50emiliosmaug: since `mContent` is null we never run `UninstallAnonymousContent`
10:50smaugaha
10:51smaugso then you need to track why mContent is null :)
10:51emiliosmaug: yup, on it... :)
12:13emiliosmaug: anyway, if you can give me a test-case, I can try to optimize it
12:14smaugemilio: I was running https://bug1347525.bmoattachments.org/attachment.cgi?id=8864478
12:14firebothttps://bugzil.la/1347525 FIXED, nobody@mozilla.org Setting innerHTML slower than in Chrome
12:15smaugemilio: and focused on the set_innerHTML in the profiler
12:16smaugemilio: anything under ContentAppended should be really fast
12:16smaugso if you can make things async or something, that would be great
12:19emiliosmaug: When can GetXBLBinding()->mContent != this?
12:20smaugemilio: like always? GetXBLBinding->mBoundElement == this
12:21emiliosmaug: shouldn&#39;t we clear anon content from mBoundElement then? The anon content I&#39;m looking at is rooted at mBoundElement
12:21smaugwhat you mean by &quot;rooted&quot;
12:22emiliosmaug: The root of the subtree of anon content ends up being `mBoundElement`
12:22* emilio looks closer
12:22smauganon content has GetParent() pointing to mBoundElement?
12:23smaugmBoundElement doesn&#39;t really know about XBL anon content, except via XBLBinding
12:23smaugbut top level anon content in XBL has parent pointer pointing to mBoundElement
12:25emiliosmaug: yes, GetParent() ends up pointing to (nsXULElement *) 0x7f11f3310b80
12:25smaugso UnbindFromTree hasn&#39;t been called on the mContent
12:26emiliosmaug: that XulElement->GetXBLBinding()->mContent is (nsXMLElement *) 0x7f11e5534c50
12:27smaugis it possibly xbl:content element
12:27* smaug can&#39;t recall if we changed the setup there. Or we did, but how..
12:27emiliosmaug: yes, it is
12:28smaugso, isn&#39;t the question still that why mContent is null when we try to uninstall the XBL binding
12:28smaugsince uninstalling would call UnbindFromTree
12:28smaugbut I still think that we probably want to call UnbindFromTree synchronously on xbl stuff when it is called on bound element
12:29smaugand not use scriptrunner for anything but uninstalling
12:29smaugthat is double work though, in some sense
12:31emiliosmaug: so, I think we&#39;re posting multiple runnables for the same binding, but what I&#39;m seeing doesn&#39;t make much sense to me, let me try to figure out where they are coming from
12:33smaugemilio: FWIW, I have the gut feeling the innerHTML issue is at least partially a regression from bug 1383332
12:33firebothttps://bugzil.la/1383332 FIXED, bobbyholley@gmail.com stylo: Maintain a restyle root and use it to cull the traversal
12:33emiliosmaug: sure, I mean... that&#39;s where all this machinery is introduced
12:40emiliosmaug: if you have a release build handy, can I ask you to try a patch for the innerHTML thing?
12:40emiliohttps://www.irccloud.com/pastebin/HVdOACVp/parent.patch
12:40emiliosmaug: ^
12:40smaughmm, what you mean with release build?
12:40emiliosmaug: an opt build
12:40smaugoh, sure
12:40emiliosmaug: something you can profile on, I haven&#39;t got an up-to-date one :)
12:46smaugFWIW, the profile was done using Nightly (pgo)
12:46smaugbut my local build isn&#39;t pgo
12:50emiliosmaug: yeah, that&#39;s probably not important, if we&#39;re hitting that case, the improvement should be pretty big
12:51emiliosmaug: fwiw, forget about the multiple runnables, I was getting confused by a lot of other XBL bindings for scrollbars going away
12:51emiliosmaug: so yeah, we&#39;d need to unbind sync that anon content, I think
12:53smaugemilio: so your patches makes FF to crash. Apparently the parent process, and some child process stays alive
12:54smaugah, the child process didn&#39;t stay alive this time
12:54smaugbut anyhow, crash
12:54emiliosmaug: whoops, I know why
12:57emiliosmaug: anyway, will look into it, going to fix bugs one by one so they don&#39;t pile up :)
12:58smaugok, thanks :)
13:07emiliosmaug: would you be able to review the sync unbinding patch? Who should I flag?
13:08smaugI could review
13:08smaugis the patch ready?
13:08smaugI could open my queue for a minute
13:08emiliosmaug: Patch is up in bug 1397983 I think you don&#39;t need to open your queue to go there and stamp it
13:08firebothttps://bugzil.la/1397983 NEW, emilio@crisal.io stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::Conten
13:09emiliosmaug: will try to find a crashtest that repros properly (this one requires hovering and such), but that doesn&#39;t need to block I guess
13:10smauglooking the patch
13:10smaugemilio: and better to push this to try with all the possible tests enabled
13:10emiliosmaug: yes, I&#39;ve already done that, looks reasonably green so far
13:11emiliosmaug: (but hasn&#39;t finished 100%)
13:13emiliosmaug: of course s/UnbindToTree/UnbindFromTree in the commit message, sigh
13:13* emilio should learn how to type eventually
13:14smaugemilio: hmm, so I&#39;m surprised if this works.
13:15smaugUninstallAnonymousContent cut mContent->parent link
13:15bkellyI guess speedometer uses geometric mean now?
13:15emiliosmaug: it does, it seems, though I wondered if I should clear out mContent, somehow
13:15smaugI&#39;d expect you need something similar to UninstallAnonymousContent, but which doesn&#39;t set parent to null, but only document
13:15smaugbkelly: yes
13:15smaugall the numbers changed
13:16smaugbkelly: also, looks like different numbers of tests are being run
13:16smaugI think we used to run 900 tests, then it dropped to 450 and then 480
13:18bkellysmaug: where is our hosted revision of speedometer these days?
13:18emiliosmaug: Hmm, why wouldn&#39;t we want to clear the parent link?
13:18smaugbkelly: https://mozilla.github.io/arewefastyet-speedometer/2.0//
13:19bkellythanks
13:19smaugemilio: isn&#39;t it surprising that when xbl destructors run, you can&#39;t get from anonymous content to your actual element
13:19smaugthe bound element
13:19smaugaka, &#39;this&#39;
13:20emiliosmaug: oh, I see what you mean, I didn&#39;t know about XBL destructors (though they sound kinda terrifying)
13:20emiliosmaug: so I want to basically null unbindfromtree with aNullParent == false, I guess?
13:20emilioerr s/null//
13:20smaugemilio: yeah, I think there is something similar for ShadowDOM nearby in that code
13:21smaugyeah, bottom of the method
13:24smaugone day I&#39;ll figure out what https://health.graphics/quantum measures
13:24emiliosmaug: you mean all the UpdateInsertionParent / ClearInsertionPoints stuff?
13:24smaugspeedometer % dropped there again
13:24smaugemilio: about shadow DOM?
13:25smaugemilio: I mean the UnbindFromTree part
13:25emiliosmaug: ohh, I was looking at `nsXBLBinding::ChangeDocument`
13:25bkellyI guess we had optimized some of the dominant test cases more... with geo mean we are not as close to chrome any more
13:25emiliosmaug: k, makes sense, thanks
13:26bkellyfrom 84% to 76%
13:31emiliosmaug: do we need to do the RemoveSubtreeFromDoc bits? If so, may as well pass the boolean to UninstallAnonContent.
13:31smaugemilio: I think I&#39;d do that
13:54farrebkelly: do you have 2 seconds to look at Bug 1398109?
13:54firebothttps://bugzil.la/1398109 NEW, afarre@mozilla.com Crash in nsPIDOMWindowInner::UpdateWebSocketCount [clone .cold.576]
13:55farrebkelly: it&#39;s just a missing null check
13:55bkellyfarre: r+
13:55farrebkelly: thanks
14:04bkellyfroydnj: how would you feel if I add some MOZ_DIAGNOSTIC_ASSERT to nsPipe code?
14:04froydnjbkelly: I think I would feel OKish about that
14:04bkellyfroydnj: this bug is worrying me... and we only trigger these assertions in debug builds https://bugzilla.mozilla.org/show_bug.cgi?id=1398109
14:04bkellyno idea how much it is affecting shipping code
14:05froydnjbkelly: that bug that farre just fixed?
14:05bkellyfroydnj: sorry, no... https://bugzilla.mozilla.org/show_bug.cgi?id=1397595
14:05firebotBug 1397595 ASSIGNED, bkelly@mozilla.com Intermittent extensions/cookie/test/browser_test_favicon.js | application crashed [@ nsPipe::Advance
14:05froydnjaha!
14:06bkellyI&#39;m going to write a patch to tightern assertions and push it... and leave the bug open
14:14bkellyfroydnj: also, now that we don&#39;t expose nsPipe directly to addons... I think I can convert these soft NS_ASSERTIONs to hard assertions as well...
14:25emiliosmaug: so, took a look at the innerHTML thing, can you check if this helps? (I&#39;m still doing an opt build...)
14:25emiliohttps://www.irccloud.com/pastebin/1Yr4YEJr/root.patch
14:25smaugemilio: just doing another build, so may take some time
14:28emiliosmaug: oh, ok, nvm then.
14:53smaugemilio: hmm marquee :/
14:53smaugemilio: does that test crash?
14:53smaugor fail in some other way?
14:54emiliosmaug: it asserts with the pretty &quot;This will end badly&quot; assert from SetPrimaryFrame...
14:54smaug:)
14:54emiliosmaug: presumably because we try to construct frames after we&#39;ve unbound, which sounds exciting
14:59emiliosmaug: I&#39;ll take a look, why would we even try to construct frames for that? sigh... Every XBL related patch ends up being harder than expected somehow :-)
15:20bkellyI wish nsTArray::RemoveElementsBy() returned the number of elements removed
15:22froydnjbkelly: why?
15:22bkellyfroydnj: so I can more easily assert whether anything was removed or not
15:22bkellywithout having to write extra code to do it in my lambda
15:23bkellyfroydnj: you&#39;ll see in the patch I&#39;m going to ask you to remove!
15:23froydnjbkelly: RemoveElement() returns a bool to indicate whether it removed something, don&#39;t see why RemoveElementsBy couldn&#39;t do the same
15:23* froydnj enjoys removing patches more than reviewing them
15:24bkellyfroydnj: some re- work I guess
15:35jgrahambkelly: https://pastebin.mozilla.org/9031864 has a more detailed version of the numbers I had yesterday + mochitest
15:37bkellyjgraham: ok... I still think its hard to infer anything useful from this kind of analysis
15:38bkellyfor example, if we remove some nsI xpcom class it might end up roto-tilling a bunch of tests
15:39jgrahambkelly: Right, I mean I agree it&#39;s not the be-all and end-all. It&#39;s a starting point for further investigation. But it at least tells me we are still creating a lot of mochitests even in areas where there is wpt coverage
15:40jgrahamSo it suggests places to dig in and see what those tests are to find out if it&#39;s technical problems or cultural ones
15:40bkellyjgraham: I don&#39;t think we will ever stop creating mochitests... anything that needs special powers to be tested will end up there... some browser vendors don&#39;t even want edge case type bug checks in WPT, only spec compliance stuff there
15:41jgrahamThose browser vendors are wrong
15:41bkellyfor example, if we crash when we put a large response body in Cache API, should that be a WPT or a mochitest? I have made it a WPT test in the past, but I think safari folks would say it should be a browser-specific regression test
15:42jgrahamThen maybe I should talk to the Safari folks. I&#39;m totally in favour of that kind of thing being a wpt test
15:43jgrahamSpecial powers is interesting, because not all special powers are equally special. Some things apply to all implementations but just can&#39;t be exposed to normal web content
15:43jgrahamTrusted events being an obvious example
15:43jgrahamThose things can be added to wpt, so hopefully the number of things that require special powers goes down
15:43bkellywe had a service worker test that required a pref to work (for referrer stuff) and it got removed at blink&#39;s request
15:44bkellyI don&#39;t think its clear that all testing should move to WPT
15:44bkellysorry, but I have to run to get a gift for my daughter&#39;s birthday... I&#39;ll be back later
15:44jgrahamSo I can understand not wanting tests that don&#39;t correspond to a possible shipping configuration
15:45jgrahamBut generally I&#39;m interested in those cases, because part of the long term plan is to make browsers more testable
15:46jgrahamSo maybe we have to consider adding test APIs or whatever. Obviously if stuff is really implementation-specific then it&#39;s not a good candidate for wpt. But I would expect that&#39;s a minority of tests, and it&#39;s clear we&#39;re not there yet
18:10bkellywow, the move to geo mean had a big impact on 32-bit for some reason: https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=score
18:25bkellythis seems like it could be good or bad: https://developers.googleblog.com/2017/09/introducing-mobile-web-specialist.html
18:30jgrahamIs the course there really a list of links to MDN / developers.google.com?
21:00bkellyINFO Passed: 728
21:00bkellyINFO Failed: 0
21:00* bkelly laughs at the &quot;Todo&quot; line.
21:06qDotover 9000?
21:11bkellyqDot: no... it was only 70 or so... but I don&#39;t think those numbers ever go down
21:38bkellyI love having to push to windows try just to get non-e10s tests... what joy... what efficiency
9 Sep 2017
No messages
   
Last message: 12 days and 6 hours ago