Currently, when you open a new window, you can briefly see the title of the first tab in the new window change from "New Tab" to "Firefox Start Page" (or "Connecting..." if it's a custom home page). Can we set the title text to the correct value directly? Can we set the page info indicator directly (when the home page is the default, so we already know what the indicator is going to be)?

(In reply to Marco Castelluccio [:marco] from comment #0) > Currently, when you open a new window, you can briefly see the title of the > first tab in the new window change from "New Tab" to "Firefox Start Page" > (or "Connecting..." if it's a custom home page). On a slow computer, "New Tab" is displayed for several seconds before it gets replaced by something else. I think just leaving the tab empty would be better than this misleading/useless string. Of course setting it immediately to the correct value would be even better.

Comment on attachment 8881957 [details] [diff] [review] Remove "New Tab" while a new window is loading Review of attachment 8881957 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! In the future, please include the bug number in the commit message of your patches. ::: browser/base/content/tabbrowser.xml @@ +1527,5 @@ > title = gNavigatorBundle.getFormattedString("customizeMode.tabTitle", > [ brandShortName ]); > + } else { > + // Still no title? Use a blank label while the tab loads. > + title = ""; This has the unfortunate side-effect of leaving the existing title in the tab when 'about:blank' (or any page with an empty title) is loaded in an existing tab. I wonder if there would be a way to avoid receiving the DOMTitleChanged message for the initial about:blank load when opening a new window. @@ +5908,5 @@ > let { restoreTabsButton } = this; > restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs")); > > var tab = this.firstChild; > + tab.label = ""; I think we can just remove this line instead of setting the label to an empty string.

(In reply to Florian Quèze [:florian] [:flo] from comment #3) > @@ +5908,5 @@ > > let { restoreTabsButton } = this; > > restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs")); > > > > var tab = this.firstChild; > > + tab.label = ""; > > I think we can just remove this line instead of setting the label to an > empty string. This is wrong when opening about:blank in a new window.

I'm

I'm un-assigning myself; for anyone who takes a look at this, I couldn't find a solution in the front-end code, or at least a solution using only front-end code (see attachment #8881957 [details] [diff] [review]).

I'll take it.

I took a slightly different approach, and less complete than looking into potential reduction of DOMTitleChange calls. There are five places where the title may be set: 1) OnLocationChange 2) addTab 3) SessionRestore 4) in the constructor of the tabbrowser 5) In DOMTitleChanged I made the following changes: 1) Set the title in OnLocationChange only if the new page is not blank or from 'about:' protocol. My reasoning is that blank/about pages will load fast and setting them to the URL name doesn't make sense. 2) I removed the setInitialTabTitle from addTab for nonBlankPage My reasoning is that it doesn't make sense to duplicate the title setting, and the tab receives "OnLocationChange" right after it, where it'll set the title to the URL anyway. 3) I removed setting the title to emptyTab label in the constructor Since every tab receives DOMTitleChanged from the sessionrestore, or from loading the page, it didn't make sense to make it flicker through emptyTab label. 4) I reordered the operations in setTabTitle It didn't make sense to first try to use the title and only then check if the page is a BlankPage. Especially, that in several cases the title of the blank page was set to a string (sic!) "undefined" before it switched to "New Tab". I suspect it's some bug coming from Activity Stream. Either way, reordering makes it clean - if it's a blank page, we'll take the emptyTab string, otherwise, we'll take the title, otherwise we'll take the URI. 5) I refactored the URI stringification a bit. Nothing major, but I think I made it less confusing by using a temporary variable while in the process. That's it. I tested the result against opening many windows, tabs, switching to new tab, home, external url, pages with no url, restoring sessions etc. I reduced the number of label updates and there doesn't seem to be any flickering anymore. I know that there's upcoming rework of the tab loading animation for Photon. I can investigate further optimizations once that is done. For now this patch I believe fixes the bug and it should be enough :)

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try searching for changes remote: abort: abandoned transaction found! remote: (run 'hg recover' to clean up transaction) abort: stream ended unexpectedly (got 0 bytes, expected 4)

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try searching for changes remote: abort: abandoned transaction found! remote: (run 'hg recover' to clean up transaction) abort: stream ended unexpectedly (got 0 bytes, expected 4)

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea1ea710de03cbd9dabdf4599822efe4abfdd65

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review174856 Thanks for the patch! r- because "New Tab" still sometimes flickers when opening a new window. Adding debug logging in tabbrowser.xml's _setTabLabel method shows that when opening a new window (Cmd + N), we set the label twice: _setTabLabel, label = New Tab _setTabLabel@chrome://browser/content/tabbrowser.xml:1545:26 setTabTitle@chrome://browser/content/tabbrowser.xml:1533:20 receiveMessage@chrome://browser/content/tabbrowser.xml:5217:34 _setTabLabel, label = Nightly Start Page _setTabLabel@chrome://browser/content/tabbrowser.xml:1545:26 setTabTitle@chrome://browser/content/tabbrowser.xml:1533:20 receiveMessage@chrome://browser/content/tabbrowser.xml:5217:34 Are you sure OnLocationChange happens soon enough? Somehow I thought it only happened once we have successfully opened a network socket to the server. ::: browser/base/content/tabbrowser.xml:1529 (Diff revision 2) > - var characterSet = browser.characterSet; > + var characterSet = browser.characterSet; > - const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"] > + const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"] > - .getService(Components.interfaces.nsITextToSubURI); > + .getService(Components.interfaces.nsITextToSubURI); > - title = textToSubURI.unEscapeNonAsciiURI(characterSet, title); > + readableUri = textToSubURI.unEscapeNonAsciiURI(characterSet, readableUri); > - } catch (ex) { /* Do nothing. */ } > + } catch (ex) { /* Do nothing. */ } > - } else { > + title = radableUri; There a typo here. But you probably already knew as lots of test failures on try are due to it.

(In reply to Florian Quèze [:florian] [:flo] from comment #14) > Are you sure OnLocationChange happens soon enough? Somehow I thought it only > happened once we have successfully opened a network socket to the server. It's actually happening even later than I thought. When opening a bugzilla bug, it takes almost 2s for the blank tab title to be replaced by the actual page title. So it seems OnLocationChange happens only once we start receiving the HTML from the server (when the "Waiting for" status message disappears).

> Are you sure OnLocationChange happens soon enough? Somehow I thought it only happened once we have successfully opened a network socket to the server. Yeah, it seems you're right, I may bring it back later. For now, I want to fix the New Tab flickering. In my testing it happens reliably if I press ctrl+n twice quickly. It opens two windows, fires OnLocationChange and DOMTitleChange to about:blank, and then fires OnLocationChange and DOMTitleChange to the about:home page. So, the situation I'm in is: 1) If you set "blank page" in your preferences the events are: OnLocationChange(about:blank) DOMTitleChange(about:blank) 2) If you set "about:home" in your preferences it most of the time is: OnLocationChange(about:home) DOMTitleChange(about:home) but if you try to open two windows quickly it is: OnLocationChange(about:blank) DOMTitleChange(about:blank) OnLocationChange(about:home) DOMTitleChange(about:home) OnLocationChange(about:blank) DOMTitleChange(about:blank) OnLocationChange(about:home) DOMTitleChange(about:home) ============= That makes it impossible (if I understand correctly) to distinguish between the "about:blank" being called on either as the final one, and as the intermediary before loading the real page. My guess is that we somehow set the page to "about:blank" when we create new content process or sth, and if things are slow enough (debug build, opening multiple windows), we end up delivering this "about:blank" all the way to the tab title. I'm wondering if there's a way to: a) Not set the page to about:blank automatically b) Somehow recognize that the "about:blank" is just temporary and we're actually loading "about:home" I'm currently sprinkling printfs around dom/base/nsDocument::DoNotifyPossibleTitleChange which fires *a lot* (like, 20 times during startup of one window with one about:home page!). Not sure if I'll get to anything, but any suggestions on where to look would be helpful :)

Ok, gave up digging into dom/base - there's a lot going on there, and I don't feel confident touching it. But I think I found a way. In the tabbrowser.xml constructor, I check `browser.startup.page` and `browser.startup.homepage`. If it's supposed to be `about:blank`, I set the title to emptyTab. This allows me to set it for new tab and new window immediately. Now I need to make sure that I do not set it when DOMTitleChange|onLocationChange switches to about:blank on the way to something else, but do set it when about:blank is the final url.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17) > I'm wondering if there's a way to: > a) Not set the page to about:blank automatically http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/base/content/tabbrowser.xml#2177-2181

(In reply to Dão Gottwald [::dao] from comment #19) > (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17) > > I'm wondering if there's a way to: > > a) Not set the page to about:blank automatically > > http://searchfox.org/mozilla-central/rev/ > e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/base/content/tabbrowser. > xml#2177-2181 I wish this worked. Unfortunately, first of all, this is only called from addTab which is called on additional tabs open, not for the first tab in a window, and what's worse, it doesn't really help as `uriIsAboutBlank` is literally `aURI === "about:blank"`. I tried to test several other params around, there's InLoadURI, mBlank and others, but they all seem to be fairly consistent in the case of a new window and not distinguish between about:blank that is loaded temporarily right before about:home is, and the one that is loaded as final. I'll keep playing with the concept of using prefs to recognize that the new tab will be about:blank.

I didn't know this nodefaultsrc attribute. Maybe we can fix it to also be used on the first tab? The patch I landed in bug 1372518 may also give ideas about how to ignore about:blank until the first real load.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20) > (In reply to Dão Gottwald [::dao] from comment #19) > > (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17) > > > I'm wondering if there's a way to: > > > a) Not set the page to about:blank automatically > > > > http://searchfox.org/mozilla-central/rev/ > > e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/base/content/tabbrowser. > > xml#2177-2181 > > I wish this worked. > > Unfortunately, first of all, this is only called from addTab which is called > on additional tabs open, not for the first tab in a window, Sure, I was merely responding to your "if there's a way..." question. Obviously we don't currently use nodefaultsrc the way you'd want to use it.

> The patch I landed in bug 1372518 may also give ideas about how to ignore about:blank until the first real load. I don't really see anything in the patch that would be useful. Do you? I'm really starting to think that it would be worth investigating this on the DOM level: 1) Why are we loading about:blank before we load the URL? It seems like it would impose a performance penalty as at least the front end code is reacting to it which adds processing. 2) Why is nsDocument::DoNotifyPossibleTitleChange called so many times? This piece of code: http://searchfox.org/mozilla-central/rev/7d43c93d35e4c0fbaba7d639278e892b4565db63/dom/base/nsDocument.cpp# is fired over 20 times during startup on my debug build. It seems that it fires with blank title at least 14-16 times, and on top of that a couple times with the product name "Nightly" and a couple times with the about:home title. If I understand correctly the goal is to set the window title to "Nightly", then load the document and switch tab to "Nightly Start Page" and window title to "Nightly Start Page - Nightly". But that means at most two calls to set the title... not 20something? Both those trails seems like potential material for Quantum Flow bugs that may impact performance on ts_paint and tpaint. :smaug - are you the right person to answer the questions? Is it worth filing bugs?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #23) > > The patch I landed in bug 1372518 may also give ideas about how to ignore about:blank until the first real load. > > I don't really see anything in the patch that would be useful. Do you? The code I added determines early which page will be loaded, displays the appropriate UI for it before first paint, and then makes the UI ignore all notifications related to about:blank until the expected page is loaded. For the tab title, you are in a very similar situation. But yes, if we could avoid these about:blank loads completely, it would be better. Less hackery.

The very initial about:blank isn't loaded, but created whenever someone accesses document property of window/docshell. Are you seeing about:blank being also loaded? If so, that could be something we could optimize out. Getting so many DOMTitleChanged events is a bit surprising. Do we change the contents of title element several times?

> The very initial about:blank isn't loaded, but created whenever someone accesses document property of window/docshell. Are you seeing about:blank being also loaded? I'm sorry, I don't know the difference. All I know is that while debugging this bug I found out that if I set the new window page to "about:home", those two places are triggered twice: - http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/base/content/tabbrowser.xml#788 - http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/base/content/tabbrowser.xml#5256 First for about:blank, and only then for about:home (see comment 17) It feels to me that the ultimate solution is for the DOM not to announce that the location is changing to about:blank and the DOMTitle changed to "" right before the location is changed to about:home and the title to "Nightly Start Page". I don't know what code attempts to access the document property of window/docshell, but I would expect this temporary about:blank to not be broadcasted as a locationchange/titlechange. STR: 1) Launch Firefox 2) Set start page to "about:home" 3) Press ctrl+n twice to open two new windows. Current result: OnLocationChange and DOMTitleChange are first called for "about:blank" and then for "about:home" Expected result: OnLocationChange and DOMTitleChange only called for "about:home". Do you think it's worth filing a bug?

> Getting so many DOMTitleChanged events is a bit surprising. Do we change the contents of title element several times? We shouldn't. There's a single document to be loaded - about:home, with a single title. But it seems that we're loading about:blank first, and only then about:home. We're also setting the title for the whole window (Nightly, and then "Nightly Start Page - Nightly" which is the active tab title combined with the app name), and I guess for every other document in the UI as well? What I know is that when starting the browser the function nsDocument::DoNotifyPossibleTitleChange is called 66 times. The function is here: http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/base/nsDocument.cpp#7170 STR: 1) Instrument the code in line 7189 with: ``` printf("DoNotifyPossibleTitleChange called with title: %s\n", ToNewCString(title)); ``` 2) Launch the browser with "about:home" set up as a start page. 3) The log shows 66 printfs, most with blank title. (as attached) I can imagine that there are SVG documents in UI being loaded etc., so maybe the only actionable thing is the problem with loading about:blank before about:home, but I'm not sure, and maybe there is something more that is actionable as well.

