See bug 1420423 comment 63 and following.

For the second patch, I have some question, though. After you do that, what would actually trigger the restyle? I cannot really find answer from the code immediately. It seems to me PresShell::Init() sets NeedStyleFlush, and that flag is only cleared inside PresShell::DoFlushPendingNotifications. That indicates that we would either have had an initial restyle which uses the old media feature values, or we would do a flush which includes a media feature values flush itself. Why does it make sense to do an additional flush in PresShell::Initialize()? Or maybe it is expected that we do the initial style but leave NeedStyleFlush set there, and hope the next style flush would be a no-op?

Want to convince me that calling Center before LoadMiscPersistentAttributesFromXUL is ok?

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3) > For the second patch, I have some question, though. After you do that, what > would actually trigger the restyle? I cannot really find answer from the > code immediately. > > It seems to me PresShell::Init() sets NeedStyleFlush, and that flag is only > cleared inside PresShell::DoFlushPendingNotifications. That indicates that > we would either have had an initial restyle which uses the old media feature > values, or we would do a flush which includes a media feature values flush > itself. Why does it make sense to do an additional flush in > PresShell::Initialize()? > > Or maybe it is expected that we do the initial style but leave > NeedStyleFlush set there, and hope the next style flush would be a no-op? Right. It doesn't make (much) sense, it's just the way PresShell::Initialize works. Most of the time the needs flush flag would already be set by MediumFeatureValuesChanged... It's unfortunate, in the sense that it's usually not needed, but the idea is that the next restyle would be a no-op. It'd be nice to be able to write PresShell::Initialize in terms of FlushPendingNotifications IMO. (In reply to Olli Pettay [:smaug] from comment #4) > Want to convince me that calling Center before > LoadMiscPersistentAttributesFromXUL is ok? I thought it was because I didn't look at all the widgets, but some do access size mode from their nsWindow impl, so it needs to run before Center() indeed. Fixed. I didn't bother guarding it in mChromeLoaded since a size mode change can also change media features, and thus cause a full doc restyle (though I suspect we don't have any size mode media queries now).

Comment on attachment 8952689 [details] Bug 1439875: Size the XUL window before doing layout. https://reviewboard.mozilla.org/r/221918/#review227806 The setup is a bit messy, since BeforeStartLayout and OnChromeLoaded do almost the same things. But I guess this is fine.

[ Triage 2018/02/21: P3 ]

This should probably be P1 since it may block stylo-chrome from shipping.

So this fails browser/base/content/test/performance/browser_windowopen_flicker.js like: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ab2a737c3b46e0ea456afb09d07ad1156669707&selectedJob=163658575 This is sort-of expected, because right now these are suppressed in: https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/browser/base/content/test/performance/browser_windowopen_flicker.js#77 Florian, should I just update the test expectations?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11) The main change here is that you are painting the browser window before its images have loaded. I think this is fine, especially now that bug 1434945 is fixed and that everything has the right size and position within the window initially. However, when looking at the test failures, I also see that - on Mac the window gets resized an fully repainted. - on Windows 7, the window is initially painted inactive (visible on the titlebar color) > Florian, should I just update the test expectations? I think it's fine to update the test expectations to accept that the window can be painted first without icons, and that all the icons appear at once later.

Comment on attachment 8953097 [details] Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window. https://reviewboard.mozilla.org/r/222380/#review228318 Looks reasonable to me, thanks!

Comment on attachment 8953097 [details] Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window. https://reviewboard.mozilla.org/r/222380/#review228320 r+ for the .cpp

Comment on attachment 8952690 [details] Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. https://reviewboard.mozilla.org/r/221920/#review228364 r=dbaron with the following: ::: layout/base/PresShell.cpp:1753 (Diff revision 3) > + // Ensure the pres context doesn't think it has changed, since we haven't even > + // started layout. > + // > + // This avoids spurious restyles / reflows afterwards. Please explicitly comment that this is *before* the setting of mDidInitialize to true so that FlushPendingMediaFeatureValuesChanged will do only the style change and not the notification.

Comment on attachment 8953096 [details] Bug 1439875: Fix extension windows. https://reviewboard.mozilla.org/r/222338/#review228408

I'm still going through the browser/base/content/test/performance/browser_windowopen_flicker.js expectations, trying to get them right in all platforms... I'll land the second commit, which can land separately, in another bug

So those should be easy to figure out, I've just been sidetracked. Now, there's another scary failure that I can't reproduce (because I'm on Linux), but may be more annoying... It's the test for XUL window size constraints, bug 357725: layout/xul/test/test_windowminmaxsize.xul. Looks like Windows and OSX don't honor the size constraints if they're already sized. It doesn't look like we use this feature for anything though. Now we're starting to size the window before layout, while before the constraints were applied while the window was 1x1. Neil, Boris, what should I do about that test? Should I remove it / mark it as failing and remove the code from bug 357725 in a followup? Should I move the minwidth / minheight attribute handling to LoadSizeFromXUL? (That last one may be the proper fix, but if we don't use it nor plan to use that we may as well remove it IMO).

I don't really know anything about this size constraint stuff offhand, sorry. :(

