mozilla :: #content

15 May 2017
09:31smaugfarre: by any chance, are you using couple of days old Nightly?
09:31smaugor anyone
09:34heycamsmaug: I'm on 2017-05-11 if that helps...
09:35smaugheycam: want to do some random comparison
09:35smaug(that is old enough)
09:35smaugheycam: scrolling of (mochitest-plain list)
09:35smaugusing that older Nightly
09:35smaugand latest Nightly
09:35smaugis one jankier, slower, faster
09:36smaugjust kind of initial reaction
09:36smaugperhaps there is no difference
09:36* heycam downloads latest nightly
09:36smaugand don't spend too much time :)
09:38heycamsmaug: subjectively feels the same to me
09:40smaugok, thanks
10:51farresmaug: ping
10:51smaugfarre: pong
10:51farresmaug: hi! can you help me understand nsGlobalWindow tear down?
10:52farreI'm getting this:, and it's because FreeInnerObjects isn't called before ~nsGlobalWindow
10:53smaugare you sure it is about that?
10:53farreand we're shutting down
10:54farrethe assert that triggers in that call stack wouldn't have triggered if we'd called FreeInnerObjects (which calls ClearAllTimeouts, which in turn cancels and nulls the timer that we nullcheck for in ~TimeoutManager)
10:55smaugfarre: What if FreeInnerObjects has been called, but then something accesses Window again and re-creates something timeouts related?
10:56farresmaug: hmm, yep. that could be it, I guess
10:56smaugfarre: I mean, FreeInnerObjects is called when we're switching inner window, but if some code still keeps reference to the old inner window, perhaps something gets recreated
10:58farresmaug: I have a quick hack that would protect us from that
10:58farrewill try
11:26smaugbaku: is Bug 1350644 still an issue?
11:26firebot NEW, Remove the PBlob::Msg_WaitForSliceCreation sync IPC
11:26smaugmaybe it is
11:27smaugat least I see
11:31bakusmaug: this is an old issue. When janv finishes the review
11:31baku that PBlob sync method will be removed.
11:31bakucurrently it's in used only for IndexedDB + PBlob.
11:32smaugbaku: ok, great
11:32smaugI was just going through some qf:p1 bugs
11:41smaugbaku: what is the bug which blocks Bug 1350644 ?
11:41firebot NEW, Remove the PBlob::Msg_WaitForSliceCreation sync IPC
11:42smaugwant to add dependency there?
12:26bakusmaug: the refactoring.
12:26bakusmaug: it is just part of the old PBlob code. and that code is still in the repo.
12:28smaugbaku: but isn't the review for IDB happening elsewhere?
12:28bakuyes bug 1360185
12:28firebot NEW, Use IPCBlob in IndexedDB
12:28bakuI'll set it as a dependence.
12:29smaugbaku: just to make tracking a bit easier
13:38catalinb!seen bsmedberg
13:38firebotbsmedberg was last seen 2 days and 18 hours ago, saying '\o/' in #mdn.
13:39catalinb!seen rweiss
13:39firebotrweiss was last seen 41 days and 18 hours ago, saying 'Many thanks' in #fx-team.
14:05smaugcatalinb: ping
14:05smaugthe numbers look surprising
14:05firebotBug 651120 NEW, Remove DOM node child array storage
14:05smaugcomparing to other browsers
14:06catalinbsmaug: any test in particular?
14:06smaugcatalinb: well, say "Remove 10000 children from the back"
14:06smaugwhy would we be so slow
14:06smaugcatalinb: are you using opt builds ?
14:07smaugI've never seen anything like that
14:07smaugcomparing to other browsers
14:08smauginserting/removing from the beginning of child list is currently slow, but adding/removing from the end...
14:09catalinbsmaug: let me run that test with the profiler
14:09catalinbon nightly
14:10smaugcatalinb: I mean, how is Nightly taking 1700ms when Chrome test 21ms. That is like crazy difference
14:10smauger, Safari 21
14:10smaugcatalinb: or are you testing other browsers with multiplier=1 ?
14:11catalinbsmaug: no, I ran chrome and safari with 10x multiplier
14:12catalinbI'm using latest chrome release and the OS X bundled safari
14:13smaugwith the patch I'd expect tests insert/append/remove to be very close to other browsers
14:13smaug(no range involved)
14:14catalinbso for the remove from back
14:14catalinb87% is spent in IndexOfChild
14:18smaugcatalinb: so why are we spending much time with the patch?
14:18smaugdo we still have the IndexOf call there?
14:21catalinbsmaug: as far as I can tell we already call IndexOf in nsINode before nsContentUtils::ContentRemoved/Inserted, thus because IndexOfChild uses a cache there's no significant extra time spent in nsRange::ContentRemoved/Inserted>IndexOf
14:21catalinbsmaug: with the exception of the test that alternates removal of the first and last child
14:22smaugI'm not interested in nsRange case
14:22smaugjust the generic DOM perf
14:22smaugI guess the patch doesn't remove use of indexes?
14:23bkellyinteresting... I just build m-c and I get "the address isn't valid" on about:startpage
14:49mystorsmaug: Do you know off of the top of your head what the relationship between noopener and sessionstorage is? If you `"foo.html", "_blank", "noopener")` should you share sessionstorage with the page you came from?
14:50smaugI don't remember
14:51mystorsmaug: mk
15:07mystorannevk: It seems like both firefox & chrome copy sessionStorage into the newly created window when it is created with or similar. I'm having trouble finding this in the spec however :S
15:10mystorannevk: nvm, found it. I was expecting it to be in the definition of creating a new auxiliary browsing context instead of being in the sessionstorage area
15:28smaugmystor: you mean which variant of
15:28smaugor perhaps you talk about
15:29mystorsmaug: Did I use
15:29* mystor finds the names being so similar very confusing
15:30smaugyes, you did say, and "When called with 3 or more arguments, calls"
15:31smaugisn't the web fun
15:32mystorsmaug: ugh - I'm clumsy
15:32mystorsmaug: It's the worst
15:34mystorsmaug: If there's an existing type which I want to send over IPC, what is the best way for me to implement serialization/deserialization for it?
15:34mystorsmaug: Should I create a wrapper type or just implement the methods directly on it
15:35smaugwhich methods?
15:35mystorsomething like that - /me has only a vague idea of how this works
15:35* mystor was just going to copy pasta from around the codebase
15:36smaugmystor: something like
15:36smaugor perhaps is a bit better example
15:37smaug has some random ParamTraits
15:37mystorsmaug: looking, thanks
15:37mystorsmaug: Last question - this type is a single implementation XPCOM class, should I implement this stuff on the interface, or static_cast<> to the implementation before sending over IPC?
15:38smaugon the interface?
15:38smaugI would just do static_cast if needed
15:38mystorsmaug: kk, that&#39;s what I thought but wanted to check
15:39smaugmystor: is another option, but that is old code and I think not used anymore anywhere
15:39smaugstatic_cast sounds better
15:39mystorsmaug: That&#39;s the approach we take with IPC::Principal too, right?
15:39smaugcan&#39;t recall
15:40smaugah, yes
15:52annevkmystor: we should refactor that, file an issue?
15:52mystorannevk: Sure thing
15:53annevkAll action at a distance shall be removed
15:53mystorIt is quite confusing to someone like me who isn&#39;t super familiar with the specs
16:00mystorannevk: I was fairly surprised that we copy sessionStorage into the new window created with noopener
16:03annevkmystor: oh we should maybe treat that as a bug
16:03mystorannevk: Should I file it?
16:03smaugbaku: I assume &quot;should not&quot;
16:03firebotBug 1364925 NEW, navigator.sendBeacon should not force application/octet-stream as content-type for ArrayBuffer
16:03annevkmystor: maybe mention in same issue
16:04mystorannevk: Ok - I already filed that issue - I&#39;ll make it a comment
16:04annevkmystor: technically noopener means not auxiliary
16:04mystorannevk: Well, not in the spec really - the spec says that it immediately disowns, no?
16:05annevkmystor: yeah, there&#39;s another bug on that
16:05mystorah, ok
16:05mystorso comment in same issue or new issue?
16:05annevkmystor: I&#39;m getting closer, but so much refactoring and yak shaving required
16:05annevkmystor: same wfm
16:05mystorfair, I can imagine
16:06bakusmaug: err, yes
16:07smaugbaku: any idea how to test this?
16:07smaugusing wpt
16:07smaugI&#39;m not familiar enough with wpt to know whether it can deal with this
16:07bakusmaug: I don&#39;t know how to do this checks with WPT, yet.
16:43bkellyfun... my nightly stopped painting while watching the company meeting
16:47Caspy7This post on reddit is currently at the top of /r/firefox
16:47Caspy7I&#39;m wondering if Quantum DOM carries with it any security benefits
16:47Caspy7and maybe someone can comment on the topic there
16:53bkellyCaspy7: quantum DOM is not a security sandbox
16:54Caspy7bkelly: ok, thanks.
16:55Caspy7apparently Google is making iFrames OOP for security. Was wondering if there&#39;s anything in a similar realm that Firefox can do
16:56mystorCaspy7: Doing things like that is on our radar, but we aren&#39;t immediately working on it right now.
17:02* Caspy7 nods
17:02Caspy7thanks for the response
17:33bkellyehsan: just FYI, trying to get that second timer bug written today
17:48* bkelly finds some more timer deadcode to remove.
17:53jdmmystor: when would be a good time to steal your attention for reviewing my IPC cookie writeup?
17:54mystorMaybe in like 10 minutes?
17:54* mystor wants to finish something
17:54jdmmystor: sure. let me know when you&#39;re free!
17:57Caspy7Don&#39;t want to be the annoying fanboy, but wonder how&#39;s the status of Quantum DOM? Does it look like it&#39;s healthily on track for 57? Are there any metrics yet that can be seen now to indicate performance wins?
18:04mystorjdm: blob URIs don&#39;t talk to the parent either, right?
18:04* jdm does not know
18:07mystorjdm: You might not want to call the reference count on the document objects mRefCnt, as IIRC that name is special cased in some static analyses
18:08mystormaybe just mCount?
18:08mystorthat works too
18:09ehsanbkelly, cool
18:09ehsanbkelly, I should review your patches...
18:09ehsansorry for the delay
18:16ehsandoes anybody know what MOZ_DIAGNOSTIC_ASSERT means now that we don&#39;t have aurora?
18:17froydnjehsan: I thought they had been trying to make early beta the time when MOZ_DIAGNOSTIC_ASSERT was turned on
18:18ehsanearly beta?
18:18ehsanis that a separate branch?
18:18* ehsan thought that proposal was canned?
18:22ehsanbkelly, out of curiosity, any particular reason why you chose 4ms?
18:23bkellyehsan: no... but 25% of the frame seemed like it would let a reasonable amount of timers to run (if they are cheap), but also leave plenty of time for other things to happen within the frame (if necessary)
18:24ehsanbkelly, that&#39;s fair... I&#39;m gonna ask smaug to f+ that part to, I&#39;m ok with it
18:25mystorjdm: How are we handling the SetCookieStringInternal in child process stuff
18:25jdmmystor: that&#39;s a pretty broad question
18:34bkellyehsan: hmm... I didn&#39;t realize we depended on the nsTimer object to hold the Timeout linked list elements alive
18:35jugglinmikebkelly: Do you know Jake Archibald&#39;s IRC handle?
18:35bkellyjugglinmike: JakeA on freenode
18:36jugglinmikegot it, thanks!
18:37bkellyI wonder what the best way to deal with ref-counted linked lists is
18:40mystorbkelly: &quot;deal with&quot; in what way?
18:40bkellymystor: well... I want the list to own and keep the element alive
18:41bkellyI guess I can use a self ref or something
18:41mystorbkelly: Ah, but you can&#39;t do refcounting on the timer thread?
18:41mystoror is that not it
18:41bkellymystor: I&#39;m explicitly breaking the one-to-one with timer thread things here
18:41mystorah, k
18:42bkellysadly, the LinkedListElement doesn&#39;t have any hook like OnRemove() or anything
18:43mystorbkelly: The linkedlist is written such that it doesn&#39;t hold strong references?
18:43bkellymystor: correct
18:52froydnjbkelly: LinkedList supports refcounted elements nowadays, fwiw
18:52bkellyfroydnj: oh?
18:53bkellyfroydnj: awesome, thanks!
18:53bkellyfroydnj: looks like there is only one consumer so far... so I don&#39;t feel too bad not seeing it
18:54froydnjbkelly: yeah, it&#39;s not exactly well-advertised
18:54smaugbkelly: I assume you tested the bug 1343912 with some real world web sites
18:54smaughow does it behave?
18:54firebot ASSIGNED, TimeoutManager::RunTimeout() should limit consecutive callbacks using a time budget
18:55smaugdo we end up running out of budget often in such way that next timeout isn&#39;t run?
18:55bkellysmaug: I tested it with my stress tests and the thing
18:55bkellysmaug: we should yield much, much less than we do today
18:56bkellyyou can see that pretty clearly on
18:57smaugI&#39;ll try that once the patch lands to m-i :)
18:57bkellyI&#39;ll push it now
19:00mystorbz: When opening a noopener window with a name other than &quot;_blank&quot;, it&#39;s ignored and treated as _blank (assuming no matching windows exist), right?
19:08ehsanbkelly, does something blow up?
19:10bzmystor: per spec, in our impl, or platonically?
19:10mystorbz: How about we conquer each in turn, because the way you&#39;re asking makes it sound like that&#39;s going to matter :S
19:11bzSure will
19:11bzSo per spec, it&#39;s all borked
19:11mystorbz: I&#39;ve been reading the noopener specs, they&#39;re pretty bad
19:11bzThat&#39;s because mkwst never actually merged any of his prs
19:12bzor otherwise take action on any of the things we agreed on before we implemented
19:12bzAnyway, that&#39;s the spec end
19:13mystorok - I&#39;m mostly worried about what happens once we have decided we&#39;re not targeting an existing window, because I was aware of some weirdness with noopener targeting existing iframes
19:13mystorBasically, if I `;;, &quot;apples&quot;, &quot;noopener&quot;)`, is the newly opened window&#39;s name &quot;apples&quot;?
19:14bzIn our impl, I&#39;m _fairly_ certain the behavior is like so
19:14bz1) The name is ignored when looking for an existing thing: noopener with a target other than _self/_top/_parent always opens a new thing
19:14bz2) That new thing gives the given name.
19:14bzer, gets the given name
19:14bzSo in your example, the new window gets named &quot;apples&quot;
19:14mystorbz: We do target iframes though, no?
19:14bzNot for noopener
19:14mystorbz: Or was that only with noreferrer
19:15mystorbz: great. I love consistency
19:15bzWe targeted neither at iframes
19:15bzBased on the agreement from
19:15bzBut since no one else plans to implement any of this stuff, apparently, and since the spec is bonkers
19:15mystorYeah, I remember this. I R+-ed your patch to back that out due to webcompat shenanigans
19:15bzand since there was web compat fallout, imo
19:15bzAnd various people don&#39;t implement referrer policies....
19:15bzSo there isn&#39;t even a workaround.
19:15bzWe changed it.
19:16bzSo in Chrome, if you have two links with the same target and one is noopener and one is not
19:16bzThen I&#39;m pretty sure behavior depends on the order you click the links in
19:16* mystor facepalms
19:17bzbecause they do the search, and if they find and existing thing, they just load in there
19:17bzAnd if not they kick it over to a new window which is _often_, but not _always_ in a separate process
19:17bzAnd hence not targetable.
19:17bz(Their targeting is per-process, so whether it&#39;s targetable ends up depending on process allocation policy decisions)
19:17mystorYeah - I remember the crap with process boundaries being scope for targeting
19:18bz&quot;awesome&quot; pretty much sums it up. ;)
19:18mystorbz: Ok. /me is looking into moving noopener-ed link loads into new processes
19:18mystorbz: And I was trying to figure out if I would need to pass the name over
19:18mystorsounds like I do
19:18bzProbably best to....
19:19mystorI&#39;m a tad worried about how we&#39;re going to do the popup spam stuff with these changes, but...
19:19bzWe should really think about how to rejigger the popup blocke
19:19bzer, blocker
19:20bzIt&#39;s basically &quot;whatever jst came up with oh so long ago&quot;
19:20mystorbz: Yeah
19:20bzWhy is our popup spam cap global and not per-event?
19:20bzWhy is it 20 and not 1?
19:20bzWho knows
19:20mystorIunno - I don&#39;t really know anything about how popup blocking works
19:21* bz does
19:21bzBut I&#39;m not sure that helps your life. ;)
19:21bkellyehsan: blow up?
19:21mystorbz: Alright - perhaps I&#39;ll finish this behind a pref, then we can fix the popup blocking stuff and unpref it
19:21bzmystor: makes sense
19:22ehsanbkelly, with regards to the Timeouts being held alive by the linked list
19:22bkellyehsan: no... I was just trying to figure out how to hold the timers alive now that I&#39;m remving the nsITimer for each timeout... froydnj showed me how
19:23ehsanok cool
19:30bkellybillm: can nsPIDOMWindowInner::EventTargetFor() return a different event target over time?
19:31billmbkelly: I think at shutdown it reverts to the main thread or something
19:32billmbkelly: er, no, for windows it just goes to null
19:32billmbkelly: so there&#39;s only one possible non-null result
19:33bkellyok, thanks
19:33bkellybillm: I was trying to figure out why we passed the queue and argument here... since it always seems to get the same value, more or less
19:43billmbkelly: yeah, I think it could be simplified. also, I think the code has undergone some refactoring since I did that part. maybe we needed that argument before.
19:44bkellyI&#39;m probably going to nuke that parameter
19:44billmsounds good
19:47bkellyha... the email that says &quot;goodbye to yammer&quot; got put in my spam folder
19:48mystormconley: I find this comment very confusing considering the code which immediately follows it
19:50jdmI really like the way yammer interpreted the slack channel names
19:50jdmLooking to connect with nearby employees? #undefined, #undefined, #undefined, #undefined, #undefined(remotees), #undefined, #undefined, #undefined,
19:56smaugAnyone on linux?
19:57smaugI can&#39;t reproduce bug 1336478, but perhaps someone else could?
19:57firebot NEW, Crash in mozilla::CycleCollectedJSContext::ProcessMetastableStateQueue
19:58smaugoh, hmm
19:58mystorbz: Oh goodie OpenWindowInternal calls LegacyIsCallerChromeOrNativeCode
20:03bzmystor: it&#39;s horrible
20:04mystorbz: All I want to do is send a message to the parent process, asking it to handle opening the window, and not create a gaping security hole.
20:04* mystor is going to have a hard time
20:05bzmystor: heh
20:09bkellyfroydnj: can LinkedList<RefPtr<T>> be used in cycle collection macros like nsTArray?
20:10froydnjbkelly: not yet! :)
20:12qDotsmaug: re bug 1336478, Nightly worked fine for me on linux with the STR in the bug, let me check this build I just made
20:12firebot NEW, Crash in mozilla::CycleCollectedJSContext::ProcessMetastableStateQueue
20:13smaugqDot: I think that is &quot;just&quot; a bug in NoScript
20:15qDotsmaug: Ok, yeah, loads on this build with a clean profile too.
20:16smaugthanks for testing
20:16smaugI&#39;m waiting for feedback from NoScript dev
20:18bkellytimer refcounting is crazy
20:18bz&quot;creative&quot; is the word I think you were looking for
20:52mystorI don&#39;t know what we&#39;ve done but the workday animation is much smoother for me now than it was last time I had to fill in a quarterly goal, so that&#39;s nice :P
20:55bkellymystor: ~~quantum~~
20:58mystorbkelly: I mean, it&#39;s still janky as all hell, but it&#39;s better.
21:02smaugmystor: you&#39;re filling goals so early :p
21:03* smaug will wait still for some time
21:03mystorsmaug: I got an email telling me to file my q2 goals... I ended up getting pissed off at the ui and going home instead of filing them because it deleted my goals.
21:04mystorsmaug: so Ill have to do this again in a week or so. Let&#39;s see if we can make the animation even less janky then :P
21:04smaugyou could do some profiling :)
21:07mystorsmaug: maybe when I&#39;m not so busy with other things :P
21:08bkellymccr8: is there a way for me to note a ref to a cycle collected class where that ref is owned by an object that is not itself cycle collected?
21:08mccr8bkelly: No, because that reference can&#39;t be part of a cycle.
21:08bkellymccr8: I just need to nuke it in the UNLINK?
21:09mccr8bkelly: nuke what? if it is a strong reference, we will never CC object A if a non-CCed object B has a reference to object A.
21:09bkellymccr8: so, I have nsGlobalWindow which owns TimeoutManager which owns a LinkedList of Timeout object
21:10bkellymccr8: I want to make the LinkedList<RefPtr<Timeout>> to simplify some of the refcounting
21:10bkellymccr8: is there a way to do this?
21:10mccr8bkelly: if the timeout manager is uniquely owned by the window, then you could traverse the timeouts from the window unlink
21:11mccr8if the timeout manager is itself refcounted, you need to make it CCed
21:12mccr8but if it is uniquely owned you can just pretend that the things it owns are actually owned by the window
21:12bkellymccr8: the traverse in nsGLobalWindow already iterates over the Timeout objects and does cb.NoteNativeChild(timeout, NS_CYCLE_COLLECTION_PARTICIPANT(Timeout));
21:12mccr8bkelly: so is it manually addrefing right now?
21:13mccr8bkelly: Ah, ok. If you aren&#39;t changing how the ownership works, you shouldn&#39;t have to change the CC stuff.
21:13mccr8bkelly: though ideally the list would get cleared in unlink
21:13mccr8but if it isn&#39;t right now, then I guess it is &quot;okay&quot;.
21:13bkellywell, my changes are leaking all over the palce
21:13bkellyso its not okay
21:16mccr8bkelly: well, maybe you didn&#39;t remove one of the addrefs somewhere? just replacing manual addrefs with refptr stuff shouldn&#39;t cause leaks.
21:17mccr8it does seem odd to me that the timeout manager isn&#39;t cleared in unlink.
21:17bkellymccr8: I&#39;ve been staring at it all afternoon... there are no more explicit AddRefs... but should the NoteNativeChild() in traverse mean I don&#39;t need to do anything in UNLINK?
21:17mccr8bkelly: no, it should be cleared there. Technically, you can often get away with not breaking every part of a cycle, because it&#39;ll fall apart if you just break it at the right place.
21:17mccr8bkelly: can I see your patch?
21:20firebot ASSIGNED, consider using a single nsITimer for setTimeout() in windows
21:23mccr8hmm yeah I see that there are no manual addrefs left.
21:25bkellymccr8: I&#39;ll keep looking at it
21:26bkellyideally this should require CC to clean up anyway
21:26mccr8the remove() call in the timeout unlink should be sufficient
21:26bkellymccr8: I don&#39;t see it trying to do that
21:27mccr8yeah if it never identifies the timeout as garbage it won&#39;t call that.
21:28mccr8bkelly: you do add an addref in Timeout::InitTimer, but nowhere else, so something must be going wrong with that.
21:29bkellymccr8: I commented that out and it didn&#39;t help!
21:29mccr8Very odd.
21:29bkellymccr8: I added a breakpoint to ~Timeout() and it seems to *only* be called from CC destruction
21:30bkellyanyway, I have to run
21:30bkellythanks for your help
21:30mccr8Sorry I wasn&#39;t more helpful. :)
21:54mconleymystor: hi - reading..
16 May 2017
No messages
Last message: 12 days and 3 hours ago