> The code I added determines early which page will be loaded, displays the appropriate UI for it before first paint, and then makes the UI ignore all notifications related to about:blank until the expected page is loaded. I'll investigate workarounds like this, but I hope that the full fix will not only save us from having to have those workarounds, but actually may have a positive impact on our quantum flow metrics (my logic is that if we're broadcasting about:blank, than a lot of startup code reacts to it, which is unnecessary)

http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/docshell/base/nsDocShell.cpp#8212 and http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/docshell/base/nsDocShell.cpp#2001 fires the location change for the explicitly created about:blank. (and when I say create, I mean nothing is being parsed or anything). We can't really change that, since the API contract is that one gets location change in notified when, well, location changes. Assuming this is the about:blank we're talking about, and not some explicitly loaded about:blank, you could debug why the document is created. But optimizing out its creation can be tricky.

Pity. I find the interaction between DOM and front-end necessarily suboptimal, if the DOM is broadcasting events about page loads that are not real loads from the front-end perspective. :( In other news, :florian's lead seems to be fruitful. I was able to distinguish between "real" and "fake" about:blank loads using `window.arguments === aTab.linkedBrowser.currentURI.spec`. With this weapon in hand, I hope to finish the patch tomorrow by writing a rather lengthy note about this behavior in setTabTitle.

(In reply to Olli Pettay [:smaug] from comment #29) > We can't really change that, since the API contract is that one gets > location change in notified when, well, location changes. Wait, what? We control both ends here and can in principle change anything we want with unlimited extension support dropped. Why can we not prevent the initial about:blank creation, or treat it differently with regards to notifications (which could mean not sending them at all or at least letting the consumers know that these are special)? I'm sure there are good reasons against some of these options, but I don't buy that we simply cannot change things.

Well, sure, we could change it, but inconsistently working APIs tend to lead to bugs elsewhere. Assuming we're dealing with the initial about:blank here, and we're trying to optimize that out, someone should check why it is created. (It is created on demand)

What about that idea of also having the "nodefaultsrc" attribute for the first tab created? Could it be used to prevent some of these extra loads?

:florian - Using the `windows.arguments[0]` helped me get the last bit to work! :) There's still a lot of noise in the code(*), but in my testing this patch reduces some of the noise and fixes the visual bugs in common cases. *) for example: when you're in about:newtab, and click the home button to go to about:home, the setInitialTitle is fired 4 times from the SessionRestore for about:blank before it switches to about:home

I know there is some a11y orange and ES linter orange. I'll fix it in the next round. I also tested the windows, mac and linux artifact builds and all seem to work well

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #35) > :florian - Using the `windows.arguments[0]` helped me get the last bit to > work! :) You can't reliably use it like this, see bug 1054740.

I compared the behavior of opening from session restore with and without the patch: 1) Without the patch: http://us-next.owncube.com/index.php/s/TjZcpqtIvV2nHv0 2) With the patch: http://us-next.owncube.com/index.php/s/7sp50lwFnBqilT2 both use mozilla-central from today. Things to notice: 1) Without the patch the "New Tab" is shown before the tabstrip is populated from restore. Not present with the patch 2) Without the patch the "about:blank" has an empty title until restored. Fixed with the patch 3) The Preferences icon gets reloaded when restored. Not fixed with the patch 4) "New Tab" briefly flashing when opening new window twice. Fixed in the patch :dao - what else should I test? I understand that the patch you're pointing out is also checking if sessionrestore will override the home page, but I'm not sure if that's applicable to my case. And if there's some other scenario (like opening multiple windows in session restore) that my patch doesn't cover, I'd be happy to file a new bug and work on that, rather than delaying landing of this.

I also tested restoring with multiple windows open on the reference hardware and cannot see any issue with it and can confirm that the patch fixes the flicker.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #38) > I compared the behavior of opening from session restore with and without the > patch: > > 1) Without the patch: http://us-next.owncube.com/index.php/s/TjZcpqtIvV2nHv0 > 2) With the patch: http://us-next.owncube.com/index.php/s/7sp50lwFnBqilT2 This pages seem to get stuck in a redirect loop... > 1) Without the patch the "New Tab" is shown before the tabstrip is populated > from restore. Not present with the patch > :dao - what else should I test? I understand that the patch you're pointing > out is also checking if sessionrestore will override the home page, but I'm > not sure if that's applicable to my case. Have you tried setting your homepage to about:newtab or about:blank? I should also point out that I plan on making isBlankPageURL return true for about:home in bug 1393802. (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9) > Especially, that in several cases the title of the blank page was set to a > string (sic!) "undefined" before it switched to "New Tab". I suspect it's > some bug coming from Activity Stream. Yes, it's an Activity Stream bug. I'd prefer not wallpapering over this. Changing the logic as described would prevent about:newtab from intentionally changing its title in a meaningful way.

