mozilla :: #content

17 Apr 2017
09:40smaugfarre: I can't now find your bug, but should we have the timer telemetry per tab and not per window?
09:40smaugwe after all throttle tabs and not windows
10:49smaugor also per tab, now only per window
14:46bkellynew update dialogs in nightly seem nice
15:35ehsanasuth: is expected to be fixed for 55?
15:35firebotBug 1231222 ASSIGNED, perform service worker interception in parent process nsHttpChannel
15:36asuthehsan: yes, expected to go up for review in the next few days
15:36asuthor have the reviewer's requests satisfied, more specifically
15:37ehsanso the "in the mean time" in means in the few days before the patch lands?? :(
15:37firebotBug 1355608 NEW, Permissions aren't sent to the content process when intercepting a fetch request
15:38ehsanbkelly: asuth: ^
15:38bkellyehsan: I don't think that just bug 1231222 is adequate to remove the need for the bug 1355608 solution
15:38bkellybut maybe it is
15:39ehsanbkelly: the regression here is going to the parent process, isn't it?
15:39bkellythe regression?
15:39ehsanbkelly: the performance regression you're objecting to in the comment?
15:39bkellyehsan: the service worker script won't be running in the parent process... and thats what needs the permissions
15:39ehsanbkelly: I understand that
15:40ehsanbkelly: mystor's original solution involved sending a message to the parent, which you rejected due to that regressing perf because we currently intercept fully in the child
15:40ehsanwhich bug 1231222 will change
15:40ehsanis there something I'm missing?
15:41bkellyehsan: sending a message to parent and blocking the fetch event until it returned, yes
15:41bkellyehsan: it wasn't the message to the parent that bothered me... it was stalling requests during load when main thread is saturated
15:42mystorbkelly: Basically with the interception happening in the parent, when the message is sent to the child to actually start performing the interception I can send down permissions at the same time.
15:42ehsanbkelly: stalling which requests?
15:42bkellyehsan: all of them being controlled by a service worker AFAICT
15:42ehsanthere's no stalling
15:42ehsanI think you may have misunderstood mystor's solution
15:42bkellyehsan: I asked in the bug if there would be an async dispatch to main thread in all cases and he said yes
15:42mystorbkelly: Permissions are quite fast to send down (most people don't have many at all). It would just be a second message being sent down next to the service worker "do a fetch interception" message
15:43mystorbkelly: I don't think I said that?
15:43bkellyehsan: which means increases main thread jank for all network requests being controlled by a service worker
15:43bkellymystor: you said you could fix it not to do that if the permissions were already there... but you said the initial patches did do that
15:43ehsanbkelly: I don't see that comment in the bug...
15:43mystorbkelly: I'm not proposing those patches anymore - that was a temporary hack which I was implementing before we did the fetch interception in parent process thing
15:44ehsando you mean
15:44ehsanbkelly: see the second part ;)
15:44mystorbkelly: That won't be a thing once we get parent process fetch interception
15:44bkellyehsan: he chose not to do that but the other solution
15:44ehsanmystor: I'm gonna r- your patch, since it seems you guys miscommunicated ;)
15:44ehsanbkelly: yeah :)
15:45ehsanI'm glad I asked!
15:45ehsanbkelly: asuth: mystor: thanks!
15:45bkellyehsan: I still think putting any main thread turns in here is a bad idea when we have large sites complaining to us our service workers are already slow
15:45bkellybut whatever
15:45mystorehsan: The patch I r?-ed you on was a temporary hack to allow me to land the code before asuth landed his stuff which I could replace once asuth landed his stuff?
15:45ehsanmystor: wait
15:45mystorbkelly: With the patch ehsan is looking at we don't spin the event loop at all?
15:46bkellymystor: yes, but ehsan doesn't like it
15:46ehsanwell we send down all permissions
15:46bkellyif I'm reading all this right
15:46ehsanit's not like the patch is so great :P
15:46ehsan*all cookie permissions
15:46mystorehsan: We send down all "cookie" permissions
15:46mystoryeah, it's a shitty patch - but it will never ship outside of nightly hopefully
15:47ehsanmystor: ok, that's fine by me
15:47ehsanmystor: but now I'm confused as to what will happen after this patch lands...
15:47mystorehsan: Ideally asuth will land his thing and we can get this removed before 55 goes to beta
15:47ehsando we have a path towards ripping it out?
15:48ehsanmystor: given that permissions do need to be read on the main thread anyway and bkelly doesn't like that
15:48mystorehsan: is the bug for ripping it out
15:48firebotBug 1355899 NEW, Stop eagerly transmitting the cookie permission to the content process for service workers
15:48ehsanyeah I saw the bug
15:48ehsanI'm asking about how to solve this
15:49froydnjmystor: "it will never ship outside of nightly hopefully"...famous last words :)
15:49bkellyhow does this work in FF54?
15:50bkellysync IPC?
15:50mystorbkelly: In FF54 we do sync IPC to the parent process when the permission manager is initialized
15:50bkellypre-positioning permissions seems incrementally better than sync IPC
15:51mystorbkelly: Once we get parent process interception we can do the send down permissions only as needed thing at 0 overhead
15:51mystorbkelly: Basically to do the interception, we send down a message to nsHttpChannel which is on the main thread
15:51mystorbkelly: In that code where nsHttpChannel is deciding that it needs to do interception it will call ContentParent::TransmitPermissionsForPrincipal
15:52mystorbkelly: That method will check if the content process already has all of the necessary permissions, if it does it will do nothing, otherwise it will send a message to the content process with the permissions
15:53mystorbkelly: Then we will send down the normal "start your service worker and intercept" message
15:53mystorbkelly: The content process will receive this message (probably on the main thread because IIRC you can only start service workers on the main thread right now)
15:54mystorbkelly: Then it will read the permission from the permission manager, and start the service worker
15:54mystorbkelly: If that overhead of sending down a seperate message is unacceptable, we can do it with even less overhead
15:55mystorbkelly: The only required permission IIRC to start a service worker is the "cookie" permission, which we could instead read in the parent process (which already has all of the permissions) and send down as part of the "start your service worker" message
15:55mystorbkelly: Which would mean that we have even less overhead
15:55bkellycool, thanks
17:43* bkelly remembers he has easter candy...
18:44qDotAnyone know who I should talk to about XHTML parsing?
18:47bkellythat doesn't sound fun
18:48bzqDot: um...
18:48bzqDot: peterv
18:49bzqDot: or me, or maybe smaug
18:49bzqdot: But peterv knows the expat stuff by far the best
18:49bzqdot: That said, is your problem really specific to (1) XHTML and (2) parsing?
18:51qDotSorta not really. Bug 1351652
18:51firebot NEW, [e10s] Object element sends extra requests
18:51qDotBasically, someone is using an xhtml url as a data attribute for an object tag, but the xhtml document is null, which is invalid xhtml.
18:52qDotHowever, we're the only ones that seem to think that. All other browsers just show a blank page and don't yell about it.
18:53bzOK, so this is about XML parsing in general
18:53bzOther browsers really don't report XML parsing errors at all
18:53qDotWow uh ok, edge even injects its own dom tree? wtf.
18:53bzOr at least are not consistent about it
18:54bzfile:///Users/bzbarsky/baz.html reports an error in Chrome
18:54bzfile:///Users/bzbarsky/baz.html does not
18:54bzfile:///Users/bzbarsky/baz.html does
18:54* bz sighs
18:54bz<iframe src=&quot;data:text/xml,<abc&quot;></iframe>
18:54bzand <iframe src=&quot;data:text/xml,&quot;></iframe>
18:54bzand <iframe src=&quot;data:text/xml,abc&quot;></iframe>
18:55bzWhy is e10s relevant here?
18:56qDotOP was saying that it only happened in e10s. I haven&#39;t checked non-e10s cases yet.
18:58bzSpec for this is and
18:59bzThe former says &quot;Error messages from the parse process (e.g. XML namespace well-formedness errors) may be reported inline by mutating the Document.&quot;
18:59bzSo in other words, UAs can do whatever
19:00annevkHmm seems bad
19:00qDotHoo boy, xml parsing specs. How much more monday morning can it get.
19:00bzI should note that different XML parsers may even treat &quot;no input&quot; differently in terms of whether it&#39;s a parse error or not...
19:00* bz has no idea
19:01froydnjbut, but, xml error handling is totally specified!
19:01bz var p = new DOMParser();
19:01bz var d = p.parseFromString(&quot;&quot;, &quot;application/xml&quot;);
19:01bz alert(d);
19:01bzIn Gecko this throws
19:01bz&quot;XML Parsing Error: no root element found
19:01bzLocation: file:///Users/bzbarsky/baz.html
19:01bzLine Number 1, Column 1:&quot;
19:01bzIn Chrome it alerts an XMLDocument.
19:02bzannevk: ^
19:02mccr8fwiw erahm is also an XML peer now. :)
19:02mccr8He&#39;s at least familiar with the low level parsing stuff.
19:10erahmbz: yeah not sure how far down that goes (if it even hits the parser), we use expat, I think chrome uses libxml2 for the most part
19:10bzcorrect on expat and libxml2
19:11bzannevk: I think that testcase I pasted above not being interoperable is a problem....
19:11bzAnd right, I need to remember erahm can be on the hook for this stuff now
19:11bzerahm: The empty input case does hit the parser in our case, I think...
19:11* bz is not sure
19:12erahmbz: oh yeah that warning comes from the parser
19:14erahmbz: although I didn&#39;t get a warning from a local build...
19:21gandalfwhat&#39;s the reason we have document.onLoad but window.onUnload ?
19:22gandalfif I register observers in document.onload, should I unregister them in window.onunload to avoid leaks?
19:22gandalfor is there a different way (I&#39;m in XUL for now, but will expand to XHTML/HTML later)
19:42bzgandalf: I can try to answer those questions once I&#39;m done with this meeting...
20:01bzgandalf: you still there?
20:07mconleyehsan: hey - I suspect you&#39;re overloaded, and don&#39;t have time to give feedback on my patch for bug 1336763. Do you have a recommendation for someone else who&#39;d be good to look at that stuff (who is not smaug, who i suspect is similarly overloaded)
20:07firebot ASSIGNED, Only message for permitUnload on tabs that have indicated that they contain a frame that has a befor
20:09mconleybillm: ^-- would you have the time to give feed on my patch in bug 1336763?
20:10billmmconley: sure, but probably not today. maybe tomorrow.
20:10gandalfbz: here!
20:10bzgandalf: so.. we have no document.onLoad thing that I know of
20:10gandalfoh, right. window.onload.
20:11gandalfis window and document interchangeible?
20:11bzgandalf: um... no
20:11bzgandalf: window.onload and window.onunload are things
20:11bzgandalf: What sort of observers are you registering and how?
20:12mconleybillm: sounds good, thanks
20:13gandalfso, I have Localization.registerObservers and Localization.unregisterObservers
20:13gandalfthe l20n.js file is added as <script> to a XUL/XHTML/HTML document
20:13gandalfand it&#39;s a polyfill for the future WebL10n API
20:14gandalfit adds document.l10n API
20:14gandalfand it listens to two ObserverService events
20:14gandalfbut when the document is closed, I need to unregister the observer to prevent leaking
20:14bz ObserverService.addObserver(this, &#39;l10n:available-locales-changed&#39;, false);
20:14bzOK, so this adds the object, as a strong observer
20:14bzso yes, to avoid leaking you need to removeObserver somewhere
20:15bzbeforeunload is not the right place to do it
20:15gandalfI couldn&#39;t find any docs on when should I go for weak observer
20:15gandalfwhat is then? :)
20:15bzweak observer is for cases when you don&#39;t want the observerservice itself to keep you alive
20:16bzgandalf: nothing
20:16bzgandalf: That is, nothing is the right place given that this is document-associated
20:16gandalfthat doesn&#39;t sound promising. :(
20:16bzgandalf: What calls createLocalization?
20:16gandalfnothing, it&#39;s a running code
20:16gandalfit just executes inline
20:17bzOh, I see
20:17bzSo it only gets executed once
20:17bzAt the point when your <script> element runs
20:17bzWhen is your <script> element injected, then/
20:17gandalfin every .xul, .xhtml and .html file we&#39;ll want to localize using the new API :)
20:18bzYes, but _WHEN_?
20:18ehsanmconley: you&#39;re wrong that bug is pretty important :)
20:18bzCan it be injected multiple times per window?
20:18bzPer document?
20:18ehsanI&#39;ll look right now
20:18ehsanmconley: oh
20:18ehsanmconley: my review request is already gone :)
20:19ehsan2 mins ago
20:19ehsanshould I look?
20:19gandalfthat&#39;s what I do now:
20:19mconleyehsan: if you&#39;d like. I&#39;ll take whatever I can get. :)
20:19gandalfit can only be added once per document (I&#39;ll add checks to throw an error if document.l10n already exists)
20:20bzgandalf: OK.
20:20bzgandalf: So once per window, but not necessarily once per document....
20:21* bz sighs
20:21bzWhat question am I really trying to answer?
20:21bzAm I trying to answer the &quot;if we only do this in our chrome, via certain very constrained things&quot; question?
20:21gandalfhow should I register and unregister the observer in the lifecycle of the document
20:21bzOr the &quot;if we expose this on the web and have to handle all the edge cases?&quot; question?
20:21gandalfonly in our chrome
20:21bzgandalf: you can&#39;t, that was my point
20:22bzgandalf: we have no JS-visible events for the _document_ lifecycle.
20:22ehsanmconley: do you have f2f time?
20:22bzgandalf: If you only care about our chrome, so you&#39;re doing dealing with and whatnot....
20:22gandalfok, can you give me any pointers for how should I refactor my code then?
20:22bzgandalf: then just unregister the observer in a pagehide listener
20:22mconleyehsan: yes
20:22bzgandalf: on the window
20:22ehsanmconley: you mind coming over pleasE?
20:22bzgandalf: document.addEventListener(&#39;beforeunload&#39;, document.l10n.unregisterObservers);
20:23bzgandalf: Just to be clear, that line is buggy anyway, even if it were done off pagehide
20:23gandalfyeah, I know :)
20:23mconleyehsan: sure thing, be there in a sec
20:23bzgandalf: second arg should be document.l10n.unregisterObservers.bind(document.l10n) or something
20:23gandalfsorry, it&#39;s a WIP
20:23gandalfok, so I should replace it with pagehide event listener on window?
20:24gandalfand registering in pageshow?
20:24bzgandalf: Keep registering where you&#39;re registering now
20:24bzgandalf: unregister in pagehide
20:24gandalfok, thanks!
20:24bzgandalf: Good enough for chrome, not for the web...
20:25gandalfthose are Gecko events
20:25gandalfso the web content bindings won&#39;t use them.
20:25gandalfthank you!
20:31gandalfbz: any idea why the pagehide callback would be called twice when I open new window? (I test by adding l20n.js to browser.xul)?
20:31gandalfit does call &quot;registering callback&quot; when I open new window, but then it calls pagehide callback twice.
20:34bzgandalf: it calls page_hide_ twice?
20:35bzgandalf: on what windows? With what stacks?
20:35gandalfI added the <script> to browser.xul
20:35gandalfwhen I open first window, it calls my createLocalization, it calls &quot;registering observers&quot; etc. All good
20:35gandalfwhen I open the second window, it calls createLocalization, it calls registering observers etc.
20:35gandalfand it calls pagehide callback twice.
20:36gandalfall from within browser.xul
20:36gandalfI&#39;ll check the stacks
20:36gandalfwondering if it&#39;s some hiddenWindow.xul sheneningan
20:36gandalf(I&#39;m on linux so hopefully not)
20:41gandalfbz: minimized testcase:
20:41gandalf1) Open Firefox
20:41gandalf2) Open browser console
20:41gandalf3) Open new window
20:41gandalfResult: &quot;pagehide&quot; console.log fired twice
20:41gandalfon linux, nightly from today
20:54* bz looks
20:54bzgandalf: uh
20:54bzgandalf: You are not measuring what you think you&#39;re measuring
20:55bzgandalf: congrats. ;)
20:55bzgandalf: pagehide fires on _windows_, not documents
20:55bzgandalf: So if you&#39;re getting events there at all, they&#39;re not for the browser window
20:55bzgandalf: probably the &quot;yeah, just bubble everything from content up&quot; thing
20:55bzgandalf: add the listener on the window, check its target?
20:56gandalfbut, what window am I closing when I open a window?
20:57bzgandalf: navigations away from initial about:blank?
20:57gandalfwhy twice?
20:57bzgandalf: if you really want to know, look at
20:57bzgandalf: pinned tabs?
20:57bzgandalf: sidebars?
20:58bzgandalf: something else in your chrome?
20:58gandalfyeah, it&#39;s a blank document, both of them
20:58* bz has no idea from here
20:58gandalfit&#39;s a clean profile, so nothing else, but I get it
20:58gandalfsomething is
20:58gandalfok, will narrow down via target, thanks
20:58bzyou could save the and then set a timeout to check its location.href
20:58bzto see what got loaded in place of that about:blank
20:58bzthat might tell you where those are happening.
20:59gandalfI added console.log( === document);
20:59gandalfand it returns false twice when I open the window
20:59gandalfand twice when I close a window
21:00gandalfwhat am I doing wrong with my comparison that it doesn&#39;t give me any true?
21:01bzgandalf: the event is targeted at _windows_, not documents
21:01bzgandalf: so == document will obviously always test false.
21:01bzgandalf: because the target is a _window_
21:04gandalfhmmm shows HTMLDocument
21:05bzgandalf: hmm
21:05bzgandalf: oh, pagehide might be weird, one sec...
21:05bzAh, target is document, dispatched on window
21:05gandalfand when I close a window, it shows &quot;; twice closing &quot;about:blank&quot;
21:05bzWell, so ok, you need to compare target to document
21:06gandalfbut I&#39;m not getting an event for browser.xul document :(
21:06bzAs for why that&#39;s not fired for browser.xul... I dunno
21:06bzDo you get unload?
21:06bz(that one would be targeted at the window, I&#39;m pretty sure)
21:08gandalfbz: yeah, I&#39;m getting unload when I close a window
21:08gandalfwith a matching document
21:08gandalfshould I use it instead of pagehide?
21:09bzgandalf: as long as you don&#39;t have to worry about bfcache, you can, yes
21:09* bz does not understand why there is no pagehide
21:09* gandalf doesn&#39;t understand if he has to worry about bfcache
21:09bzIt depends on where you use this stuff
21:10gandalfbrowser.xul, aboidDialog.xul, etc.
21:10bzyes, yes
21:10gandalfFirefox UI basically
21:10bzthe &quot;etc&quot; is the important part
21:10bzIncluding UI bits loaded in tabs?
21:10bzIncludinf navigations between them?
21:10bzDo those go in bfcache (you tell me!)
21:10gandalfwell, I need to localize all of them as we&#39;ll be switching away from DTD
21:11gandalfbut I have no idea if they are going through bfcache
21:11bzwe fire both pagehide and unload from nsDocumentViewer::PageHide
21:11bzthe former via mDocument->OnPageHide
21:11bzAnd the latter directly
21:12bzStep through that code?
21:12bzAnd see why the DispatchPageTransition call in nsDocument::OnPageHide for the browser.xul document doesn&#39;t happen?
21:12bze.g. whether aDispatchTarget is null
21:13bz(which is about the only way I can see for it to not happen)
21:13bzaDispatchTarget being null in DispatchPageTransition, that is
21:53bzfroydnj: ping
23:30gandalfsmaug: ping
23:31smauggandalf: pong-ish. (you should know, it is Easter in Europe ;) )
23:31gandalfthen don&#39;t be online!
23:31gandalfdamn you ;p
23:31smaugbut ok, ask
23:32gandalfI&#39;m working on the XPCLocale observer for the intl:app-locales-changed as you suggested
23:32gandalfshould I make this new class (which is going to be the observer) a service or just a regular class that you create an instance of?
23:32smaugwell services are regular classes
23:32gandalfand if the latter, where should I store the object it creates? Inside XPCLocaleCallbacks struct?
23:32smaugI would imagine it to be just a class
23:32smaugand register it once to observer service
23:33smaugthen observer service will keep it alive
23:33gandalfthank you
23:33gandalfHappy Easter! :)
23:35* smaug sends some mmmi to everyone
23:46froydnjbz: late pong
23:46* gandalf is so scared of &quot;then observer service will keep it alive&quot; part... crosses fingers for :smaug willing to be his reviewer for this
23:47smauggandalf: that is totally fine. ObserverService explicitly either keeps the callback alive or has weakref to it
23:47smaugbetter to ensure registering happens just once
23:48gandalfsmaug: that&#39;s my POC atm -
23:48gandalfdoes it look sane?
23:49smaugXPCLocaleObserverService sounds a bit odd. XPCLocaleObserver perhaps ?
23:50smaugthat patch can&#39;t be right
23:50smaugXPCLocaleObserverService(); should be new XPCLocaleObserverService() or so, right?
23:50smaugobserverService->RemoveObserver(this, &quot;intl:app-locales-changed&quot;); in dtor can&#39;t work
23:51smaugobserverservice has strong reference to the object
23:51smaugyou just don&#39;t need that at all
23:51gandalfah, ok
23:51smaugthe object dies when observer service releases the object
23:53gandalfI&#39;m still super new to the wonderful world of C++ memory management and tend to assume that everything has to be cleaned up by hand
23:54smaugunderstanding how reference counting works is rather useful :)
23:55gandalfyeah, I think I understand that. I&#39;m just not used to programming patterns that involve retaining an object by its reference in an observer :)
23:55gandalfbut in this case it seems very clean and sound
23:59smauggandalf: doesn&#39;t JS rely on addEventListener to keep strong ref to the listener all the time
23:59smaugor onfoo handlers
23:59gandalfit does, but I never use the observer purposefully to retain the object
18 Apr 2017
No messages
Last message: 5 days and 14 hours ago