(I guess we need to keep the size constraints for nsMenuPopupFrame, but we definitely don't use it for <window>)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #22) > Now we're starting to size the window before layout, while before the > constraints were applied while the window was 1x1. Does this mean we won't anymore have to deal with the window being sized 1x1 initially? Because I just recently added a workaround for that in bug 1438504.

(In reply to Dão Gottwald [::dao] from comment #25) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #22) > > Now we're starting to size the window before layout, while before the > > constraints were applied while the window was 1x1. > > Does this mean we won't anymore have to deal with the window being sized 1x1 > initially? Because I just recently added a workaround for that in bug > 1438504. Yes, this bug will remove the workaround for that also in our tests, like https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/browser/base/content/test/performance/browser_windowopen_flicker.js#77. The issue is that now I need to update the test, which is what I'm doing now.

Oh, no wonder why I wasn't managing to make the flicker test to pass, got beaten by automatic semicolon insertion in JS. Amazing :( diff --git a/browser/base/content/test/performance/browser_windowopen_flicker.js b/browser/base/content/test/performance/browser_windowopen_flicker.js index 6367bca5d40c..a2ecf2ae225f 100644 --- a/browser/base/content/test/performance/browser_windowopen_flicker.js +++ b/browser/base/content/test/performance/browser_windowopen_flicker.js @@ -103,9 +103,8 @@ add_task(async function() { if (inRange(r.h, 13, 15) || !inRange(r.w, 14, 16)) return false; // Not an icon - return - inRange(r.y1, 40, 80) || // in the toolbar - inRange(r.x1, 9, 24) // in the tab strip + return inRange(r.y1, 40, 80) || // in the toolbar + inRange(r.x1, 9, 24); // in the tab strip } } ];

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27) > Oh, no wonder why I wasn't managing to make the flicker test to pass, got > beaten by automatic semicolon insertion in JS. Amazing :( > > > diff --git > a/browser/base/content/test/performance/browser_windowopen_flicker.js > b/browser/base/content/test/performance/browser_windowopen_flicker.js > index 6367bca5d40c..a2ecf2ae225f 100644 > --- a/browser/base/content/test/performance/browser_windowopen_flicker.js > +++ b/browser/base/content/test/performance/browser_windowopen_flicker.js > @@ -103,9 +103,8 @@ add_task(async function() { > if (inRange(r.h, 13, 15) || !inRange(r.w, 14, 16)) > return false; // Not an icon > > - return > - inRange(r.y1, 40, 80) || // in the toolbar > - inRange(r.x1, 9, 24) // in the tab strip > + return inRange(r.y1, 40, 80) || // in the toolbar > + inRange(r.x1, 9, 24); // in the tab strip > } > } > ]; eslint should have complained about this (because unreachable code). I guess that's not enabled in tests, or something?

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review229076 ::: browser/base/content/test/performance/browser_windowopen_flicker.js:100 (Diff revision 1) > - let rects = compareFrames(frame, previousFrame).filter(rect => { > let inRange = (val, min, max) => min <= val && val <= max; > let width = frame.width; > > let exceptions = [ > - {name: "bug 1421463 - reload toolbar icon shouldn't flicker", > + { We still need this exception for the reload icon; it flickers, we should fix it, and the test should catch this. If this becomes caught by another exception, then that other exception is too broad and makes the test miss its purpose. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:101 (Diff revision 1) > let inRange = (val, min, max) => min <= val && val <= max; > let width = frame.width; > > let exceptions = [ > - {name: "bug 1421463 - reload toolbar icon shouldn't flicker", > - condition: r => r.h == 13 && inRange(r.w, 14, 16) && // icon size > + { > + name: "Loading of icons", This exception is so broad that it's almost equivalent to disabling the test. In all the screenshots I've seen from the failing test in your previous try run, all the icons appeared at once; we should enforce that in the test. Have you encountered a case where the exception we have a few lines above wasn't enough? (ie where we first paint the window as inactive and without icon, then active without icons, and finally active with icons)

Comment on attachment 8952690 [details] Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. https://reviewboard.mozilla.org/r/221920/#review229078 Nice! :-)

So, here are the failures in startup_flicker.js only with the activation bits: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c5532b064f35c91256fada2ea287acac16d569 Ignoring C3 (that's comment 22), looks like: * On Windows somehow the minimize button flickers. I have no clue what it's about. * On OSX, we have this weird window resize stuff, which apparently is intermittent, since in my previous try run wasn't there. I cannot reproduce any of those, since I'm on Linux. Florian, is it acceptable to just file bugs / exceptions for those? I suspect that the OSX issue is either test-harness related, or a widget issue... I'll submit a patch with those now. If not, what should I do about those?

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review229180 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/test/performance/browser_windowopen_flicker.js:109 (Diff revision 2) > - inRange(r.y1, 40, 80) && // in the toolbar > + inRange(r.y1, 40, 80) && // in the toolbar > - // near the left side of the screen > + // near the left side of the screen > - // The reload icon is shifted on devedition builds > + // The reload icon is shifted on devedition builds > - // where there's an additional devtools toolbar icon. > + // where there's an additional devtools toolbar icon. > - AppConstants.MOZ_DEV_EDITION ? inRange(r.x1, 100, 120) : > + AppConstants.MOZ_DEV_EDITION ? inRange(r.x1, 100, 120) : > - inRange(r.x1, 65, 100) > + inRange(r.x1, 65, 100) Error: Missing semicolon. [eslint: semi]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #36) > * On Windows somehow the minimize button flickers. I have no clue what it's > about. The minimize button is in the hovered state on the second screeshot. Is it possible that the mouse cursor happens to be in the most inconvenient place before starting the test? If so, we could probably force the mouse to move to a harmless place before we open the window. > * On OSX, we have this weird window resize stuff, which apparently is > intermittent, since in my previous try run wasn't there. To figure out what's intermittent here, I would suggest running try with this syntax: try: -b do -p linux,linux64,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc,test-verify-e10s -t none --try-test-paths browser-chrome:browser/base/content/test/performance This resize issue is similar to something I had to workaround in bug 1336227. Here is my workaround: https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/browser/components/nsBrowserGlue.js#62-64 Given that you are removing a resize event, you are likely also removing what was hiding this bug. So this is most likely a widget bug. When you ask if it's ok to add an exception for it, I assume you are really asking if it's ok to ship with this bug or if it's a blocker. If it's just a small resize flicker that lasts one frame and happens only on OS X, it's probably not a blocker. But if it's causing new windows to be opened at a size 22px (the height of a standard titlerbar on OS X) larger than specified, it's probably a blocker. Whether it actually blocks landing this bug or not, filing a bug to needinfo widget people is likely useful. Your try push also has a browser/base/content/test/performance/browser_startup_images.js orange (in the Windows 10 x64 debug bc4 job).

It seems to me this change may have some risks, so it'd be better that we can land it before the soft code freeze...

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review229942 ::: browser/base/content/test/performance/browser_windowopen_flicker.js:89 (Diff revision 2) > + // focus change if there are at least 5 areas that changed near the top of > + // the screen, but will only ignore this once (hence the alreadyFocused > + // variable). > + if (!alreadyFocused && rects.length > 5 && rects.every(r => r.y2 < 100)) { > + alreadyFocused = true; > + // This is likely an issue caused by the test harness, but log it anyway. I think we are now relatively sure that it's a real bug and not caused by the test harness. I had doubt about the test harness for the startup test, but for the 'windowopen' case, there's no reason for it to interfere. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:101 (Diff revision 2) > let inRange = (val, min, max) => min <= val && val <= max; > let width = frame.width; > > let exceptions = [ > - {name: "bug 1421463 - reload toolbar icon shouldn't flicker", > - condition: r => r.h == 13 && inRange(r.w, 14, 16) && // icon size > + { > + name: "bug 1421463 - reload toolbar icon shouldn't flicker", I don't see the point of changing the style here. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:120 (Diff revision 2) > + if (AppConstants.platform !== "win") > + return false; > + const isWin10 = > + AppConstants.isPlatformAndVersionAtLeast("win", "10") && > + AppConstants.isPlatformAndVersionAtMost("win", "10"); > + if (!isWin10) This can all be simplified to: AppConstants.isPlatformAndVersionAtLeast("win", "10") && AppConstants.isPlatformAndVersionAtMost("win", "10") && before the actual check for r.y1 ::: browser/base/content/test/performance/browser_windowopen_flicker.js:122 (Diff revision 2) > + const isWin10 = > + AppConstants.isPlatformAndVersionAtLeast("win", "10") && > + AppConstants.isPlatformAndVersionAtMost("win", "10"); > + if (!isWin10) > + return false; > + return r.y1 === 1 && r.x1 === 998 && r.w === 45 && r.h === 32; nit: The style in browser/ is usually "==" rather than "===", unless there is a specific reason to need a strict equality. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:126 (Diff revision 2) > + return false; > + return r.y1 === 1 && r.x1 === 998 && r.w === 45 && r.h === 32; > + } > + }, > + { > + name: "bug TBD - Mac window shouldn't resize and activate late causing flickering", I'm not really comfortable with whitelisting this failure, but if we end up having to do it, we should check that it's indeed a resize happening, ie. frame and previousFrame have different sizes.

So I have a tentative patch thanks to the chat I had with Markus running through try that should fix the Mac flickering thingie. After that I'll look into fixing the size constraints tests, though I need to do it from the office since I don't have a Mac / Windows handy.

Current state of affairs with that fix is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9a14e25ce885bad07a8e9cc07357befcaf7c212. Looks like that fix did work, but now Win is unhappy about one window resizing test... Fabulous.

I'm having fun trying to repro those bugs, but since I have time, here's a talos run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e5a87d482c43&newProject=try&newRevision=f32537280336478be68e60ea1f7fb4afdb1ddc43&framework=1 Which looks very promising.

So... I guess I understand what's happening now. So, in the very early stage when we resize the window from XUL, we pass in the size we want for the client area, however, the widgets of Windows and macOS expect that size to be the outer window size, i.e. the size including the window decoration. And consequently, that window size setting ends up sizing the client area with a smaller area than desired. It doesn't happen on GTK widget because it doesn't allow you to set outer window size. The client area size is identical to the size you pass via nsIWidget::Resize. So I guess the right solution here is to invoke nsIWidget::ClientToWindowSize with the size we want before passing it into the widget.

So, according to my local test, my patch in bug 1442521 fixes test_resize_move_windows.xul and test_windowminmaxsize.xul on Windows. Given that cocoa widget has a non-trivial ClientToWindowSize like Windows, I have confidence to some extent that this should fix the issue on macOS as well.

And... that somehow works even without emilio's proposed patch to apply constraints in nsXULWindow. Sounds like that is a pretty promising fix, although I have no idea how it works then...

Hmmm... but that patch itself is probably triggering another set of failures :/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=6008ded5034eb92ed0951bc196b788da236a2f13

Patches from emilio + my patch in bug 1442521: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8093b22ccce84f1786677ed37f387b14f0f49ea7 Mac fails test_bug665540.html which according to emilio, is an issue from this bug and still under investigation. Windows fails test_resize_move_windows.xul and I'm trying to see why... It wasn't failing in my previous try push :/

The reason test_resize_move_windows.xul fails is weird... So here is what happens: When we initially open the window, we try to resize the content area to 300x100, and with the patch in bug 1442521 we figured out that we need to add 16x39 on top of the content size for the window outer size, so we pass 316x139 to widget and ask it resize. Then Windows calls back to the handler saying the client area is 304x133, and thus we set the docshell with such size. Since the outer window size is still 316x139, the difference between outer and inner size becomes only 12x6. After that, when we try to resize the window again for the features passed to window.open, we calculate the outer window size based on the difference above. Given that we want to size it to 170x170, we pass 182x176 to the widget. When that happens, it seems the client rect is magically restored to 16x39, so the docshell size becomes 166x137... It seems the result from GetClientRect isn't trustful at that time for reason I don't know.

Jim, do you have any idea about comment 50? Maybe GetClientRect isn't reliable when window hasn't been shown? (Actually I don't really understand why the client area is 16x39 smaller than window outer size... There doesn't seem to be such bold border...)

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #51) > Jim, do you have any idea about comment 50? Maybe GetClientRect isn't > reliable when window hasn't been shown? > > (Actually I don't really understand why the client area is 16x39 smaller > than window outer size... There doesn't seem to be such bold border...) Might be in the middle of a window transition maybe? I guess it depends on when this gets called. I could see this happening. For your 16x39 it could be: 8 px border x 2, 23 px (titlebar) + 8px border x 2

OK I think I've found the last missing piece on Windows widget... Need some more investigation on what's the best place to handle it.

There is a regression in tpaint that this will potentially fix. Tracked as blocking so it is closely monitored by relman team. The current plan is to let this fix bake in Nightly for a bit and then uplift to Beta60 if the risk to release is tolerable.

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review233094 ::: browser/base/content/test/performance/browser_windowopen_flicker.js:89 (Diff revision 2) > + // focus change if there are at least 5 areas that changed near the top of > + // the screen, but will only ignore this once (hence the alreadyFocused > + // variable). > + if (!alreadyFocused && rects.length > 5 && rects.every(r => r.y2 < 100)) { > + alreadyFocused = true; > + // This is likely an issue caused by the test harness, but log it anyway. That being said, it's a pre-existing bug, it just happened to be clobbered by the "tinyPaint" check. Mind if I file a followup for this?

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review229942 > I think we are now relatively sure that it's a real bug and not caused by the test harness. I had doubt about the test harness for the startup test, but for the 'windowopen' case, there's no reason for it to interfere. I filed bug 1445161 for this. > I'm not really comfortable with whitelisting this failure, but if we end up having to do it, we should check that it's indeed a resize happening, ie. frame and previousFrame have different sizes. This was fixed.

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review233094 > That being said, it's a pre-existing bug, it just happened to be clobbered by the "tinyPaint" check. > > Mind if I file a followup for this? This was an old comment that wasn't published, sigh.

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review229076 > We still need this exception for the reload icon; it flickers, we should fix it, and the test should catch this. If this becomes caught by another exception, then that other exception is too broad and makes the test miss its purpose. It does get caught by the active window thing, unfortunately, and doesn't happen on Linux. I have tried a few times and couldn't come up with a magic combination that allowed that condition to still be useful, and I can't repro locally, which makes it harder. I can keep it if you want, but try is green without it.

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review233112 ::: browser/base/content/test/performance/browser_windowopen_flicker.js:89 (Diff revision 4) > + // will only ignore this once (hence the alreadyFocused variable). > + if (!alreadyFocused && rects.length > 5 && rects.every(r => r.y2 < 100)) { > + alreadyFocused = true; > + // This is likely an issue caused by the test harness, but log it anyway. > + todo(false, > + "the window should be focused at first paint, " + rects.toSource()); Isn't this bug 1445161? ::: browser/base/content/test/performance/browser_windowopen_flicker.js:110 (Diff revision 4) > inRange(r.x1, 65, 100) > }, > + { > + name: "bug 1445159 - Windows 10 minimize button shouldn't be hovered", > + condition(r) { > + if (AppConstants.platform !== "win") != is enough And actually, the next thing you do is an isPlatformAndVersionAtLeast("win" check, so you don't need the platform check at all. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:112 (Diff revision 4) > + { > + name: "bug 1445159 - Windows 10 minimize button shouldn't be hovered", > + condition(r) { > + if (AppConstants.platform !== "win") > + return false; > + return AppConstants.isPlatformAndVersionAtLeast("win", "10") && This can easily follow the same format as the other exception above. ::: browser/base/content/test/performance/browser_windowopen_flicker.js:114 (Diff revision 4) > + condition(r) { > + if (AppConstants.platform !== "win") > + return false; > + return AppConstants.isPlatformAndVersionAtLeast("win", "10") && > + AppConstants.isPlatformAndVersionAtMost("win", "10") && > + r.y1 == 1 && r.x1 == 998 && r.w == 45 && r.h == 32; This 998 constant is fragile, it'll break whenever the window size changes even slightly. I think having something relative to the right end of the window would be safer (ie. a smaller constant compared to (width - r.x1)).

(In reply to Florian Quèze [:florian] from comment #39) > To figure out what's intermittent here, I would suggest running try with > this syntax: > > try: -b do -p linux,linux64,macosx64,win32,win64 -u > mochitest-bc,mochitest-e10s-bc,test-verify-e10s -t none --try-test-paths > browser-chrome:browser/base/content/test/performance Before r+ing the next version I would like to see a try run with this syntax.

(In reply to Florian Quèze [:florian] from comment #65) > Before r+ing the next version I would like to see a try run with this syntax. Roger, on it :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a36e1d1c3bd658369af8a81eeb22b50bb0ad9f63 (wow these take a lot to finish :/)

I removed the windows thingie because it turns out that after rebasing I cannot longer repro, here's a try run that I did to try to figure out the window width in that test, which blatantly caught nothing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b22826d460c55024045cf68c522f92a417e258b

Comment on attachment 8953980 [details] Bug 1439875: Update browser_windowopen_flicker.js. https://reviewboard.mozilla.org/r/223130/#review233176 ::: browser/base/content/test/performance/browser_startup_flicker.js:33 (Diff revision 5) > // We'll assume the changes we are seeing are due to this focus change if > // there are at least 5 areas that changed near the top of the screen, but > // will only ignore this once (hence the alreadyFocused variable). > if (!alreadyFocused && rects.length > 5 && rects.every(r => r.y2 < 100)) { > alreadyFocused = true; > // This is likely an issue caused by the test harness, but log it anyway. In my previous review comment, I mostly meant that we should remove this 'likely an issue caused by the test harness' comment now that we think it's an actual bug. But including the bug number in the test output was a good idea too!

Comment on attachment 8958431 [details] Bug 1439875: Flag chevron.svg as intermittently not loaded on windows. https://reviewboard.mozilla.org/r/227386/#review233182 I suggest johannh to review this patch. ::: browser/base/content/test/performance/browser_startup_images.js:64 (Diff revision 1) > }, > > { > file: "chrome://browser/skin/chevron.svg", > platforms: ["win", "linux", "macosx"], > + intermittentNotLoaded: ["win"], I'm not sure that's the correct fix for the failure you were seeing on try, I wouldn't be surprised if "intermittentShown" was what you need.

Comment on attachment 8958431 [details] Bug 1439875: Flag chevron.svg as intermittently not loaded on windows. https://reviewboard.mozilla.org/r/227386/#review233202

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25e681a47121 Size the XUL window before doing layout. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/fc25cbe8e22e Fire MozBeforeInitialXULLayout before sizing the window. r=florian,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/a1237d0711a2 Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/3296aef96276 Update browser_windowopen_flicker.js. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/bd81fafecd95 Flag chevron.svg as intermittently shown on windows. r=johannh

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f21bdb9b67e8 Fix extension windows. r=kmag

Backed out for failing mochitest clipboard at /browser_editcontrols_update.js and mochitest headless at /test_bug1414336.html Log for Browser-chrome failure: https://treeherder.mozilla.org/logviewer.html#?job_id=167701419&repo=mozilla-inbound&lineNumber=7171 Log for clipboard failure: https://treeherder.mozilla.org/logviewer.html#?job_id=167701424&repo=mozilla-inbound&lineNumber=4450 Log for devtools-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=167701404&repo=mozilla-inbound&lineNumber=1992 Log for mochitest-headless (h2) failure : https://treeherder.mozilla.org/logviewer.html#?job_id=167701449&repo=mozilla-inbound&lineNumber=4174

Failures look like bug 1444588, which will be fixed by bug 1443943. Will retry as soon as those land I guess :(

Backout by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/01685f397a53 Backed out 6 changesets for failing mochitest clipboard at browser/components/customizableui/test/browser_editcontrols_update.js and mochitest headless at dom/events/test/pointerevents/test_bug1414336.html on a CLOSED TREE

The clipboard failures where from the patches backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3705faaa8c.

Mochitest headless failure I _suspect_ it's not my fault, but will verify before relanding.

Bug 1444588 should suffice here.

:emilio, yes, the clipboard failures were caused by another push. Sorry. Before relanding, please check the wpt2 failures on "/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html" Log: https://treeherder.mozilla.org/logviewer.html#?job_id=167699148&repo=mozilla-inbound&lineNumber=3159

The h2 test is intermittent (see another try run with my patches where it doesn't appear[1]), I managed to repro under rr and have a patch after discussing with kats. Xidorn said he was going to take a look today to the wpt2 OSX failures, otherwise I'll grab a loaner tomorrow and set up everything again to be able to debug. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d21c581cc3e8ca1da2ee82689af5d74d1c3b8fb5

I suspect this is also an issue of persist... What seems to happen here is that, when the window is opened with neither width nor height feature set, the next window would have height shrunk. If I remove the line of "left=0,top=0", everything works fine. My guess is that, we persist the window size when none of the two features is set, and thus that affects the result here. I tried to apply the patches in bug 144525, but that seems to make things even worse in the sense that one more subtest start failing as well :/

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b33ad14ce8c Size the XUL window before doing layout. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/9ded55072903 Fire MozBeforeInitialXULLayout before sizing the window. r=florian,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b96ce3490b Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/69550dec0534 Update browser_windowopen_flicker.js. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/7f25473e9b29 Flag chevron.svg as intermittently shown on windows. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee033e0061a Fix extension windows. r=kmag

Backed out for failing browser chrome at browser/base/content/test/general/browser_private_browsing_window.js Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1ee033e0061abc239cbe8a0479ee762109b8d334 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168037127&repo=mozilla-inbound&lineNumber=13400 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/be470e9c8c1450257c431e9ea5606cff686c5bad

Ouch, those clearly weren't there yesterday... :( I've looked and one of my try runs this morning has that failure, and it was on top of central. So from the try run in comment 88 to that try run there are: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=deb7714a7bcd3448952440e92d0209abec6b886d&tochange=80b4777a6421d8df4bb27ac23fb607c318a3006c But given these failures weren't there when my patch was backed out that leaves: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=01685f397a53&tochange=80b4777a6421d8df4bb27ac23fb607c318a3006c I have no bright idea about these unfortunately, and don't see anything particularly suspicious... Bug 1427186 touched about:privatebrowsing, but seems an innocent enough change. I guess I can try to debug them tomorrow, though I have no clue about where to start investigating... ideas welcome

There are some errors in the log when that test runs that seem related: [task 2018-03-14T21:22:51.622Z] 21:22:51 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/browser-tabsintitlebar.js, line 283: TypeError: this._menuObserver is undefined [task 2018-03-14T21:22:52.104Z] 21:22:52 INFO - GECKO(1042) | JavaScript error: resource:///modules/ProcessHangMonitor.jsm, line 412: TypeError: win.gBrowser is undefined [task 2018-03-14T21:22:52.106Z] 21:22:52 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/browser-tabsintitlebar.js, line 283: TypeError: this._menuObserver is undefined [task 2018-03-14T21:22:52.126Z] 21:22:52 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/tabbrowser.js, line 301: TypeError: browser._createAutoScrollPopup is not a function [task 2018-03-14T21:22:52.581Z] 21:22:52 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/tabbrowser.js, line 301: TypeError: browser._createAutoScrollPopup is not a function [task 2018-03-14T21:22:53.729Z] 21:22:53 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/browser-tabsintitlebar.js, line 283: TypeError: this._menuObserver is undefined [task 2018-03-14T21:22:54.267Z] 21:22:54 INFO - GECKO(1042) | JavaScript error: chrome://browser/content/tabbrowser.js, line 301: TypeError: browser._createAutoScrollPopup is not a function

(In reply to Panos Astithas [:past] (please ni?) from comment #93) > There are some errors in the log when that test runs that seem related: > > [task 2018-03-14T21:22:51.622Z] 21:22:51 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/browser-tabsintitlebar.js, line 283: > TypeError: this._menuObserver is undefined > [task 2018-03-14T21:22:52.104Z] 21:22:52 INFO - GECKO(1042) | JavaScript > error: resource:///modules/ProcessHangMonitor.jsm, line 412: TypeError: > win.gBrowser is undefined > [task 2018-03-14T21:22:52.106Z] 21:22:52 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/browser-tabsintitlebar.js, line 283: > TypeError: this._menuObserver is undefined > [task 2018-03-14T21:22:52.126Z] 21:22:52 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/tabbrowser.js, line 301: TypeError: > browser._createAutoScrollPopup is not a function > [task 2018-03-14T21:22:52.581Z] 21:22:52 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/tabbrowser.js, line 301: TypeError: > browser._createAutoScrollPopup is not a function > [task 2018-03-14T21:22:53.729Z] 21:22:53 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/browser-tabsintitlebar.js, line 283: > TypeError: this._menuObserver is undefined > [task 2018-03-14T21:22:54.267Z] 21:22:54 INFO - GECKO(1042) | JavaScript > error: chrome://browser/content/tabbrowser.js, line 301: TypeError: > browser._createAutoScrollPopup is not a function Hmm, so that looks like DOMContentLoaded never ran before we unloaded... I guess that can make make us leak? (sounds like it shouldn't, if we never initialized in the first place...). Anyway, that gives me somewhere to look at... Seeing the stacks for these on automation would be super-helpful, btw. These are on Linux, so I guess I can try to hunt them down under rr or something...

Kris pointed out that browser._createAutoScrollPopup prevents a ton of initialization code from running, and that it's an XBL method... At this point I'm looking somewhat suspiciously at bug 1443948, though I'm not confident it'll be related... Anyway, will fire a few try runs with logging and try to figure out what's going on tomorrow morning.

FWIW, the errors around "this._menuObserver" don't seem to be related. Those errors happen before when the failure doesn't happen as well. This is probably because browser_private_browsing_window.js closes windows it just opens synchronously, and thus TabsInTitlebar is unloaded before it actually gets initialized. "browser._createAutoScrollPopup" is probably where the problem is, as it isn't shown up in logs from builds before. But this looks weird. This code is in _gBrowser._setupInitialBrowserAndTab() [1] which is called from _gBrowser.init(), [2] and the latter is invoked from gBrowserInit.onDOMContentLoaded() via renaming _gBrowser to gBrowser and calling gBrowser.init(). [3] gBrowserInit.onDOMContentLoaded() is invoked by a event handler of DOMContentLoaded. [4] _createAutoScrollPopup is called on the element with id "tabbrowser-initialBrowser" [5] which is a browser element in browser.xul. [6] And the browser element is supposed to have binding browser.xml#browser [7] which defines _createAutoScrollPopup. It means that this change makes us not apply the binding to the element when DOMContentLoaded is fired with the old style system? [1] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.js#301 [2] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.js#28 [3] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/browser.js#1191-1193 [4] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/browser.js#1177-1179 [5] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.js#289-292,295 [6] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/browser.xul#1228-1235 [7] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/toolkit/content/xul.css#176-178

I'd like to say that's impossible because we don't change the time DOMContentLoaded fires, nor the fact that we do layout (and thus construct frames, and thus attach XBL bindings) right before that. But you never know when you're talking about XBL, so will debug this properly tomorrow.

Hmmm... actually I can reproduce this on Windows! I don't know why it doesn't show up in treeherder, maybe just because we haven't tried to run it at all! I'll have a look at what's happening.

I know what's going on (sorry! I suck at sleeping)... I managed to catch this with RR. The reason is that the window closes before DOMContentLoaded, so we destroy the PresShell. Then when we fire DOMContentLoaded there's no presShell anymore, and thus no frames / no XBL bindings. Badness ensues. Stack coming. I'm moderately sure this is not related to my patch, other than changing the timing conditions... But worth fixing anyway, I guess.

This looks bad, because I'm pretty sure it can be hit without my patches.

This is super easy to catch with rr. Once you're in nsGlobalWindowInner::Dump you can step through GetComputedStyle and such, verify it has no binding and the document no shell, etc.

Comment on attachment 8959057 [details] Stack where we destroy the pres shell before DOMContentLoaded, (potentially) causing a browser window leak. (I haven't verified per se this leaks, but it has a lot of chances, per the comments above, and the fact that breaks gBrowser's initialization).

I guess we probably should just change the test to wait for the window to load... I don't think in general we want to close a window synchronously after we open it...

So if this is really what's causing the leak, it's pretty bad, and there's no other thing we can do than making the browser deal with it, somehow... Now, I'm not sure this is actually the reason we leak, of course...

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #103) > I guess we probably should just change the test to wait for the window to > load... I don't think in general we want to close a window synchronously > after we open it... In terms of landing this, sure... But this can probably be hit with DOM APIs from content, right? (window.open / window.close)

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #103) > I guess we probably should just change the test to wait for the window to > load... I don't think in general we want to close a window synchronously > after we open it... I agree that we don't want to do that, but I also agree with Emilio that we should handle it sensibly. DOMContentLoaded events are supposed to always reliably fire before unload. Adding caveats to that seems like a footgun. But just changing the test to wait for the window to load seems like a reasonable workaround in order to land these patches.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #105) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #103) > > I guess we probably should just change the test to wait for the window to > > load... I don't think in general we want to close a window synchronously > > after we open it... > > In terms of landing this, sure... But this can probably be hit with DOM APIs > from content, right? (window.open / window.close) Content window.open() and window.close() are different from opening a chrome window the way we do here. They return a reference to a content window, and only once the browser window is fully loaded. Calling close() on them dispatches an internal DOM event that the browser chrome handles specially to decide whether to close the window. The situation in the test is not really comparable.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #107) > Content window.open() and window.close() are different from opening a chrome > window the way we do here. They return a reference to a content window, and > only once the browser window is fully loaded. Calling close() on them > dispatches an internal DOM event that the browser chrome handles specially > to decide whether to close the window. > > The situation in the test is not really comparable. I see, that's really a relief.

So, this comment may be relevant: https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/dom/base/nsGlobalWindowOuter.cpp#6120 That check may be the one that fails making us destroying the presentation, and making the browser confused. Given: (rr) p this->mHavePendingClose $53 = true It may be fine to delay the closing so stuff happens in the proper order... Will push to try and actually go to sleep. If that doesn't work, I'll tweak the test for now and file a bug to figure out what should be going on here.

Hmm, actually that comment doesn't make that much sense to me, thinking a bit more about it...

In the sense of: mHavePendingClose is true because we set it as a guard for that method, but there's nothing guaranteeing (inside that method) that something will enter there again, I believe... Though maybe that's just fine?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #92) > But given these failures weren't there when my patch was backed out that > leaves: > > https://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=01685f397a53&tochange=80b4777a6421d8df4bb27ac23fb607c3 > 18a3006c This is not true. This can be in the range merged from autoland as well, so the suspect range should still be https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=deb7714a7bcd3448952440e92d0209abec6b886d&tochange=80b4777a6421d8df4bb27ac23fb607c318a3006c Actually, I'm suspecting bug 1443849 which landed in autoland at about the same time you land that code in m-i. This bug touches the initialization path pretty much. According to my local run, part 2 is where this issue starts showing.

OK, so the reason that part 2 causing this is because of this change: > - addEventListener("DOMContentLoaded", function() { > + document.addEventListener("DOMContentLoaded", function() { Originally we listen to DOMContentLoaded event on the window, and since when the window is closed, the event would not be propagated to it, so we are not calling gBrowserInit.onDOMContentLoaded() at all in that case. However, if we hook the listener on the document, the event would be fired there, and we start calling gBrowserInit.onDOMContentLoaded() where we didn't. Changing both listener to listen on window seems to fix this specific issue.

Actually, change DOMContentLoaded back is enough here, changing MozBeforeInitialXULLayout to listen on window isn't necessary. But maybe we can still do it for consistency.

try push with all the patches here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79e520fdc39f83befd969ba2c77890bb6ee3e7b1

When this bug first landed (comment 90), we noticed the following performance improvements. == Change summary for alert #12143 (as of Wed, 14 Mar 2018 17:48:44 GMT) == Improvements: 3% sessionrestore linux64 opt e10s stylo 293.96 -> 286.42 2% sessionrestore_no_auto_restore linux64 pgo e10s stylo322.58 -> 314.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12143

Comment on attachment 8959068 [details] [diff] [review] fixup to part 2 Review of attachment 8959068 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer this to be "window.addEventListener" rather than just "addEventListener" to match the other 12 'window.addEventListener' we have in the file.

Comment on attachment 8959068 [details] [diff] [review] fixup to part 2 > if (document.documentElement.getAttribute("windowtype") == "navigator:browser") { >- document.addEventListener("MozBeforeInitialXULLayout", function() { >+ addEventListener("MozBeforeInitialXULLayout", function() { Can you merge this into part 2 before landing? Bonus points for making this an arrow function (change 'function()' to '() =>').

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/610ba5dfaae1 Size the XUL window before doing layout. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee5a2d21901 Fire MozBeforeInitialXULLayout before sizing the window. r=florian,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e091577d2a Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/9c764daa1234 Update browser_windowopen_flicker.js. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/187aef60922c Flag chevron.svg as intermittently shown on windows. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/990a8eb972cd Fix extension windows. r=kmag

https://hg.mozilla.org/mozilla-central/rev/610ba5dfaae1 https://hg.mozilla.org/mozilla-central/rev/1ee5a2d21901 https://hg.mozilla.org/mozilla-central/rev/f8e091577d2a https://hg.mozilla.org/mozilla-central/rev/9c764daa1234 https://hg.mozilla.org/mozilla-central/rev/187aef60922c https://hg.mozilla.org/mozilla-central/rev/990a8eb972cd

mozregression points me to this bug. My issue with latest firefox (61.0a1 - 16/03/2018) is that my mouse stop work, and I can't click anywhere or scroll or right click - nothing. I can switch only fist 2-3 tabs )depends how many tabs I have opened, but then again mouse not work. I can scroll page with keyboard though.

(In reply to drJeckyll from comment #122) > mozregression points me to this bug. My issue with latest firefox (61.0a1 - > 16/03/2018) is that my mouse stop work, and I can't click anywhere or scroll > or right click - nothing. I can switch only fist 2-3 tabs )depends how many > tabs I have opened, but then again mouse not work. I can scroll page with > keyboard though. Are you on Linux? It sounds pretty similar to bug 1446242.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #123) > (In reply to drJeckyll from comment #122) > > mozregression points me to this bug. My issue with latest firefox (61.0a1 - > > 16/03/2018) is that my mouse stop work, and I can't click anywhere or scroll > > or right click - nothing. I can switch only fist 2-3 tabs )depends how many > > tabs I have opened, but then again mouse not work. I can scroll page with > > keyboard though. > > Are you on Linux? It sounds pretty similar to bug 1446242. Yes, Gentoo Linux. Sorry for not adding that info in my previous comment.

Sherrifs have been asked to back this out and respin Nightlies.

Backed out 6 changesets (bug 1439875) for massive breakage for nightly users a=backout Backout link: https://hg.mozilla.org/mozilla-central/rev/c43177719f368d4316d244cac5382f8cfce3832f

So, FWIW I was using local builds for a week+ on Linux with these patches for regular browsing and everything works fine. But apparently this only affects X11 (and I'm using Wayland). Sadness. Sorry for the breakage everybody :(.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2b5cb5120b Size the XUL window before doing layout. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/482afec9e81f Fire MozBeforeInitialXULLayout before sizing the window. r=florian,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/244ea0dbcc70 Update browser_windowopen_reflows.js to not wait for a resize that no longer exists. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/6095f1c83e38 Update browser_windowopen_flicker.js. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd4c2b95f30 Flag chevron.svg as intermittently shown on windows. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/e117b94b5235 Fix extension windows. r=kmag

https://hg.mozilla.org/mozilla-central/rev/aa2b5cb5120b https://hg.mozilla.org/mozilla-central/rev/482afec9e81f https://hg.mozilla.org/mozilla-central/rev/244ea0dbcc70 https://hg.mozilla.org/mozilla-central/rev/6095f1c83e38 https://hg.mozilla.org/mozilla-central/rev/dbd4c2b95f30 https://hg.mozilla.org/mozilla-central/rev/e117b94b5235

Testing in todays' Nightly cset: https://hg.mozilla.org/mozilla-central/rev/97160a734959 on Win10 x64 The issue with opening in 'normal' mode is NOT fixed...I can still reproduce. Opening new window with Ctrl+N or Private window with Ctrl+Shift+P opens in 'Normal' not maximized.

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #130) > Testing in todays' Nightly cset: > https://hg.mozilla.org/mozilla-central/rev/97160a734959 on Win10 x64 > > The issue with opening in 'normal' mode is NOT fixed...I can still reproduce. > > Opening new window with Ctrl+N or Private window with Ctrl+Shift+P opens in > 'Normal' not maximized. Let's track this in bug 1446264.

I thought the reason why we needed to make changes to the flicker tests was that we were now showing the browser window before its load event (and so the flicker tests were capturing screenshots of the window with some SVG icons not loaded yet). When looking at a startup profile (https://perfht.ml/2IzZaFS), this is not what I see, the first paint is still after the 'load' event. However, opening a new window (https://perfht.ml/2Iudek2) seems to paint between the 'DOMContentLoaded' and the 'load' events (or is it just the refresh driver tick causing a paint in the previous window?). Maybe this is the reason why you had to add new exceptions to the browser_windowopen_flicker test but not the browser_startup_flicker one. Is there room for improvements here?

When this landed in comment 121, we noticed the following perf improvements. The relanding after the backout, brought them but with lower values. == Change summary for alert #12168 (as of Thu, 15 Mar 2018 08:25:13 GMT) == Improvements: 7% tpaint windows10-64 pgo e10s stylo 216.58 -> 201.83 7% tpaint windows10-64 opt e10s stylo 257.83 -> 241.00 4% sessionrestore windows10-64 pgo e10s stylo375.00 -> 359.58 4% sessionrestore_no_auto_restore windows10-64 pgo e10s stylo443.46 -> 426.50 4% sessionrestore windows10-64 opt e10s stylo433.92 -> 418.75 3% ts_paint windows10-64 pgo e10s stylo 489.29 -> 473.25 3% ts_paint_webext windows10-64 opt e10s stylo637.62 -> 617.83 3% ts_paint windows10-64 opt e10s stylo 560.50 -> 544.17 3% ts_paint linux64 pgo e10s stylo 439.83 -> 427.75 2% sessionrestore linux64 pgo e10s stylo 277.00 -> 270.27 2% sessionrestore_many_windows windows10-64 pgo e10s stylo1,315.79 -> 1,285.08 2% ts_paint_heavy linux64 pgo e10s stylo 441.38 -> 431.50 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12168

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1438775#c17 I'm going to mark this wontfix for 60.

We probably should partially backout this (i.e. disable the new code path introduced here rather than a complete backout) to give more time for bug 1446264. That's much more involved than it appears to be.

That sounds ok to me, I can write the patch tomorrow. I'm ambivalent about what to do wrt automation though. On one side, we want to test this patch on automation so that people don't break it. On the other, we want to test what we release to users... So we can do three things, roughly: #if 0 SyncAttributesToWidget(); SizeShell(); #endif That means that we loose all test coverage for this patch. if (xpc::IsInAutomation()) { SyncAttributesToWidget(); SizeShell(); } That means that we publish something that is totally not what we're testing on our automation. We could do something intermediate like: #ifdef DEBUG SyncAttributesToWidget(); SizeShell(); #endif But that means that we need to keep test expectations both before and after this patch, which looks like a pain. Any strong opinion? I'd like to try (3), and (1) if it fails, not testing what we ship doesn't look great IMO, but I could try to be convinced of doing it.

I guess we can pref this and make the perf tests run with this pref, sounds fine?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #137) > I guess we can pref this and make the perf tests run with this pref, sounds > fine? Risk there is that Talos no longer measures the bits we ship, and in this case the measurements will differ significantly. Not a hard blocker, but not really ideal. Is this really at risk of somebody breaking it over the course of a few days?

Yeah, probably not. It took a lot to land, but... I guess if it regresses as long as we have a regression range fixing it is way easier.

Maybe still worth preffing it to be able to test it easily?

It actually looks green. Ok, let's back out. I'll file a new bug to keep track of this more easily.

Yeah, pref seems like a good idea.