(In reply to Dão Gottwald [::dao] from comment #41) > This pages seem to get stuck in a redirect loop... Ugh, not sure why. It works for me from all browsers and different oses. Here it is on youtube: 1) https://youtu.be/Iy0SoOdAbUU 2) https://youtu.be/iN_Ohcz9HaE > Have you tried setting your homepage to about:newtab or about:blank? Yep. Both work. Do you want me to record videos? > I should also point out that I plan on making isBlankPageURL return true for about:home in bug 1393802. I only use `isBlankPageURL` in one place and I only need it there for when the page is `about:nettab` or `about:blank`. I can fork isBlankPageURL or I can exclude `about:home` manually in my `if`. > Yes, it's an Activity Stream bug. I'd prefer not wallpapering over this. > Changing the logic as described would prevent about:newtab from > intentionally changing its title in a meaningful way. I removed the specialcasing for "undefined" string.

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review175054 ::: browser/base/content/tabbrowser.xml:844 (Diff revision 5) > } > } > > + // If the URI is not for about protocol, > + // set tab title to the loading URL. > + if (!aLocation.spec.startsWith('about:')) { If you already have a URI object, .schemeIs("about") should be faster than getting the spec and checking the string. nit: should use double quotes for strings (and I think eslint would report these single quotes here) ::: browser/base/content/tabbrowser.xml:857 (Diff revision 5) > // replaceState (bug 550565) or a hash change (bug 408415). > if (!this.mTab.hasAttribute("pending") && > aWebProgress.isLoadingDocument && > !isSameDocument) { > this.mBrowser.mIconURL = null; > + nit: don't add an empty line here ::: browser/base/content/tabbrowser.xml:1535 (Diff revision 5) > + } else if (currentURI.startsWith('about:')) { > + return false; > } else { > - if (browser.currentURI.displaySpec) { > + let readableURI; > - try { > + try { > - title = this.mURIFixup.createExposableURI(browser.currentURI).displaySpec; > + readableURI = this.mURIFixup.createExposableURI(currentURI).displaySpec; createExposableURI takes an nsIURI parameter, but you are giving it a string. ::: browser/base/content/tabbrowser.xml:2511 (Diff revision 5) > var uriIsAboutBlank = aURI == "about:blank"; > > if (!aNoInitialLabel) { > if (isBlankPageURL(aURI)) { > t.setAttribute("label", this.mStringBundle.getString("tabs.emptyTabTitle")); > - } else { > + } else if (!aURI.startsWith('about:')) { nit: use double quotes ::: browser/base/content/tabbrowser.xml:6111 (Diff revision 5) > restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs")); > > var tab = this.firstChild; > - tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"); > + > + if (window.arguments) { > + const startPages = window.arguments[0].split('|').map(s => s.trim()); Assuming that window.arguments[0] is always a string won't work. See the the code in browser.js that accesses window.arguments[0].

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review179966 No flickering anymore :-) ::: browser/base/content/tabbrowser.xml:1521 (Diff revision 7) > } > delete aTab._labelIsInitialTitle; > } > > let isContentTitle = false; > - if (title) { > + let currentURI = aTab.linkedBrowser.currentURI; "aTab.linkedBrowser" can be simplified to "browser" here. ::: browser/base/content/tabbrowser.xml:1528 (Diff revision 7) > let brandBundle = document.getElementById("bundle_brand"); > let brandShortName = brandBundle.getString("brandShortName"); > title = gNavigatorBundle.getFormattedString("customizeMode.tabTitle", > [ brandShortName ]); > + } else if (title) { > isContentTitle = true; Is there a reason why the patch makes isContentTitle false for the customizemmode case? It was true before the patch. ::: browser/base/content/tabbrowser.xml:1536 (Diff revision 7) > } else { > - if (browser.currentURI.displaySpec) { > + let readableURI; > - try { > + try { > - title = this.mURIFixup.createExposableURI(browser.currentURI).displaySpec; > + readableURI = this.mURIFixup.createExposableURI(currentURI).displaySpec; > - } catch (ex) { > + } catch (ex) { > - title = browser.currentURI.displaySpec; > + readableURI = currentURI.spec; Do we know in which case createExposableURI will throw? Is there a reason for using .spec here instead of .displaySpec before the patch? ::: browser/base/content/tabbrowser.xml:5386 (Diff revision 7) > > switch (aMessage.name) { > case "DOMTitleChanged": { > + let title = data.title; > + if (!title) { > + break; (this comment is the reason for the r-) I don't think this change is correct. Here's how I tested it: 1. load data:text/html,<title>blah</title> 2. open devtools (cmd+alt+i) 3. edit the title inside the html markup to be an empty string. Without the patch, the tab title ("blah") is replaced with the url. With the patch, the tab title remains "blah". Here is another way that doesn't involve the devtools: 1. Load about:mozilla 2. Load about:blank in the same tab. With the patch, the about:mozilla title remains in the tab after loading about:blank. ::: browser/base/content/tabbrowser.xml:6110 (Diff revision 7) > restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs")); > > var tab = this.firstChild; > - tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"); > + > + // If the first tab of the new window is going to be > + // about:blank or about:newtab, set it's title immediately to spelling: its title ::: browser/base/content/tabbrowser.xml:6112 (Diff revision 7) > var tab = this.firstChild; > - tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"); > + > + // If the first tab of the new window is going to be > + // about:blank or about:newtab, set it's title immediately to > + // improve the perceived performance. > + if (window.arguments && typeof window.arguments[0] === "string") { nit: front-end code usually uses == unless === is required ::: browser/base/content/tabbrowser.xml:6113 (Diff revision 7) > - tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"); > + > + // If the first tab of the new window is going to be > + // about:blank or about:newtab, set it's title immediately to > + // improve the perceived performance. > + if (window.arguments && typeof window.arguments[0] === "string") { > + const startPages = window.arguments[0].split("|").map(s => s.trim()); could be: let startPage = window.arguments[0].split("|")[0].trim(); (we only look at the first url, so we don't need to trim them all)

Thanks for the review Florian! (In reply to Florian Quèze [:florian] [:flo] from comment #46) > 1. load data:text/html,<title>blah</title> > 2. open devtools (cmd+alt+i) > 3. edit the title inside the html markup to be an empty string. > > Without the patch, the tab title ("blah") is replaced with the url. > With the patch, the tab title remains "blah". This is fixable, I just have to special case about:blank in avoiding setting the blank title. > Here is another way that doesn't involve the devtools: > 1. Load about:mozilla > 2. Load about:blank in the same tab. But this... cuts to the chase of the problem that I see as irreconcilable. Basically, it boils down to two scenarios: In both, user starts with a tab in "about:newtab". Then: 1) User loads "about:blank". 2) User loads "about:mozilla" (which fires OnLocationChange and DOMTitleChange for "about:blank" in the process) I can fix all the other review comments, but this one seems to be a rock-paper-scisors loop: a) I can react to "about:blank" DOMTitleChange by setting the title to "New Tab" b) If I don't react to "about:blank" DOMTitleChange c) I can react to "about:blank" DOMTitleChange by setting the title to blank Currently, nightly does (a) which causes the "New Tab" to be displayed in many scenarios where the new document is loading. New window opening, new private window, etc. I tried (b) which fixes the "New Tab" flickering on new window, but it causes the tab title not to change in result of navigating to "about:blank". (c) fixes the flicker and works with navigating to "about:blank", but causes a flicker when opening a new tab because now we do: 1) Set the title of a newly opened tab to "New Tab" instantly 2) Then "about:blank" is loaded which sets it to "" 3) Then "about:newtab" is loaded which sets it to "New Tab" again I see two solutions: - I could skip (1) That would imho result in degraded UX since the newly opening tab would have a blank title until "about:newtab" is loaded instead of being set immediately. - I could try to find a way to make a check of `if (currentURI === "about:blank" && currentURI !== urlbar.uri)` This would be imho the ultimate solution allowing us to not only fix this bug but clean the whole tabbar behavior to filter out all loads of "about:blank" that currently trigger actions in tabbrowser while they all should be filtered out (title, favicon etc.) I tried to poke at it, but I have no idea how to get the equivalent of `urlbar.uri`. What do you think?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #47) > > Here is another way that doesn't involve the devtools: > > 1. Load about:mozilla > > 2. Load about:blank in the same tab. > > But this... cuts to the chase of the problem that I see as irreconcilable. > > Basically, it boils down to two scenarios: > > In both, user starts with a tab in "about:newtab". Then: > > 1) User loads "about:blank". > 2) User loads "about:mozilla" (which fires OnLocationChange and > DOMTitleChange for "about:blank" in the process) > > I can fix all the other review comments, but this one seems to be a > rock-paper-scisors loop: > > a) I can react to "about:blank" DOMTitleChange by setting the title to "New > Tab" > b) If I don't react to "about:blank" DOMTitleChange > c) I can react to "about:blank" DOMTitleChange by setting the title to blank What about: d) Don't react to "about:blank" until a non-about:blank page is first loaded in the tab, if the initial url to load was not about:blank. This is what I was suggesting in comment 24. > - I could try to find a way to make a check of `if (currentURI === > "about:blank" && currentURI !== urlbar.uri)` > > This would be imho the ultimate solution allowing us to not only fix this > bug but clean the whole tabbar behavior to filter out all loads of > "about:blank" that currently trigger actions in tabbrowser while they all > should be filtered out (title, favicon etc.) > > I tried to poke at it, but I have no idea how to get the equivalent of > `urlbar.uri`. Maybe gURLBar.value? I'm not sure how that behaves for about:blank. Reading the value in the urlbar seems the last resort solution.

Looking at your question on IRC now ("I don't know how to react to that case and not react to "about:newtab" -> "about:mozilla" case which reports "about:blank" in the middle."). Why do we have about:blank in the middle of this? Is it because about:newtab is remote and about:mozilla is loaded in the chrome process? If so, I think that's an edge case we can ignore and let it flicker...

> d) Don't react to "about:blank" until a non-about:blank page is first loaded in the tab, if the initial url to load was not about:blank. If by "the initial url to load" you mean the window.arguments[0], then I can do that, but that seems like a very specific fix that adds to the complexity of tabbrowser.xml code. It would not fix, for example, the flicker caused by temporary about:blank while loading a new tab. I can workaround it for some cases by either - giving up on the perceived performance benefit of setting "New Tab" in `addTab` function (then the tab starts blank, goes through about:blank with blank title and only gets the title "New Tab" when about:newtab is loaded). - being ok with an intermittent flicker when in `addTab` I set `New Tab` title, then it may go through `about:blank` and switch title to blank, and end up in about:newtab setting `New Tab` string again. > Maybe gURLBar.value? I'm not sure how that behaves for about:blank. Reading the value in the urlbar seems the last resort solution. I understand that it's not pretty, but the core problem is that from the observable perspective of tabbrowser.xul many urls do load about:blank before they load their content. And unless we can recognize that one about:blank is the destination URL, while the other is just an intermediate url reported from docShell as OnLocationChange and DOMTitleChange that we should not react to, we'll always have to keep fragile dirty hacks in the code that will keep regressing. > Looking at your question on IRC now ("I don't know how to react to that case and not react to "about:newtab" -> "about:mozilla" case which reports "about:blank" in the middle."). Why do we have about:blank in the middle of this? Is it because about:newtab is remote and about:mozilla is loaded in the chrome process? If so, I think that's an edge case we can ignore and let it flicker... I don't fully understand why we have "about:blank" in the middle. I asked about it in comment 23 and :smaug tried to explain to me, but I don't fully understand the inner works of the docshell. All I know is that docshell is broadcasting to the front-end code a url that is not the url that is to be loaded. I believe that to be an architectural mistake that should have negative performance impact as various pieces of code react to the about:blank while it should be not affecting the front-end at all. I also don't know if it's only affecting about:* protocol, or is it just happening when new process is starting or sth. But I know thta, it's unfortunately affecting "about:newtab" which means that when you're opening a new tab (the most common operation in the tabbar), you're either getting delayed title (negative perceived performance), or a flicker (negative UX), or I can fix it by filtering out about:blank, but then I need to distinguish between the finalURI about:blank and the intermediary URI about:blank and only filter out the latter. What do you want me to do?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #50) > - being ok with an intermittent flicker when in `addTab` I set `New Tab` > title, then it may go through `about:blank` and switch title to blank, and > end up in about:newtab setting `New Tab` string again. I don't understand what would flicker here. The tab title for about:blank is 'New Tab' too, right? Or am I missing something here?

> I don't understand what would flicker here. The tab title for about:blank is 'New Tab' too, right? Or am I missing something here? The title for "about:blank" is "". We artificially set it to "New Tab" in tabbrowser.xml based on the URL name, but we don't have to. If we get back to doing that, then opening new tab will set "New Tab" in addTab, then when about:blank's DOMTitleChange is fired we'll once again set it to "New Tab" and then when ultimately about:newtab loads, it'll set it to "New Tab". So, no flicker. But then we need to figure out how not to set it when you load, say about:preferences, since it'll load through about:blank as well, triggering "New Tab" to flicker before "Preferences" title is loaded. And we'll need the supression in new window to avoid "New Tab" being artificially set while loading new window. I believe that recognizing the "temporary" about:blank and filtering it out from setting title would be a more reliable solution. Would you agree?

Ok, tried the gURLBar.value, but that unfortunately doesn't work well because a) in about:blank case the gURLBar.value is empty, and b) user can change the value... :smaug - is there an API that we could use from chrome content to learn what URL is the destination for the given tab at the given moment. I'm particularly talking about the scenario when docshell triggered DOMTitleChange for an about:blank which is *not* the destination. The browser.currentURI shows "about:blank" at that point, but I need to know what URL the tab is really being loaded.

That is a question to frontend folks. Gecko itself doesn't expose the current url in the parent process. (I'm not quite sure what you mean with 'chrome content' or 'destination'). Child side could of course send the url when DOMTitleChange is sent here http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/content/browser-child.js#430

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #52) > Would you agree? I think at this point I'm confused enough that I can't give any useful answer without reading the whole thread again. Does it help if we make the DOMTitleChanged message from the child process also include |location: content.document.location.href| (as suggested in comment 54) ? Unrelated: it seems activity stream is expected to replace about:home soon, which kinda changes the situation here as its title is (currently?) also "New Tab".

I updated the patch and tried it with the suggestion from comment 54. It doesn't seem to help much since the problematic about:blank doesn't use the DOMTitleChange that ends up here: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5425 but the one that ends up here: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5904 With the patch that is currently attached I see: - no "New Tab" when opening new window - no flicker when opening new tabs - proper behavior on "about:mozilla" -> "about:blank" - proper behavior when manually removing title from data: - proper behavior when restoring multiple tabs I have to say that it feels to me like something recently changed in the lower level behavior because I cannot anymore reproduce the intermediate "about:blank" before about:nettab, in new window etc. The only remaining issue where I still do see the intermediate "about:blank" and which in result causes the flicker via "New Tab" is when I set the "about:preferences" as a home page and open a new window. It goes through this DOMTitleChanged: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5904 It loads first "about:blank" and then "about:preferences". But this flicker also exists in Nightly so I'm at least not regressing. Florian, I believe it's worth landing this and possibly filing a follow up at this point. It's been a quite long process to get this patch and if we want it in 57, it should get some testing in nightly.

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review181296 (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #57) > I have to say that it feels to me like something recently changed in the > lower level behavior because I cannot anymore reproduce the intermediate > "about:blank" before about:nettab, in new window etc. With the patch applied, I can still see "New Tab" flickering in new windows, but rarely the first 2 or 3 windows. My guess is that there's a race between showing the window, and creating the content tab: if we need to create a new content process, we don't seem to have time for an about:blank load, whereas if we are re-using an existing content process it seems about:blank loads in the content tab before the chrome code has sent to the content process the url to load. ::: browser/base/content/tabbrowser.xml:1574 (Diff revision 8) > let brandBundle = document.getElementById("bundle_brand"); > let brandShortName = brandBundle.getString("brandShortName"); > title = gNavigatorBundle.getFormattedString("customizeMode.tabTitle", > [ brandShortName ]); > + } else if (title) { > isContentTitle = true; Question from comment 46: Is there a reason why the patch makes isContentTitle false for the customizemmode case? It was true before the patch. ::: browser/base/content/tabbrowser.xml:2555 (Diff revision 8) > > var uriIsAboutBlank = aURI == "about:blank"; > > if (!aNoInitialLabel) { > - if (isBlankPageURL(aURI)) { > + if (["about:blank", "about:newtab"].includes(aURI)) { > t.setAttribute("label", this.mStringBundle.getString("tabs.emptyTabTitle")); Can this just call setAboutBlankTabTitle?

> My guess is that there's a race between showing the window, and creating the content tab You may be right. With today's clobbered debug build, I can reproduce it again on new window open. It seems to me that generally there two classes of title changes: 1) It may or may not go through and intermittent "about:blank" 2) It may go through either: 2.a) http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/base/content/tabbrowser.xml#5470 2.b) http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/base/content/tabbrowser.xml#5949 the "about:preferences" case goes through (2.b) and mossop says that it's probably because it's in the chrome context. Both of them (2.a) and (2.b) can optionally go through the intermittent "about:blank". Fixing the 2.b is imho less important, but to fix 2.a I still need to get a hold on what is the destination URL, so that I can differentiate between the about:blank being a destination vs. about:blank being an intermittent. The idea from comment 54 (using browser-child.js to get content.document.location.href) doesn't help since this href is "about:blank" for the intermittent and then "about:home" for the final url. I believe that the only way forward is to be able to know, in the DOMTitleChanged triggered by the intermittent "about:blank" that it's not the final URL and we should not update the title based on it's title (empty) or URL. I'm also getting concerned about getting it in into 57. I would love to but we have ~1 week left. I'll try to NI people who might be able to help us get the hold on the final URL in that context.

Mike may know of how we can get it!

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review179966 > Is there a reason why the patch makes isContentTitle false for the customizemmode case? It was true before the patch. Because customizemode is not a content set title, it's the browser set title. The only use of `isContentTitle` is to sanitize the output if it comes from the user. I thought it's a bug. Do you want me to bring it back? > Do we know in which case createExposableURI will throw? Is there a reason for using .spec here instead of .displaySpec before the patch? copy&paste mistake, fixed.

Eureka! With a lot of help from :mconley and :gijs, I was able to finally understand where the about:blank comes from. First of all, we were not setting `nodefaultsrc` in the initialBrowser, so that triggered about:blanks. But also, we were not filtering out on `this.mBlank` in OnLocationChange which triggered full location change and in result settitle. From my testing this patch handles everything - new windows, new windows with about:blank, restoring tabs, switching to about:preferences, and at no point does it flickers intermittent about:blank! :)

There seems to be a nice win on tpaint, sessionrestore and maybe even a super small on ts_paint! https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=875f424dcdf6&newProject=try&newRevision=4a24fc41e20fbd6a3d745d0e4df4103b5da16629&framework=1&showOnlyImportant=0 On the other hand, I need to fix a few tests that assume that a new browser window will end up with load even when no url is passed.

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review181758 Looks good to me (although I have to admit I've looked at variations of this patch enough times that I'm not seeing much anymore when staring at this code :-/), and I couldn't find any issue while testing the patch locally on Mac. And I'm glad Talos is happy about it too :-). Thanks! ::: browser/base/content/tabbrowser.xml:893 (Diff revision 10) > if (findBar.findMode != findBar.FIND_NORMAL) { > findBar.close(); > } > } > > + if (!aLocation.schemeIs("about")) { I think we need a comment here, to avoid puzzling the next reader of the code. Something like: "This sets the tab title to the url we are loading; we can skip this temporary title for about pages as they are expected to load quickly." But do we even need this check here? You have: } else if (currentURI.schemeIs("about")) { return false; in the setTabTitle implementation. ::: browser/base/content/tabbrowser.xml:6191 (Diff revision 10) > let { restoreTabsButton } = this; > restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs")); > > var tab = this.firstChild; > - tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"); > + // If the first tab of the new window is going to be > + // about:blank or about:newtab, set its title immediately to The common cases here will be about:home (showing "Nightly start page" if activity stream isn't enabled there, or "New Tab"), and about:privatebrowsing (showing "Private Browsing"). Would be nice if these could be optimized too. That's totally material for a follow-up though :-).

Warning: the patch in bug 1394188 touches the same code.

While the patch works well in the product, changing the tests for `nodefaultsrc` on the initialBrowser is going to be a PITA. I separated it out into bug 1397365. I'll try to work with :mconley to see the timeline since I'd like to try to get both patches into 57, and strictly speaking, this one doesn't depend on the previous, but it doesn't completely work without it.

:florian: So, since the `nodefaultsrc` breaks quite a number of tests and will probably take some time to fix, I thought about a way to get this landed ASAP to make sure we get it in 57 irrelevant of the `nodefaultsrc` solution. One thing I came up with is a temporary attribute on the <browser> element that I called "initialLoad". This attribute starts with a value "0" and when the first OnLocationChange is triggered for the intermittent about:blank it changes the status to "1". When the second OnLocationChange is loaded, it removed the "initialLoad". This emulates pretty well the intended behavior we'll receive once bug 1397365 is fixed and doesn't require any updates to tests which is a major plus. I originally tried to set `src="about:blank?mozinitial=true"` on the <browser> but it broke a lot of tests since a lot of code just checks for `url === "about:blank"`. I don't fully like this dirty hack, and I hope we'll get the nodefaultsrc in soon, but I'd like to land this ASAP to get some testing before 57. I believe the majority of the perf gain is coming from `nodefaultsrc` so without it, we're not affecting talos, but at least we're removing the flicker (which is very noticable on reference hardware). Does this plan work for you?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #71) > Does this plan work for you? When testing locally this newer version of the patch, the "New Tab" flicker is back when opening new windows with about:home or about:privatebrowsing in the first tab :-(.

Ugh... It's frustrating. I would really prefer to fix it via bug 1397365. Not only it is the clean way, but also gives us a performance win. Unfortunately, I'm not sure if it'll happen in time for 57 and I'd really like to get this bug fixed in time. I spent today trying to make progress in bug 1397365 and I'll spend tomorrow working on a dirty hack here. I assume we're good if we can lang a patch before the next Friday.

All things considered, I think this bug is still a minor glitch; not a big deal if this misses 57, imho. However, splitting your time on both bugs and neither making it because of that would be most unfortunate. I think we should focus all efforts on bug 1397365 so that this has the best chances to make it into 57.

I have no idea why I didn't think about it before, but now it seems pretty obvious. Instead of forcefully migrating all the tests to not depend on the inherit about:blank, we can just move the initialBrowser to be created programmatically and keep the old behavior for all cases except of when we're not loading about:blank when opening a new window. This made the patch much simpler. I only had to port one of the patches from bug 1397365 that already had r=kmag to make extensions lose nodefaultsrc when rooted. Waiting for a new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f36af4329befe7a178d092de0052bc88643839 but the previous one looked pretty green. I'll be happy to work on salvaging the pieces of bug 1397365 where we improved tests to make them less racy, but that's lower priority than this I believe.

Comment on attachment 8908519 [details] Bug 1362774 - Reduce flickering of the tab title when opening tabs and loading pages. The second part of the patch with r?florian is exactly the same as previously. I realize that we unified about:newtab and about:home and may want to add about:home to fast-path for displaying New Tab, but I didn't want to touch this patch to make the review easier. Adding it will be trivial if we want to.

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review185346 ::: browser/base/content/tabbrowser.xml:1858 (Diff revision 14) > > <method name="updateBrowserRemoteness"> > <parameter name="aBrowser"/> > <parameter name="aShouldBeRemote"/> > <parameter name="aOptions"/> > + <parameter name="aKeepNoDefaultSrcInInitialBrowser"/> It looks like it would be better to include this in the aOptions parameter. ::: browser/base/content/tabbrowser.xml:2168 (Diff revision 14) > b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu")); > b.setAttribute("tooltip", this.getAttribute("contenttooltip")); > > + if (aParams.initial) { > + b.setAttribute("anonid", "initialBrowser"); > + b.setAttribute("primary", "true"); Could these 2 attributes be set in the constructor instead, so that _createBrowser behaves the same for all browsers? ::: browser/base/content/tabbrowser.xml:2241 (Diff revision 14) > var stack = document.createElementNS(NS_XUL, "stack"); > stack.className = "browserStack"; > stack.appendChild(b); > stack.setAttribute("flex", "1"); > + if (aParams.initial) { > + stack.setAttribute("anonid", "browserStack"); I don't see any code using the "browserStack" anonid. Is this actually used? (I suspect it being currently set only on the initial browser is an indicator that it's no longer used) ::: browser/base/content/tabbrowser.xml:5784 (Diff revision 14) > > <constructor> > <![CDATA[ > - this.mCurrentBrowser = document.getAnonymousElementByAttribute(this, "anonid", "initialBrowser"); > - this.mCurrentBrowser.permanentKey = {}; > + let urlIsAboutBlank = false; > + if (window.arguments && > + typeof window.arguments[0] == "string" && nit: I would put window.arguments[0] in a variable instead of duplicating it 3 times.

Here's a new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ab6bb1722da4acd026cbb9608a6252d1cef9e9 In the previous I reversed the true/false for uriIsAboutBlank

Comment on attachment 8898190 [details] Bug 1362774 - Use nodefaultsrc on the initialBrowser in tabbrowser when possible. https://reviewboard.mozilla.org/r/169558/#review185402 ::: browser/base/content/tabbrowser.xml:24 (Diff revision 17) > <content> > <xul:stringbundle anonid="tbstringbundle" src="chrome://browser/locale/tabbrowser.properties"/> > <xul:tabbox anonid="tabbox" class="tabbrowser-tabbox" > flex="1" eventnode="document" xbl:inherits="handleCtrlPageUpDown,tabcontainer" > onselect="if (event.target.localName == 'tabpanels') this.parentNode.updateCurrentBrowser();"> > <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer"> drive-by nit: change this from <xul:tabpanels></xul:tabpanels> to <xul:tabpanels/> ::: browser/base/content/tabbrowser.xml:1954 (Diff revision 17) > // loader is created the opener is set correctly. > aBrowser.presetOpenerWindow(aOptions.opener); > } > > + if (!aOptions.keepNoDefaultSrcInInitialBrowser && > + aBrowser.getAttribute("anonid") === "initialBrowser") { nit: == is fine here, we only do the strict comparison when there's a specific reason ::: browser/base/content/tabbrowser.xml:5775 (Diff revision 17) > > <constructor> > <![CDATA[ > - this.mCurrentBrowser = document.getAnonymousElementByAttribute(this, "anonid", "initialBrowser"); > - this.mCurrentBrowser.permanentKey = {}; > + let uriIsAboutBlank = true; > + if (window.arguments) { > + if (["about:blank", ""].includes(window.arguments[0])) { usual caveat: window.arguments[0] is bogus when restoring a session

Thanks :dao! I updated the patch also adding one more constrain to our nodefaultsrc for initialBrowser to fix accessibility tests that were orange on try. Basically, accessibility test framework opened windows with firstURL `null`. I documented the reason for the changes a bit more. > usual caveat: window.arguments[0] is bogus when restoring a session Since `SessionStartup.willOverrideHomepagePromise` is a Promise, I don't know how to handle that. I believe that async injection of the initialBrowser in the constructor would require a lot of rework. It seems to me from manual testing and session restore tests working that the behavior with the patch is not breaking sessionrestore. :florian - I believe the rest of oranges on try are as expected. Let me know if you need anything else from me or another try to prove that a11y is fixed.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #94) > :florian - I believe the rest of oranges on try are as expected. Let me know > if you need anything else from me or another try to prove that a11y is fixed. I think your latest try push is https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf63d3e0d8a6&selectedJob=131332682 The "TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_sibling-off-domain.html | Test timed out." failures look like they are directly caused by the patches here, and interestingly the JS errors in the test log are the same as what I was seeing locally when keeping Cmd+N pressed for a little while: JavaScript error: chrome://browser/content/tabbrowser.xml, line 2414: TypeError: invalid 'in' operand browser JavaScript error: chrome://browser/content/tabbrowser.xml, line 5828: TypeError: this.mCurrentBrowser._createAutoScrollPopup is not a function JavaScript error: chrome://browser/content/tabbrowser.xml, line 1915: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener] On one hand these failures are very scary (broken browser windows that will never finish loading). On the other hand, seeing that try catches the problem I was seeing locally makes me more confident that we'll likely have something that works once we can get green test runs.

Good catch. It seems that if I comment out the `this._autoScrollPopup` section in the constructor there's one more line that races `this.webProgress.addProgressListener - this.webProgress is undefined`. When I guard those two around `if()` the test passes. :florian, do you have a recommendation on how to solve this?

So, the createAutoScrollPopup is the less important half of the puzzle. I can comment it out and things flow. The more interesting one is the `invalid 'in' operand browser`. I got to this stack: GECKO(20295) | _insertBrowser@chrome://browser/content/tabbrowser.xml:2414:23 GECKO(20295) | getRelatedElement@chrome://browser/content/tabbrowser.xml:7082:11 GECKO(20295) | set_selectedIndex@chrome://global/content/bindings/tabbox.xml:406:31 GECKO(20295) | tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:275:13 GECKO(20295) | @chrome://browser/content/tabbrowser.xml:34:9 GECKO(20295) | tabbrowser_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5819:11 GECKO(20295) | get@chrome://browser/content/browser.js:248:21 GECKO(20295) | getOpenTabsAndWinsCounts@resource:///modules/BrowserUsageTelemetry.jsm:100:5 GECKO(20295) | onLoad@resource:///modules/BrowserUsageTelemetry.jsm:638:22 GECKO(20295) | EventListener.handleEvent*_onWindowOpen@resource:///modules/BrowserUsageTelemetry.jsm:646:5 GECKO(20295) | observe@resource:///modules/BrowserUsageTelemetry.jsm:349:9 Basically, when tabbrowser.xml constructor is calling `this.mCurrentTab = this.tabContainer.firstChild;` in line 5814 it triggers tabbox.xml constructor which sets selectedIndex to 0. The setter calls `let linkedPanel = this.getRelatedElement(tab);` which calls `_insertBrowser` which expects the `aTab.linkedBrowser` to be already there. Unfortunately, `this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;` is called 18 lines below in line 5832. It is intermittent because the whole flow is caused by telemetry. The telemetry trigger is `ondomwindowopened`. Now, first of all, I'm confused how this patch exposes the raciness. The same code existed before at least in this part of the stack: GECKO(20295) | _insertBrowser@chrome://browser/content/tabbrowser.xml:2414:23 GECKO(20295) | getRelatedElement@chrome://browser/content/tabbrowser.xml:7082:11 GECKO(20295) | set_selectedIndex@chrome://global/content/bindings/tabbox.xml:406:31 GECKO(20295) | tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:275:13 GECKO(20295) | @chrome://browser/content/tabbrowser.xml:34:9 GECKO(20295) | tabbrowser_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5819:11 so, even if the tabbrowser_XBL_Constructor was triggered by something else, it should have cause the same problem. Or maybe tabbox constructor was called before tabbrowser.xml constructor? I don't know. I think the problem is isolated, and fairly reproducible, and worth fixing both to land this and to remove the race condition, but I'm not sure how should it work because in most naive term, it seems that sth like this is happening: ``` 1) the tabbrowser constructor is triggering tabbox constructor to get the selectedTab 2) tabbox constructor is setting selectedIndex 3) this calls for the first tab 4) which wants to _insertBrowser 5) which wants the first tab to already have linkedBrowser 6) and only when it gets the tab it sets tab.linkedBrowser = currentBrowser ``` this should always fail so my guess is that there's some other place which assigns `linkedBrowser` for the first tab before the constructor in tabbrowser.xml was called, so that the constructor of tabbox.xml, which has to be done by or before `tabContainer.firstTab` call in tabbrowser.xml's constructor, can selectIndex and get the linkedBrowser for the first tab.

I think there are three issues: 1) the createAutoScrollPopup is called sometimes too early in tabbrowser.xml constructor 2) the error described in comment 97 3) the this.webProgress (and this.docShell) being undefined in tabbrowser constructor sometimes I can sole (1) by commenting out the block, I can solve (2) by switching to ``` this.mCurrentTab = document.querySelector('.tabbrowser-tab'); ``` in the constructor instead of `tabContainer.firstChild`. I'm not sure how to fix the third and if I guard it in `if(this.webProgress)` I'm getting an error in `updateBrowserRemoteness` where it tries to remove the listener from webProgress.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #96) > Good catch. > > It seems that if I comment out the `this._autoScrollPopup` section in the > constructor there's one more line that races > `this.webProgress.addProgressListener - this.webProgress is undefined`. > > When I guard those two around `if()` the test passes. > > :florian, do you have a recommendation on how to solve this? I'm replying a bit late, but if I had to investigate I would start with trying to figure out the `invalid 'in' operand browser` error. This error already occurred without your patch... and I suspect it may have the same root cause as the failures we care more directly about here.

Kevin, Mike pointed you out as a person who knows more than a mere mortal should ever have to be exposed to about tabbrowser creation time. I'm moving the initialBrowser to be constructed in the constructor here, and it seems to expose three race conditions. The autoscroll one is not interesting and we know how to fix it. But the one I described in comment 97 is more complex and I'm looking for someone to help me decide where should I break the loop in order to bring sanity back to the construction flow. Can you help here?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #100) > Kevin, > > Mike pointed you out as a person who knows more than a mere mortal should > ever have to be exposed to about tabbrowser creation time. > > I'm moving the initialBrowser to be constructed in the constructor here, and > it seems to expose three race conditions. The autoscroll one is not > interesting and we know how to fix it. > > But the one I described in comment 97 is more complex and I'm looking for > someone to help me decide where should I break the loop in order to bring > sanity back to the construction flow. > > Can you help here? Sorry that I'm not familiar with this bug, but which constructor? Do you mean the xul:browser constructor?

(In reply to Kevin Jones from comment #101) > Sorry that I'm not familiar with this bug, but which constructor? Do you > mean the xul:browser constructor? Oh, I'm sorry for the noise, I see now you are talking about tabbrowser constructor.

Yeah :) It's the tabbrowser.xml constructor. The patch is really small: https://bugzilla.mozilla.org/attachment.cgi?id=8898190 The problem is described in comment 94. Are you familiar with the tabbrowser's constructor? Can you advise on how to remove the race condition when constructing the code?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #103) > Yeah :) > It's the tabbrowser.xml constructor. The patch is really small: > https://bugzilla.mozilla.org/attachment.cgi?id=8898190 > > The problem is described in comment 94. > > Are you familiar with the tabbrowser's constructor? Can you advise on how to > remove the race condition when constructing the code? Zibi, as there is quite a bit of discussion here to wade through, can you give me a STR, and exactly what failure(s) I am looking for? It sounded like in one of your comments that this was reproduce-able.

Sure! 1) Get m-c clone 2) Apply the first patch here: https://bugzilla.mozilla.org/attachment.cgi?id=8898190 3) ./mach mochitest docshell/test/navigation/test_sibling-off-domain.html You may have to launch it a couple times, but very soon you should be able to crash it. As described in comment 98 there are three problems that come from the raciness of the code. The autoscroll issue is the least important as :florian wants to remove it all together, so you can just comment it out. Now, the remaining two are the culprit. The first one is visible as "JavaScript error: chrome://browser/content/tabbrowser.xml, line 2414: TypeError: invalid 'in' operand browser" and the stack trace and some intro explanation of the race condition is in comment 94. The other is basically that at the time <constructor> is fired, the `this.docShell` is undefined, which causes `this.webProgress` to be undefined as well. I'd appreciate help with those two issues.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #105) > Sure! > > 1) Get m-c clone > 2) Apply the first patch here: > https://bugzilla.mozilla.org/attachment.cgi?id=8898190 > 3) ./mach mochitest docshell/test/navigation/test_sibling-off-domain.html > > You may have to launch it a couple times, but very soon you should be able > to crash it. > > As described in comment 98 there are three problems that come from the > raciness of the code. The autoscroll issue is the least important as > :florian wants to remove it all together, so you can just comment it out. > > Now, the remaining two are the culprit. The first one is visible as > "JavaScript error: chrome://browser/content/tabbrowser.xml, line 2414: > TypeError: invalid 'in' operand browser" > > and the stack trace and some intro explanation of the race condition is in > comment 94. > > The other is basically that at the time <constructor> is fired, the > `this.docShell` is undefined, which causes `this.webProgress` to be > undefined as well. > > I'd appreciate help with those two issues. I'm not able to get docshell/test/navigation/test_sibling-off-domain.html to run without crashing even without the patch applied. I get the same error. It would seem that the problem (at least as far as this test is concerned) is not related to moving initialBrowser to the constructor per se.

Your patch needs rebasing BTW.

Rebased the patches. That's a good news actually! It means IIUC that in your environment you're able to reproduce it more reliably than I can (for me it is intermittent). If in the log before the crash you see the error "JavaScript error: chrome://browser/content/tabbrowser.xml, line 2414: TypeError: invalid 'in' operand browser" this is the one I'm trying to debug and this is the one I provided stack for analysis in comment 94. Do you think you can help me debug it and advise on how to break out of the condition race? I understand that its not my patch causing it, my patch is just making it more permanent in some scenarios, but the bug exists without the patch and I believe it makes it even more important to fix :)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #110) > Rebased the patches. > > That's a good news actually! It means IIUC that in your environment you're > able to reproduce it more reliably than I can (for me it is intermittent). > > If in the log before the crash you see the error > "JavaScript error: chrome://browser/content/tabbrowser.xml, line 2414: > TypeError: invalid 'in' operand browser" > > this is the one I'm trying to debug and this is the one I provided stack for > analysis in comment 94. > > Do you think you can help me debug it and advise on how to break out of the > condition race? > > I understand that its not my patch causing it, my patch is just making it > more permanent in some scenarios, but the bug exists without the patch and I > believe it makes it even more important to fix :) Here are some clues for you: I set some probes and I assigned windows and tabs unique IDs for tracking. This is what I observed: Error free run: 21:03:30 : ~/mozilla-central/mozilla-central_1 ./mach mochitest docshell/test/navigation/test_sibling-off-domain.html | grep 'Error\|error\|XXX' GECKO(16213) | XXX : window.uid : 1505963301489 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963301489 GECKO(16213) | XXX : window.uid : 1505963301489 tabbox.xml : constructor : set this.selectedIndex : tab.uid : 1505963301489 GECKO(16213) | XXX : window.uid : 1505963325735 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963325735 GECKO(16213) | XXX : window.uid : 1505963325735 tabbox.xml : constructor : set this.selectedIndex : tab.uid : 1505963325735 GECKO(16213) | XXX : window.uid : 1505963325887 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963325887 GECKO(16213) | XXX : window.uid : 1505963325887 tabbox.xml : constructor : set this.selectedIndex : tab.uid : 1505963325887 GECKO(16213) | XXX : window.uid : 1505963301489 tabbrowser.xml : addTab : tab.uid : 1505963325985 GECKO(16213) | XXX : window.uid : 1505963301489 tabbrowser.xml : _insertBrowser : browser : [object XULElement] linkedPanel : aTab.uid : 1505963325985 GECKO(16213) | XXX : window.uid : 1505963301489 tabbrowser.xml : addTab : tab.uid : 1505963326318 GECKO(16213) | XXX : window.uid : 1505963301489 tabbrowser.xml : _insertBrowser : browser : [object XULElement] linkedPanel : aTab.uid : 1505963326318 Run that throws error: 21:09:13 : ~/mozilla-central/mozilla-central_1 ./mach mochitest docshell/test/navigation/test_sibling-off-domain.html | grep 'Error\|error\|XXX' GECKO(16279) | XXX : window.uid : 1505963372543 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963372543 GECKO(16279) | XXX : window.uid : 1505963372543 tabbox.xml : constructor : set this.selectedIndex : tab.uid : 1505963372543 GECKO(16279) | XXX : window.uid : 1505963372543 tabbrowser.xml : addTab : tab.uid : 1505963380817 GECKO(16279) | XXX : window.uid : 1505963372543 tabbrowser.xml : _insertBrowser : browser : [object XULElement] linkedPanel : aTab.uid : 1505963380817 GECKO(16279) | XXX : window.uid : 1505963380961 tabbox.xml : constructor : set this.selectedIndex : tab.uid : undefined GECKO(16279) | XXX : window.uid : 1505963380961 tabbrowser.xml : _insertBrowser : browser : undefined linkedPanel : aTab.uid : undefined GECKO(16279) | XXX : window.uid : 1505963380961 ERROR : tabbrowser.xml : _insertBrowser : TypeError: invalid 'in' operand browser GECKO(16279) | XXX : stack : _insertBrowser@chrome://browser/content/tabbrowser.xml:2397:1 GECKO(16279) | XXX : stack : getRelatedElement@chrome://browser/content/tabbrowser.xml:6969:11 GECKO(16279) | XXX : stack : set_selectedIndex@chrome://global/content/bindings/tabbox.xml:408:31 GECKO(16279) | XXX : stack : tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:277:13 GECKO(16279) | XXX : stack : @chrome://browser/content/tabbrowser.xml:46:9 GECKO(16279) | XXX : stack : tabbrowser_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5726:11 GECKO(16279) | XXX : stack : get@chrome://browser/content/browser.js:248:21 GECKO(16279) | XXX : stack : getOpenTabsAndWinsCounts@resource:///modules/BrowserUsageTelemetry.jsm:100:5 GECKO(16279) | XXX : stack : _onTabOpen@resource:///modules/BrowserUsageTelemetry.jsm:615:28 GECKO(16279) | XXX : stack : handleEvent@resource:///modules/BrowserUsageTelemetry.jsm:365:9 GECKO(16279) | XXX : stack : addTab@chrome://browser/content/tabbrowser.xml:2713:13 GECKO(16279) | XXX : stack : loadOneTab@chrome://browser/content/tabbrowser.xml:1713:23 GECKO(16279) | XXX : stack : _openURIInNewTab@chrome://browser/content/browser.js:5221:15 GECKO(16279) | XXX : stack : browser_openURIInFrame@chrome://browser/content/browser.js:5362:12 GECKO(16279) | XXX : stack : GECKO(16279) | XXX : window.uid : 1505963380961 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963380974 GECKO(16279) | XXX : window.uid : 1505963372543 tabbrowser.xml : addTab : tab.uid : 1505963381120 GECKO(16279) | XXX : window.uid : 1505963372543 tabbrowser.xml : _insertBrowser : browser : [object XULElement] linkedPanel : aTab.uid : 1505963381120 GECKO(16279) | XXX : window.uid : 1505963381711 tabbrowser.xml : constructor : mCurrentTab.uid : 1505963381711 GECKO(16279) | XXX : window.uid : 1505963381711 tabbox.xml : constructor : set this.selectedIndex : tab.uid : 1505963381711 Observe the following: In the error-free run, tabbrowser.xml constructor always runs prior to tabbox.xml constructor for each and every window. In the error run, for window id=1505963380961, tabbox.xml constructor runs prior to tabbrowser.xml constructor. Yet I also observed something else interesting. Code for tabbrowser.xml for window id=1505963380961 is available and bound to the tabbox (at least `getRelatedElement` and `_insertBrowser`), even though the constructor has not yet run. Thus `getRelatedElement` and `_insertBrowser` gets run on a tab which has not had the browser linked to it yet (which would happen in the constructor). (Observe also that aTab.uid in the `_insertBrowser` call is `undefined`, which would have been assigned had the tabbrowser constructor run.) It would seem to me that the raciness in this case is an XBL issue and how XBL elements get bound and constructors run. `tabbrowser` is the higher-order binding, yet in some cases, the constructor for `tabbox` - the lower order binding - gets run first. Yet the higher-order methods bound on the element are available. Another observation I made is that: In all cases where I observed everything ran in the expected order (and no error), `addTab` never ran on the first window until after tabbrowser and tabbox constructors had run for the subsequent windows. In all cases where I observed everything did not run in the expected order (and had error(s)), `addTab` ran on the first window before bindings were set for the subsequent windows. I am assuming that the windows were opened asynchronously, and thus the sequence of `addTab` running is not predictable. It may be possible that the `addTab` call(s), if run before/while the subsequent windows are setting their tabbox/tabbrowser bindings, somehow trigger a glitch in the XBL binding process for those windows.

Thank you! That's super helpful. I think I'll spin a new bug (111 comments here!) to just deal with the race condition in tabbox/tabbrowser constructors and block this bug on it.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #112) > Thank you! That's super helpful. > > I think I'll spin a new bug (111 comments here!) to just deal with the race > condition in tabbox/tabbrowser constructors and block this bug on it. You're welcome... Personally I don't technically see the issue as blocking this bug, unless you somehow see the changes proposed by this bug as exacerbating the issue. But I was very consistently able to reproduce the issue here without your patch. It failed quite a bit more often than not.

> Personally I don't technically see the issue as blocking this bug, unless you somehow see the changes proposed by this bug as exacerbating the issue. But I was very consistently able to reproduce the issue here without your patch. It failed quite a bit more often than not. Same here on my machine. Unfortunately, it seems that it exposes the race quite a bit on try, which is why :florian rised it as a blocker.

Hey gandalf, are you still working on this?

Per discussion with gandalf on IRC, I'm going to try to drive this over the line.

I've (manually; with the de-XBL and property renames and moving the markup of this stuff + the status panel into browser.xul, too much else changed) rebased and updated the first patch, and from a naive check it seems to work. Let's see if try agrees... https://treeherder.mozilla.org/#/jobs?repo=try&revision=86354f7f983b1eeb38b9f6b18610a11d650b246c

Try looks reasonable, some orange in browser/base/content/test/general/browser_newWindowDrop.js that I have a local fix for, but also some orange in browser/components/resistfingerprinting/test/browser/browser_roundedWindow_newWindow.js (seems to be win10-only?) that I'm a bit stumped on. I wrote the patch on a win10 machine myself and can't reproduce. What I *can* reproduce is that all the other fingerprint resistance window sizing tests fail with off-by-one errors because apparently it doesn't like machines with non-integer DPI factors (this one has 1.5 or 1.75 for the screen, I think). Probably fodder for a separate bug. :-\ Arthur/Tim, any idea where I'd start with that test? Why would the fingerprint resistance code for inner window sizes care whether or not there's an about:blank window as part of creating the browser? Not being able to reproduce the orange means I'm a bit lost for a point to debug at. I'll note that the test ( https://searchfox.org/mozilla-central/source/browser/components/resistfingerprinting/test/browser/browser_roundedWindow_newWindow.js ) seems a bit suspicious in that it creates a new window and then opens a new dummy tab in that window and then measures in there (rather than just measuring in about:blank). I don't know why the test was written that way, but it seems odd. If you have ideas about that, let me know.

Semi-random hunch attempt to fix this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cf78ed5c0e28997475bf1dc0bdd531f10cbac21 Using the same code that we normally use to open a new browser window, instead of the custom BTU code (not sure why that's written the way it is) and no longer opening a tab, because we shouldn't really need to... would still be good to understand better why this test is failing, even if this workaround turns out to fix the test.

(In reply to :Gijs (he/him) from comment #119) > Semi-random hunch attempt to fix this: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2cf78ed5c0e28997475bf1dc0bdd531f10cbac21 gBrowser.linkedBrowser should be gBrowser.selectedBrowser.

(In reply to Dão Gottwald [::dao] from comment #120) > (In reply to :Gijs (he/him) from comment #119) > > Semi-random hunch attempt to fix this: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=2cf78ed5c0e28997475bf1dc0bdd531f10cbac21 > > gBrowser.linkedBrowser should be gBrowser.selectedBrowser. Yeah, I forgot to `hg amend` before pushing to try - so that change is currently in the commit with the try msg, but it's included.

(In reply to :Gijs (he/him) from comment #119) > Semi-random hunch attempt to fix this: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2cf78ed5c0e28997475bf1dc0bdd531f10cbac21 > > Using the same code that we normally use to open a new browser window, > instead of the custom BTU code (not sure why that's written the way it is) > and no longer opening a tab, because we shouldn't really need to... would > still be good to understand better why this test is failing, even if this > workaround turns out to fix the test. Perhaps unsurprisingly, the test is still orange: 730 INFO TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/browser/browser_roundedWindow_newWindow.js | The screen.height has a correct rounded value - 907 == 900 - If people have ideas, that would be very helpful...

When fingerprinting resistance is enabled. It will measure the chrome UI size first and then decide the window size according to the chrome UI size [1]. And it will happen when chrome loaded [2]. So, if there are any chrome UI changes after chrome loaded might cause this. And I've noticed that it appends and removes things in _setupInitialBrowserAndTab() in your patch. Maybe this causes this problem. But, I still dunno why this only happens on Windows 10. Perhaps, it is a timing issue that _setupInitialBrowserAndTab() calls after the chrome loaded on Windows 10 but others are not. In addition, I think you are right about the test, we can measure the result in 'about:blank'. I was writing this by following the typical way of opening a tab and do some checks in it. I didn't notice that we can measure in 'about:blank' directly. Thanks for pointing this out. [1] https://searchfox.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#2468 [2] https://searchfox.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#1117

(In reply to :Gijs (he/him) from comment #118) > What I *can* reproduce is that all the other fingerprint resistance window > sizing tests fail with off-by-one errors because apparently it doesn't like > machines with non-integer DPI factors (this one has 1.5 or 1.75 for the > screen, I think). Probably fodder for a separate bug. :-\ Is this reproducible without your patch as well? If so, we should definitely open a ticket. Thanks!

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #124) > (In reply to :Gijs (he/him) from comment #118) > > > What I *can* reproduce is that all the other fingerprint resistance window > > sizing tests fail with off-by-one errors because apparently it doesn't like > > machines with non-integer DPI factors (this one has 1.5 or 1.75 for the > > screen, I think). Probably fodder for a separate bug. :-\ > > Is this reproducible without your patch as well? If so, we should definitely > open a ticket. Thanks! I think so, I'll check and get some more details and file something, probably tomorrow or Monday. (In reply to Tim Huang[:timhuang] from comment #123) > When fingerprinting resistance is enabled. It will measure the chrome UI > size first and then decide the window size according to the chrome UI size > [1]. And it will happen when chrome loaded [2]. So, if there are any chrome > UI changes after chrome loaded might cause this. And I've noticed that it > appends and removes things in _setupInitialBrowserAndTab() in your patch. > Maybe this causes this problem. But, I still dunno why this only happens on > Windows 10. Perhaps, it is a timing issue that _setupInitialBrowserAndTab() > calls after the chrome loaded on Windows 10 but others are not. > > In addition, I think you are right about the test, we can measure the result > in 'about:blank'. I was writing this by following the typical way of opening > a tab and do some checks in it. I didn't notice that we can measure in > 'about:blank' directly. Thanks for pointing this out. > > [1] > https://searchfox.org/mozilla-central/source/xpfe/appshell/nsXULWindow. > cpp#2468 > [2] > https://searchfox.org/mozilla-central/source/xpfe/appshell/nsXULWindow. > cpp#1117 Thanks for the pointers. I'll try to add some debug logging and push to try, I guess, both with and without the patch? Some avenues I think I probably need to look at: - SizeShell is called twice - once from OnChromeLoaded() and once from BeforeStartLayout(). I don't know why, or if both/either change timings wrt layout as a result of the patch. As far as I can tell from looking at it briefly, SizeShell and the stuff it calls doesn't do a layout flush before measuring so even if the content is there we might not have run layout on it yet. - worse, we also have SizeShellTo and SizeShellToWithLimit (which each have their own callers), and also an IPC protocol to size the child (I think? RemoteSizeShell?) and the parent (TabParent::RecvSizeShellTo; presumably for when content calls DOM resizing APIs?). Because we're measuring in the child, without being able to debug I don't even know if the data is already wrong in the parent or just in the child. - SizeShell checks HasPrimaryContent, which checks for a primary content frame (non-e10s) or primary tabparent (e10s). I guess it's possible that there's a race in constructing those as a result of the patch, so we might then not take the SizeShell branch. - Maybe we could sidestep some of this if I left more of the containing markup (stack etc.) hardcoded in the browser.xul document. But that might mean more hoop-jumping when inserting the initial browser, plus it feels hacky. This feels pretty murky. Am I misreading things and is it simpler than I think, or is someone perhaps working on clarifying "ownership" of the initial window size and making this more straightforward? Why do we call SizeShell() twice (before even getting to things like session restore which might resize the window again anyway) ?

OK, I finally figured out that I can reproduce the infra issue here if I set my own screen resolution to 1280x1024, which is what infra uses (though somehow the test picks 807 instead of 907 like on infra... but nevermind). Here's some log output pre-patch: 0:04.42 INFO Entering test bound test_new_window 0:04.50 GECKO(4104) Calling SizeShell before starting layout 0:04.50 GECKO(4104) SizeShell: from xul was false 0:04.50 GECKO(4104) SizeShell: got primary content: true 0:04.50 GECKO(4104) SizeShell: forcing rounded dimensions 0:04.51 GECKO(4104) SizeShell: would have overridden positionSet but didn't because macos or windows 0:04.51 GECKO(4104) SizeShell: loading pos from XUL with specified width (-1) and height (-1) 0:04.51 GECKO(4104) Intrinsically sized: no 0:04.51 GECKO(4104) SizeShell: loading position from XUL *again* with specified width (-1) and height (-1) 0:04.56 GECKO(4104) Setting up initial browser and tab 0:04.60 GECKO(4104) Calling SizeShell now that chrome has loaded 0:04.60 GECKO(4104) SizeShell: from xul was false 0:04.60 GECKO(4104) SizeShell: got primary content: true 0:04.60 GECKO(4104) SizeShell: forcing rounded dimensions 0:04.61 GECKO(4104) SizeShell: would have overridden positionSet but didn't because macos or windows 0:04.61 GECKO(4104) SizeShell: loading pos from XUL with specified width (-1) and height (-1) 0:04.61 GECKO(4104) Intrinsically sized: no 0:04.61 GECKO(4104) SizeShell: loading position from XUL *again* with specified width (-1) and height (-1) 0:04.65 PASS The screen.width has a correct rounded value - 1000 == 1000 - 0:04.65 PASS The screen.height has a correct rounded value - 900 == 900 - 0:04.65 PASS The window.innerWidth has a correct rounded value - 1000 == 1000 - 0:04.65 PASS The window.innerHeight has a correct rounded value - 900 == 900 - 0:04.68 INFO Leaving test bound test_new_window Here's some approximate output post-patch: 0:04.24 INFO Entering test bound test_new_window 0:04.32 GECKO(2124) Calling SizeShell before starting layout 0:04.32 GECKO(2124) SizeShell: from xul was false 0:04.32 GECKO(2124) SizeShell: got primary content: false 0:04.32 GECKO(2124) SizeShell: NOT forcing dimensions 0:04.32 GECKO(2124) SizeShell: would have overridden positionSet but didn't because macos or windows 0:04.32 GECKO(2124) SizeShell: loading pos from XUL with specified width (1168) and height (924) 0:04.32 GECKO(2124) SizeShell: Setting width (1168) and height (924) 0:04.32 GECKO(2124) Intrinsically sized: no 0:04.32 GECKO(2124) SizeShell: loading position from XUL *again* with specified width (1168) and height (924) 0:04.36 GECKO(2124) Setting up initial browser and tab 0:04.42 GECKO(2124) Calling SizeShell now that chrome has loaded 0:04.42 GECKO(2124) SizeShell: from xul was false 0:04.42 GECKO(2124) SizeShell: got primary content: true 0:04.42 GECKO(2124) SizeShell: forcing rounded dimensions 0:04.42 GECKO(2124) Intrinsically sized: no 0:04.42 GECKO(2124) SizeShell: still not setting position 0:04.48 PASS The screen.width has a correct rounded value - 1000 == 1000 - 0:04.48 FAIL The screen.height has a correct rounded value - 807 == 900 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:6 0:04.48 PASS The window.innerWidth has a correct rounded value - 1000 == 1000 - 0:04.48 FAIL The window.innerHeight has a correct rounded value - 807 == 900 - Stack trace: resource://testing-common/content-task.js line 59 > eval:null:10 0:04.50 INFO Leaving test bound test_new_window The "setting up" bit that I've picked out is from tabbrowser.js's setting up the initial browser. So it seems we don't call _setupInitialBrowserAndTab() until DOMContentLoaded, which is after we *first* call SizeShell(). Which means that pre-patch, the first call already has a primary content shell, and post-patch, it doesn't. Somehow (imagine me waving my hands) this means we end up with the wrong size, because the second call to SizeShell() is somehow not sufficient to actually resize the document. I could move the insertion of the initial browser to onBeforeInitialXULLayout, but that feels wrong... I would prefer that we fix the second call to SizeShell() to be more effective. Any ideas why it doesn't help, :timhuang? I'd be happy to chat on IRC and/or do more debugging, just not sure what to look for...

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #124) > (In reply to :Gijs (he/him) from comment #118) > > What I *can* reproduce is that all the other fingerprint resistance window > > sizing tests fail with off-by-one errors because apparently it doesn't like > > machines with non-integer DPI factors (this one has 1.5 or 1.75 for the > > screen, I think). Probably fodder for a separate bug. :-\ > > Is this reproducible without your patch as well? If so, we should definitely > open a ticket. Thanks! Filed bug 1475973.

I talked to Tim on IRC. Apparently ForceRoundedDimensions gets different numbers for the post-patch state after DOMContentLoaded. The content area measurement returns different numbers than pre-patch, with a difference of about 10 pixels. Then by the time that the content page actually loads, layout has changed and the "overhead" of the chrome and/or size of the content has changed as well, and we're "off" by either about 10 pixels, or even about 90 pixels because the "temporary" change in size causes us to pick a different clamped value for the window height. The fact that this is Windows 10 only seems likely to be caused by the fact that on infra, our Linux and MacOS testing machines use 1600x1200 as the resolution, and Windows machines use 1280x1024. resistfingerprinting code clamps to 1000x1000 by default, and the additional temporary overhead doesn't stop us from using that content size. It's still not clear to me what exactly is causing the change in layout that is tripping up this test. :-\

With help from dholbert + mstange, I did some frame tree dumps. Pre-patch: titlebar: 0:04.90 GECKO(13112) Box(vbox)(9)@20ad9e6dcd8 parent=20ad9e15180 next=20ad9e6e668 {0,60,76080,1980} [state=000b000080800200] [content=20ad9758300] [cs=20ad8d6ce08]< #navigator-toolbox: 0:04.91 GECKO(13112) Box(toolbox)(10)@20ad9e6e668 parent=20ad9e15180 next=20ad9ee0cb0 {0,60,76080,4440} [state=000b002080840200] [content=20ad9758880] [cs=20ad8aef508]< Post-patch: 0:04.87 GECKO(6160) Box(vbox)(9)@22a2be4fcd8 parent=22a2bdd1180 next=22a2be50668 {0,0,75960,2640} [state=000b000080800200] [content=22a2bd4f380] [cs=22a2ab79a08]< 0:04.87 GECKO(6160) Box(toolbox)(10)@22a2be50668 parent=22a2bdd1180 next=22a2beb6a80 {0,660,75960,4440} [state=000b002080840200] [content=22a2bd4f900] [cs=22a2b214b08]< So the titlebar is suddenly 2640-1980=660 css units (so 11 css pixels) taller, and the toolbox shifts for that, and so the content is lower down in comparison. This matches the numbers elsewhere in here. The difference in width (of 2 css pixels, 120 units) also matches those details. Bizarrely, this doesn't seem to be caused by the contents of the titlebar; their sizes look the same, though the content of the titlebar does also get pushed down in the post-patch state. I'll see if I can work out what's going on here tomorrow. Mike or anyone else, if you have further ideas/clues based on the patch ( https://hg.mozilla.org/try/rev/8d26ffc03cb9 ) then those are obviously welcome...

It turns out the 11px is from the -moz-appearance of -moz-window-titlebar-maximized in browser.css ( https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/themes/windows/browser.css#311 ). Commenting out that rule magically fixes the bug. The reason is that nsNativeThemeWin.cpp sets the top of items with this appearance in this code: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/widget/windows/nsNativeThemeWin.cpp#2132-2137 . This all only happens for maximized windows. Unfortunately, the change in my patch causes the window to be maximized for a short period of time, before resistFingerprinting kicks in and resizes the window to be smaller. This seems to be happening because when we run SizeShell() the first time, from https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/xpfe/appshell/nsXULWindow.cpp#2475 , post-patch we have no primary content area yet (because gBrowser.init() hasn't been called so we haven't yet set up the initial browser). Specifically, in https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/xpfe/appshell/nsXULWindow.cpp#2531-2537 if (isContent && nsContentUtils::ShouldResistFingerprinting()) { ForceRoundedDimensions(); } else if (!mIgnoreXULSize) { gotSize = LoadSizeFromXUL(specWidth, specHeight); specWidth += windowDiff.width; specHeight += windowDiff.height; } we take the first branch pre-patch, and the second one post-patch. ForceRoundedDimensions() doesn't actually do anything when called from BeforeStartLayout, because the content size it determines is 0x0, and so it mostly no-ops ( https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/xpfe/appshell/nsXULWindow.cpp#2608-2629 will do nothing because the target and current content size are both 0x0) except: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/xpfe/appshell/nsXULWindow.cpp#1114-1115 mIgnoreXULSizeMode = true; So now we're ignoring the XUL size mode, which we weren't doing before. A little later, back in SizeShell() from which we called (or didn't...) ForceRoundedDimensions() we now call LoadMiscPersistentAttributesFromXUL() ( https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/xpfe/appshell/nsXULWindow.cpp#2588 ). This has a block dealing with sizemode code, which ignores the sizemode set on the document if mIgnoreXULSizeMode is set (which it would be pre-patch, and isn't post-patch). This code is pretty confusing anyway: nsSizeMode sizeMode = nsSizeMode_Normal; if (!mIgnoreXULSizeMode && (stateString.Equals(SIZEMODE_MAXIMIZED) || stateString.Equals(SIZEMODE_FULLSCREEN))) { /* Honor request to maximize only if the window is sizable. An unsizable, unmaximizable, yet maximized window confuses Windows OS and is something of a travesty, anyway. */ if (mChromeFlags & nsIWebBrowserChrome::CHROME_WINDOW_RESIZE) { mIntrinsicallySized = false; if (stateString.Equals(SIZEMODE_MAXIMIZED)) sizeMode = nsSizeMode_Maximized; else sizeMode = nsSizeMode_Fullscreen; } } // If we are told to ignore the size mode attribute update the // document so the attribute and window are in sync. if (mIgnoreXULSizeMode) { nsAutoString sizeString; if (sizeMode == nsSizeMode_Maximized) sizeString.Assign(SIZEMODE_MAXIMIZED); else if (sizeMode == nsSizeMode_Fullscreen) sizeString.Assign(SIZEMODE_FULLSCREEN); else if (sizeMode == nsSizeMode_Normal) sizeString.Assign(SIZEMODE_NORMAL); if (!sizeString.IsEmpty()) { ErrorResult rv; windowElement->SetAttribute(MODE_ATTRIBUTE, sizeString, rv); } } ... so if mIgnoreXULSizeMode is false, we might change `sizeMode`, but we'll never enter the next block, setting the attribute. If it's true, we're guaranteed not to change `sizeMode`, but spend time checking what its value is and eventually (always) writing "normal" to the attribute, irrespective of what its value was before. Ignoring this for a moment, in the "good" pre-patch case, we arrive here and set the attribute to "normal" because mIgnoreXULSizeMode is true. In the "bad" post-patch case, we arrive here and don't enter the second block. So we don't touch the sizemode attribute. Instead, we leave it at what it was before. Before? Yeah - even though this method was called "LoadMiscPersistentAttributesFromXUL", we somehow also have "LoadPersistentWindowState". Unfortunately, the latter is also called from BeforeStartLayout() (before SizeShell()), and it sets the sizemode to maximized, because that's the persisted value. Why? Well, the browser didn't start with resistfingerprinting turned on, and we're on a machine with 1280x1024 screen resolution, so this browser.js code: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/base/content/browser.js#1191-1207 will have set the sizemode to maximized because the 0.9 * 1280 is clearly less than the TARGET_WIDTH of 1280, so line 1204-1205 will kick in and just maximize the window. Then in the test, in setup(), we specifically create another window to check what the available screen size is, which will open and then close a window, and closing the window will automatically persist the size. There are clearly too many cooks (browser JS, browser CSS, xpfe, widget code, resist fingerprinting, the OS, session restore, XUL store, nsBrowserGlue for the initial blank window, ...) in the "how do I size the window" kitchen, and I'll file a follow-up to discuss disentangling this a bit more. In the meantime, for this bug... I'm not 100% sure. Pre-patch, the only real purpose of calling ForceRoundedDimensions() is that it sets mIgnoreXULSizeMode (and mIgnoreXULSize) to true. If we did only that, irrespective of whether the window had primary content, I think that'd help. In particular, I suspect we could drop the call to ForceRoundedDimensions() when SizeShell() is called from BeforeStartLayout() and replace it with only that, which will save some spurious resizes. The only issue would be that we'd then be doing that for non-browser windows. Taking a step back again from the gory details, the root issue is we need a way for the xulwindow code to distinguish browser windows very early in the window's lifetime, because of the fingerprint resisting code which needs to know that it should ignore xul persisted size and/or sizemode and enforce a particular window size. I'm tempted to suggest that we should make the resist fingerprinting code just prevent XUL persistence from storing these values in the first place and then hopefully it won't need to mess with the xul window code anymore, but that also sounds non-trivial. Other options include checking the document URI (feels yucky, x-ref also bug 1476333 ), or leaving this whole mess as it is and just insert the initial browser sooner - but that's messy because ideally the code should live in tabbrowser, and we currently deliberately avoid initializing tabbrowser until later. Brian/Emilio/Dão, thoughts? If I had to pick right now, I guess I would go with making the code check the window type (which we do elsewhere in nsXULWindow) and decide the window is a browser window if it's navigator:browser, as this is what we use elsewhere in frontend code. bug 1476333 or follow-ups from there may then change that to some other thing.

(In reply to :Gijs (he/him) from comment #130) > Other options include checking the document URI (feels yucky, x-ref also bug > 1476333 ), As an interim solution this seems okay to me, if we can use a define after bug 1476333 instead of hardcoding the URI (or using the browser.chromeURL pref, I guess). > If I had to pick right now, I guess I would go with making the code check > the window type (which we do elsewhere in nsXULWindow) and decide the window > is a browser window if it's navigator:browser, as this is what we use > elsewhere in frontend code. bug 1476333 or follow-ups from there may then > change that to some other thing. This sounds fine too, but if we're counting on bug 1476333 anyway, we might as well start with checking the URI now.

(In reply to Dão Gottwald [::dao] from comment #131) > (In reply to :Gijs (he/him) from comment #130) > > Other options include checking the document URI (feels yucky, x-ref also bug > > 1476333 ), > > As an interim solution this seems okay to me, if we can use a define after > bug 1476333 instead of hardcoding the URI (or using the browser.chromeURL > pref, I guess). > > > If I had to pick right now, I guess I would go with making the code check > > the window type (which we do elsewhere in nsXULWindow) and decide the window > > is a browser window if it's navigator:browser, as this is what we use > > elsewhere in frontend code. bug 1476333 or follow-ups from there may then > > change that to some other thing. > > This sounds fine too, but if we're counting on bug 1476333 anyway, we might > as well start with checking the URI now. I think checking the window type makes sense in this case. What I'd like to get out of bug 1476333 is that any time we need to look up the URL we get it from one place, so that it's easier to change the URL to something else. I don't think we necessarily need to stop checking navigator:browser as a way to ask: "is this a browser window?". We still rely on it for identifying browser windows in various Services.wm calls, for instance.

(In reply to :Gijs (he/him) from comment #130) > If I had to pick right now, I guess I would go with making the code check > the window type (which we do elsewhere in nsXULWindow) and decide the window > is a browser window if it's navigator:browser, as this is what we use > elsewhere in frontend code. bug 1476333 or follow-ups from there may then > change that to some other thing. Ugh, all this code is indeed so messy... I agree this makes sense for now. Figuring out a way for this not to be so messy would be even better, but that's the purpose of bug 1476748.

OK, I have a local fix for the resistfingerprinting issue that checks the window type instead of whether we have primary content, and always sets mIgnoreXULSize and mIgnoreXULSizeMode (also when we don't have primary content yet). Let's hope I'm not breaking anything else. I also took the liberty of changing one of the "load this thing from the window" method names to try and make it clearer what it did, and rejigged the sizemode code wrt mIgnoreXULSizeMode to make it more obvious what it's doing. Trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb922494de2c85ca1fe5c945f9203bafec7f06b Assuming this is green, I suspect I'll ask for review and split out any remaining "new tab" title work to a separate bug + patch, as I *think* the "actual" perf gains are in these patches, and the "perceived" perf gain is maybe more in that to-be-separated patch which sets the window/tab title sooner.

(In reply to :Gijs (he/him) from comment #134) > Trypush: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=dbb922494de2c85ca1fe5c945f9203bafec7f06b D'oh, missed a nullcheck for the windowElement. Verified that adding one fixes things, and uploading those for review.

Comment on attachment 8993848 [details] Bug 1362774 - make fingerprint resistance not resize window before onDOMContentLoaded and detect browser windows better, https://reviewboard.mozilla.org/r/258506/#review265616 LGTM. Thanks for working on this.

Comment on attachment 8993849 [details] Bug 1362774 - insert initial browser dynamically to be able to set nodefaultsrc on it, https://reviewboard.mozilla.org/r/258508/#review265650 ::: browser/base/content/tabbrowser.js:278 (Diff revision 1) > - delete this.initialBrowser; > - return this.initialBrowser = document.getElementById("tabbrowser-initialBrowser"); > - }, > - > _setupInitialBrowserAndTab() { > - let browser = this.initialBrowser; > + const firstURL = window.arguments && window.arguments[0]; This isn't reliable due to how we load the home page and how this interacts with session restore. See gBrowserInit._uriToLoadPromise.

Comment on attachment 8993848 [details] Bug 1362774 - make fingerprint resistance not resize window before onDOMContentLoaded and detect browser windows better, https://reviewboard.mozilla.org/r/258506/#review265798 r=me ::: commit-message-4f12d:7 (Diff revision 1) > + > +This changes the fingerprint resistance code to set mIgnoreXULSize and > +mIgnoreXULSizeMode even when there isn't yet a primary content container, > +if this is a browser window. > + > +It also changes removes some dead code and reorders some other logic wrt s/changes removes/removes/ ::: xpfe/appshell/nsXULWindow.cpp:1391 (Diff revision 1) > > - // sizemode > + // If we are told to ignore the size mode attribute, force > + // normal sizemode. > + if (mIgnoreXULSizeMode) { > + ErrorResult rv; > + windowElement->SetAttribute(MODE_ATTRIBUTE, NS_LITERAL_STRING("normal"), rv); Better to just pass IgnoreErrors() as the las arg if that's what we're trying for here. ::: xpfe/appshell/nsXULWindow.cpp:1396 (Diff revision 1) > + } else { > + // Otherwise, read sizemode from DOM and, if the window is resizable, > + // set it later. > - windowElement->GetAttribute(MODE_ATTRIBUTE, stateString); > + windowElement->GetAttribute(MODE_ATTRIBUTE, stateString); > - nsSizeMode sizeMode = nsSizeMode_Normal; > - /* ignore request to minimize, to not confuse novices > + /* ignore request to minimize, to not confuse novices Do we really want to keep this comment? I would drop it, since it's just commented out code and a comment explaining what that code would do if it were uncommented, which we don't plan to do. ::: xpfe/appshell/nsXULWindow.cpp:2523 (Diff revision 1) > CSSIntSize windowDiff = GetOuterToInnerSizeDifferenceInCSSPixels(mWindow); > > - // If this window has a primary content and fingerprinting resistance is > - // enabled, we enforce this window to rounded dimensions. > - if (isContent && nsContentUtils::ShouldResistFingerprinting()) { > + // If we're using fingerprint resistance, we're going to resize the window > + // once we have primary content. > + if (nsContentUtils::ShouldResistFingerprinting() && > + windowType.EqualsLiteral("navigator:browser")) { This is a bit icky, but I have no better suggestions... :( I guess various other gecko code depends on this string already.

(In reply to Dão Gottwald [::dao] from comment #139) > This isn't reliable due to how we load the home page and how this interacts > with session restore. See gBrowserInit._uriToLoadPromise. I've added comments in the code. I can't use _uriToLoadPromise directly because it returns "null" when session restore will override the homepage, which would look like we're forcing ourselves to load about:blank even when we're not. Essentially there are 2 cases that make this more complicated due to sessionrestore. One is where session store will load about:blank after whatever homepage URL we get passed. I checked, and sessionstore will just overwrite that value so we're fine. The other is where homepage is set to about:blank ("blank page" in the prefs) but then sessionstore will overwrite it. I added code to detect this case so we can avoid loading about:blank in those cases. I also added a small optimization to the sessionstore code to make that last case faster (ie not require a promise resolution in sessionrestore) for any windows opened after the initial restore. Right now, if you have sessionrestore enabled, and open a new window after the session has been restored, willOverrideHomepage will return a promise that then resolves with `null` on the next tick, which will cause the callWithUriToLoad methods to wait an extra tick (or two) before calling their callbacks, loading URIs, etc. But sessionrestore *knows* that it's already finished and won't override the homepage again, so it can just return immediately, like it does for the first window if automatic session restore is not enabled.

Comment on attachment 8994489 [details] Bug 1362774 - return early from willOverrideHomepage if we know we won't override again, https://reviewboard.mozilla.org/r/259040/#review266078 Yeah, I like it! Thanks.

Comment on attachment 8993849 [details] Bug 1362774 - insert initial browser dynamically to be able to set nodefaultsrc on it, https://reviewboard.mozilla.org/r/258508/#review266128 ::: browser/base/content/tabbrowser.js:282 (Diff revision 2) > _setupInitialBrowserAndTab() { > - let browser = this.initialBrowser; > - this._selectedBrowser = browser; > - > - browser.permanentKey = {}; > + // Determine the first URL we load. We can't use gBrowserInit's > + // _uriToLoadPromise because it's null even if we know session restore is > + // active, in which case we shouldn't bother loading about:blank. > + const firstURL = window.arguments && window.arguments[0]; > + // accessibility tests send firstURL with value null, several mochitests Can we change those accessibility tests and then use _uriToLoadPromise here? ::: browser/base/content/tabbrowser.js:323 (Diff revision 2) > - > let tab = this.tabs[0]; > this._selectedTab = tab; > > let uniqueId = this._generateUniquePanelID(); > - this.tabpanels.childNodes[0].id = uniqueId; > + notificationbox.id = uniqueId; nit: move this up to where you set up the browser ::: browser/base/content/tabbrowser.js:331 (Diff revision 2) > tab._tPos = 0; > tab._fullyOpen = true; > tab.linkedBrowser = browser; > this._tabForBrowser.set(browser, tab); > > + this._appendStatusPanel(); ditto

(In reply to Dão Gottwald [::dao] from comment #146) > Comment on attachment 8993849 [details] > Bug 1362774 - insert initial browser dynamically to be able to set > nodefaultsrc on it, > > https://reviewboard.mozilla.org/r/258508/#review266128 > > ::: browser/base/content/tabbrowser.js:282 > (Diff revision 2) > > _setupInitialBrowserAndTab() { > > - let browser = this.initialBrowser; > > - this._selectedBrowser = browser; > > - > > - browser.permanentKey = {}; > > + // Determine the first URL we load. We can't use gBrowserInit's > > + // _uriToLoadPromise because it's null even if we know session restore is > > + // active, in which case we shouldn't bother loading about:blank. > > + const firstURL = window.arguments && window.arguments[0]; > > + // accessibility tests send firstURL with value null, several mochitests > > Can we change those accessibility tests and then use _uriToLoadPromise here? There's a few problems with that. First, the comment is from Gandalf's original patch. I don't know how many tests there are, but more generally, uriToLoadPromise returns `null` for the case where the initial URI is falsy, which then also includes the other tests that pass "", not just the a11y tests that pass null. I'd really like to get this landed rather than chase up an unknown number of tests that might be doing this first, which judging by the patches in bug 1397365 might be a substantial number... Plus, if there's a way for web content to do this and we already open about:blank for that case, we can't actually break it... Second, uriToLoadPromise can return a promise if session restore is active, and the tabbrowser.js code here can't wait for a promise, and can and should avoid loading about:blank if the homepage (ie window.arguments[0]) is not itself also about:blank, even if that homepage could get overridden by session restore. But uriToLoadPromise doesn't expose the initial URI in this case, so if it returns a promise I'd still need to do a second check against window.arguments, which IMO doesn't help code clarity at all. It also seems wrong to use the _prefixed property from a different file and object. What's the outcome you're looking for here? Would it be acceptable if instead I made uriToLoadPromise public (ie removed _), and return an object that looked something like: { initialURI: window.arguments && window.arguments[0], willOverrideHomepage: bool-or-promise } , made _callWithURIToLoad() do the `willOverrideHomePage ? null : uri` dance, and then used that from tabbrowser.js ? Either way I'll need the raw URI value immediately for the cases where uriToLoadPromise waits for session restore to init...

(In reply to :Gijs (he/him) from comment #147) > (In reply to Dão Gottwald [::dao] from comment #146) > > Comment on attachment 8993849 [details] > > Bug 1362774 - insert initial browser dynamically to be able to set > > nodefaultsrc on it, > > > > https://reviewboard.mozilla.org/r/258508/#review266128 > > > > ::: browser/base/content/tabbrowser.js:282 > > (Diff revision 2) > > > _setupInitialBrowserAndTab() { > > > - let browser = this.initialBrowser; > > > - this._selectedBrowser = browser; > > > - > > > - browser.permanentKey = {}; > > > + // Determine the first URL we load. We can't use gBrowserInit's > > > + // _uriToLoadPromise because it's null even if we know session restore is > > > + // active, in which case we shouldn't bother loading about:blank. > > > + const firstURL = window.arguments && window.arguments[0]; > > > + // accessibility tests send firstURL with value null, several mochitests > > > > Can we change those accessibility tests and then use _uriToLoadPromise here? > > There's a few problems with that. > > First, the comment is from Gandalf's original patch. I don't know how many > tests there are, but more generally, uriToLoadPromise returns `null` for the > case where the initial URI is falsy, which then also includes the other > tests that pass "", not just the a11y tests that pass null. I'd really like > to get this landed rather than chase up an unknown number of tests that > might be doing this first, which judging by the patches in bug 1397365 might > be a substantial number... Looks like Gandalf already chased down those tests and fixed them in bug 1397365. It doesn't seem like we're opening an unmanageable can of worms here. Most of these test fixes can probably be rebased easily. Even if it turns out there's more work to do, it would be nice to start with finally getting these patches landed. > Plus, if there's a way for web content to do this > and we already open about:blank for that case, we can't actually break it... Not sure what you mean here, it doesn't sound like this should be possible. > Second, uriToLoadPromise can return a promise if session restore is active, > and the tabbrowser.js code here can't wait for a promise, and can and should > avoid loading about:blank if the homepage (ie window.arguments[0]) is not > itself also about:blank, even if that homepage could get overridden by > session restore. Wait, do we actually need to load about:blank if that's the homepage? > It also seems wrong to use the _prefixed property from a different file and > object. We can remove the underscore.

(In reply to Dão Gottwald [::dao] from comment #148) > > Plus, if there's a way for web content to do this > > and we already open about:blank for that case, we can't actually break it... > > Not sure what you mean here, it doesn't sound like this should be possible. So I just checked this, and if I open e.g. https://www.example.com/ , and then run: window.open("", "_blank", "chrome,toolbar,height=500"); or even window.open("https://www.mozilla.org/", "_blank", "chrome,toolbar,height=500"); (potentially after allowing popups) then `window.arguments && window.arguments[0]` is undefined. So it seems that when web content opens a window, we don't get told what the URL is. > Wait, do we actually need to load about:blank if that's the homepage? Given how the code handling nodefaultsrc and uriIsAboutBlank (as params for adding tabs / creating browsers) was written, my understanding is that we're trying to continue to load about:blank if it's possible that that is the URL we eventually want to load into the tab anyway. If about:blank is the homepage, and we're not sure that session restore will try to load something else in that tab, then I therefore assume that we should err on the side of caution and set uriIsAboutBlank. Does that make sense? Am I missing the point of how the nodefaultsrc/uriIsAboutBlank stuff is supposed to work?

(In reply to :Gijs (he/him) from comment #149) > (In reply to Dão Gottwald [::dao] from comment #148) > > > Plus, if there's a way for web content to do this > > > and we already open about:blank for that case, we can't actually break it... > > > > Not sure what you mean here, it doesn't sound like this should be possible. > > So I just checked this, and if I open e.g. https://www.example.com/ , and > then run: > > window.open("", "_blank", "chrome,toolbar,height=500"); > > or even > > window.open("https://www.mozilla.org/", "_blank", > "chrome,toolbar,height=500"); > > (potentially after allowing popups) > > > then `window.arguments && window.arguments[0]` is undefined. > > So it seems that when web content opens a window, we don't get told what the > URL is. Hmm, this probably changed with e10s. What code path does this use then? > > Wait, do we actually need to load about:blank if that's the homepage? > > Given how the code handling nodefaultsrc and uriIsAboutBlank (as params for > adding tabs / creating browsers) was written, my understanding is that we're > trying to continue to load about:blank if it's possible that that is the URL > we eventually want to load into the tab anyway. If about:blank is the > homepage, and we're not sure that session restore will try to load something > else in that tab, then I therefore assume that we should err on the side of > caution and set uriIsAboutBlank. > > Does that make sense? Am I missing the point of how the > nodefaultsrc/uriIsAboutBlank stuff is supposed to work? That may have been the original idea but we can change this. The crucial point is that if content opens about:blank, we absolutely need to load that. With the homepage that's much less clear to me. What exactly happens if you open a window with nodefaultsrc set on the browser without loading anything?

(In reply to Dão Gottwald [::dao] from comment #150) > Hmm, [handling content window.open] probably changed with e10s. What code path does this use then? I'm not sure, but from a quick local test this works fine** even if we just set uriIsAboutBlank to false outright (meaning we always set nodefaultsrc to true). ** in the sense that the new about:blank document loads and the opener window can manipulate it and the new browser's documentURI (about:blank) and contentPrincipal (example.com inherited from the opener) are set correctly. > > > Wait, do we actually need to load about:blank if that's the homepage? > > > > Given how the code handling nodefaultsrc and uriIsAboutBlank (as params for > > adding tabs / creating browsers) was written, my understanding is that we're > > trying to continue to load about:blank if it's possible that that is the URL > > we eventually want to load into the tab anyway. If about:blank is the > > homepage, and we're not sure that session restore will try to load something > > else in that tab, then I therefore assume that we should err on the side of > > caution and set uriIsAboutBlank. > > > > Does that make sense? Am I missing the point of how the > > nodefaultsrc/uriIsAboutBlank stuff is supposed to work? > > That may have been the original idea but we can change this. The crucial > point is that if content opens about:blank, we absolutely need to load that. > With the homepage that's much less clear to me. What exactly happens if you > open a window with nodefaultsrc set on the browser without loading anything? Based on the above where I can see no reason to actually use this for content about:blank, and you not seeing a reason for using it with an about:blank homepage (and there too, I don't see issues in testing***), I just pushed to try with the patch changed to always set uriIsAboutBlank to false. Let's see how that goes and what (if anything) does break. That'd certainly be the simpler option. *** besides a pre-existing issue with the 'blank' attribute not being removed from the tab and the tab thus not rendering content that the user inserts into the empty, null-principal'd about:blank document manually via e.g. devtools or a bookmarklet. I'll file separately, doesn't seem urgent given the STR. https://treeherder.mozilla.org/#/jobs?repo=try&revision=201fa5c48986af73217cc6f268547ba9aa069a82 FWIW, given the two other csets are minor improvements in and of themselves I think I will land them in separate bugs while we hash out the details here, also to avoid bitrot. If we *can* basically always set nodefaultsrc on the initial browser now, I was considering moving back to putting things in the markup - but on the other hand, it seems like we'll likely want to set the remote and remoteType attributes (esp. once we land bug 1472212) dynamically as well, so maybe it's worth keeping that dynamic even if for this bug we could get away with having it be static.

(In reply to :Gijs (he/him) from comment #151) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=201fa5c48986af73217cc6f268547ba9aa069a82 > > FWIW, given the two other csets are minor improvements in and of themselves > I think I will land them in separate bugs while we hash out the details > here, also to avoid bitrot. > > If we *can* basically always set nodefaultsrc on the initial browser now, I > was considering moving back to putting things in the markup - but on the > other hand, it seems like we'll likely want to set the remote and remoteType > attributes (esp. once we land bug 1472212) dynamically as well, so maybe > it's worth keeping that dynamic even if for this bug we could get away with > having it be static. Based on the trypush, this breaks at least 190 tests. There are likely more because some tests timed out, causing their suites to exceed their runtime and abort, so not everything got run. How would you feel about landing this with the false-y checks present in some form (tbd if/how that'd rely on uriToLoadPromise), and filing a follow-up to see if we can improve further by adjusting tests? I realize some of the fixes in bug 1397365 might help with some of these tests, but I think working on this iteratively would be better than trying to keep a patchset together that touches quite that many tests. I'm also worried that not all the test changes in bug 1397365 look uncontroversial to me. For instance, I'm already uncomfortable with how different BTU.openNewBrowserWindow() is from how OpenBrowserWindow() works, because then we're not testing what clicking "new window" in the UI actually does, and the fix there (to make it always load a dummy data: URI) exacerbates that problem.

(In reply to :Gijs (he/him) from comment #152) > Based on the trypush, this breaks at least 190 tests. There are likely more > because some tests timed out, causing their suites to exceed their runtime > and abort, so not everything got run. How would you feel about landing this > with the false-y checks present in some form (tbd if/how that'd rely on > uriToLoadPromise), and filing a follow-up to see if we can improve further > by adjusting tests? > > I realize some of the fixes in bug 1397365 might help with some of these > tests, but I think working on this iteratively would be better than trying > to keep a patchset together that touches quite that many tests. Keeping a large patchset together hasn't worked the last time and I didn't mean to say we should try again. I would rather land the already-r+'d test fixes *now* and see where that gets us... That's iterative too, just in a different order than you're proposing...

(In reply to Dão Gottwald [::dao] from comment #153) > (In reply to :Gijs (he/him) from comment #152) > > Based on the trypush, this breaks at least 190 tests. There are likely more > > because some tests timed out, causing their suites to exceed their runtime > > and abort, so not everything got run. How would you feel about landing this > > with the false-y checks present in some form (tbd if/how that'd rely on > > uriToLoadPromise), and filing a follow-up to see if we can improve further > > by adjusting tests? > > > > I realize some of the fixes in bug 1397365 might help with some of these > > tests, but I think working on this iteratively would be better than trying > > to keep a patchset together that touches quite that many tests. > > Keeping a large patchset together hasn't worked the last time and I didn't > mean to say we should try again. I would rather land the already-r+'d test > fixes *now* and see where that gets us... That's iterative too, just in a > different order than you're proposing... Yes, the problem with your suggested order is that the patch here is bitrotting while we figure out all the tests. For instance, your refactors of the tabbrowser code caused conflicts (fine, I guess), but bug 1442582 not only caused conflicts but broke the patch after resolving the conflicts - no tests run now, startup hangs with an error about gBrowser.selectedBrowser.stop() being undefined. I haven't yet figured out why. :-(

FWIW, I split off a patch where we make creating the initial browser dynamic (instead of hardcoded in markup) to bug 1486079 , without changing whether we add the `nodefaultsrc` attribute for now. I intend to file further split-offs to land individual test changes either directly copied from bug 1397365 or different ways of fixing the same tests, starting the beginning of next week. I started work on a change to BrowserTestUtils.openNewBrowserWindow already, as I figured that'd address most of the tests. Fairly green trypush at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee9de926da36f8c3017386f14e18ac8e404c5f2c - I need to make some more changes but the fallout's not too bad. I know the patches in bug 1397365 were trying to change it to use a data URI by default, but I was hoping to just make it rely on browser.js's BrowserOpenWindow, so it'll open the default new tab page, to increase test coverage of what "actually happens" when users open new windows, instead of having BTU duplicate much of the logic that's in BrowserOpenWindow to begin with.

I'm probably jinxing it, but I'm hoping this can land this week. Latest trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4618b80ab9592e8357a03fd0b2e6a7a8567ecf1 I forgot the patch for bug 1488289, which explains the origin attributes orange. Otherwise the only thing I'm seeing that might need more attention are the printing code leaks, but I'm not convinced that has anything to do with these patches...

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0ff4d45ea0 Several dom/workers tests need to wait for the environment to be ready. r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c09042e670 don't create about:blank for the initial browser, r=dao

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4367fd26b8 followup, disable test_mousecapture.xhtml on linux64 quantumrender dbg only, r=aryx

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2395236d7f22 followup, disable leaky print preview tests on linux only (x-ref bug 1503887), r=aryx

https://hg.mozilla.org/mozilla-central/rev/8c0ff4d45ea0 https://hg.mozilla.org/mozilla-central/rev/e6c09042e670 https://hg.mozilla.org/mozilla-central/rev/2e4367fd26b8 https://hg.mozilla.org/mozilla-central/rev/2395236d7f22

Verified, that the issue is not reproducible on Nightly 65(20181120100045).