1439875 - stylo: Size the XUL window before starting layout.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1420423 comment 63 and following.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Size the XUL window before doing layout.

The only subtle thing is the mCenterAfterLoad stuff, which is gated after a
mChromeLoaded. Other than that it follows the same pattern as bug 345560.

Review commit: https://reviewboard.mozilla.org/r/221918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221918/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

This is guaranteed not to run script because of:

  https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/layout/base/nsPresContext.cpp#2152

This doesn't have any effect on the top-level chrome window, but avoids a full
document restyle on XUL popups on startup.

With this patch, there are no more full document restyles due to media feature
changes on my machine on startup.

Review commit: https://reviewboard.mozilla.org/r/221920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221920/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952689 [details]
Bug 1439875: Size the XUL window before doing layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221918/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952690 [details]
Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221920/diff/1-2/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(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 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Jet Villegas (inactive)]

[ Triage 2018/02/21: P3 ]

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 12

[Florian Quèze [:florian]]

(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 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Fix extension windows.

Now we're loading the sizemode attribute earlier, doing this on load stops
working.

Review commit: https://reviewboard.mozilla.org/r/222338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222338/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window.

This gives the chance to code that relies on setting the XUL window attributes
to run before we actually size the window.

This should prevent the resizing on OSX and fix some other untested stuff that
the first commit probably broke...

Review commit: https://reviewboard.mozilla.org/r/222380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222380/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952689 [details]
Bug 1439875: Size the XUL window before doing layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221918/diff/2-3/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952690 [details]
Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221920/diff/2-3/

Comment 17

[Florian Quèze [:florian]]

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 18

[Olli Pettay [:smaug][[e-mail]]]

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 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 20

[Kris Maglione [:kmag]]

Comment on attachment 8953096 [details]
Bug 1439875: Fix extension windows.

https://reviewboard.mozilla.org/r/222338/#review228408

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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).

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Emilio Cobos Álvarez (:emilio)]

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

Comment 25

[Dão Gottwald [:dao]]

(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.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

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
           }
         }
       ];

Comment 28

[:Gijs (he/him)]

(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 29

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Update browser_windowopen_flicker.js.

To avoid hacking around the empty window, and handle the activation issue seen
on OSX / Windows.

Also add exceptions for the loading of icons in the tab strip and the titlebar.

Review commit: https://reviewboard.mozilla.org/r/223130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223130/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952689 [details]
Bug 1439875: Size the XUL window before doing layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221918/diff/3-4/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953096 [details]
Bug 1439875: Fix extension windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222338/diff/1-2/

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953097 [details]
Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222380/diff/1-2/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952690 [details]
Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221920/diff/3-4/

Comment 34

[Florian Quèze [:florian]]

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 35

[Florian Quèze [:florian]]

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! :-)

Comment 36

[Emilio Cobos Álvarez (:emilio)]

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 37

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953980 [details]
Bug 1439875: Update browser_windowopen_flicker.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223130/diff/1-2/

Comment 38

[Code Review Bot [:reviewbot]]

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]

Comment 39

[Florian Quèze [:florian]]

(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).

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

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 41

[Florian Quèze [:florian]]

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.

Comment 42

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 44

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 48

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... but that patch itself is probably triggering another set of failures :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6008ded5034eb92ed0951bc196b788da236a2f13

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

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 :/

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 51

[Xidorn Quan [:xidorn] UTC+10]

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...)

Comment 52

[Jim Mathies [:jimm]]

(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

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 54

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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 55

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952689 [details]
Bug 1439875: Size the XUL window before doing layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221918/diff/4-5/

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953097 [details]
Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222380/diff/2-3/

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952690 [details]
Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221920/diff/4-5/

Comment 58

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953980 [details]
Bug 1439875: Update browser_windowopen_flicker.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223130/diff/2-3/

Comment 59

[Emilio Cobos Álvarez (:emilio)]

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 60

[Emilio Cobos Álvarez (:emilio)]

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 61

[Emilio Cobos Álvarez (:emilio)]

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 62

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953980 [details]
Bug 1439875: Update browser_windowopen_flicker.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223130/diff/3-4/

Comment 63

[Emilio Cobos Álvarez (:emilio)]

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 64

[Florian Quèze [:florian]]

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)).

Comment 65

[Florian Quèze [:florian]]

(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.

Comment 66

[Emilio Cobos Álvarez (:emilio)]

(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 :)

Comment 67

[Emilio Cobos Álvarez (:emilio)]

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

Comment 68

[Emilio Cobos Álvarez (:emilio)]

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 69

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439875: Flag chevron.svg as intermittently not loaded on windows.

Review commit: https://reviewboard.mozilla.org/r/227386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227386/

Comment 70

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952689 [details]
Bug 1439875: Size the XUL window before doing layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221918/diff/5-6/

Comment 71

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953097 [details]
Bug 1439875: Fire MozBeforeInitialXULLayout before sizing the window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222380/diff/3-4/

Comment 72

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952690 [details]
Bug 1439875: Update browser_windowopen_reflows.js to not wait for a resize that no longer exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221920/diff/5-6/

Comment 73

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953980 [details]
Bug 1439875: Update browser_windowopen_flicker.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223130/diff/4-5/

Comment 74

[Florian Quèze [:florian]]

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 75

[Florian Quèze [:florian]]

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 76

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953980 [details]
Bug 1439875: Update browser_windowopen_flicker.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223130/diff/5-6/

Comment 77

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8958431 [details]
Bug 1439875: Flag chevron.svg as intermittently not loaded on windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227386/diff/1-2/

Comment 78

[Johann Hofmann [:johannh]]

Comment on attachment 8958431 [details]
Bug 1439875: Flag chevron.svg as intermittently not loaded on windows.

https://reviewboard.mozilla.org/r/227386/#review233202

Comment 79

[Pulsebot]

Pushed by [e-mail]:
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

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21bdb9b67e8
Fix extension windows. r=kmag

Comment 81

[Narcis Beleuzu [:NarcisB]]

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

Comment 82

[Emilio Cobos Álvarez (:emilio)]

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

Comment 83

[Pulsebot]

Backout by [e-mail]:
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

Comment 84

[Emilio Cobos Álvarez (:emilio)]

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

Comment 85

[Emilio Cobos Álvarez (:emilio)]

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

Comment 86

[Bobby Holley (:bholley)]

Bug 1444588 should suffice here.

Comment 87

[Narcis Beleuzu [:NarcisB]]

: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

Comment 88

[Emilio Cobos Álvarez (:emilio)]

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

Comment 89

[Xidorn Quan [:xidorn] UTC+10]

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 :/

Comment 90

[Pulsebot]

Pushed by [e-mail]:
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

Comment 91

[Andreea Pavel [:apavel]]

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

Comment 92

[Emilio Cobos Álvarez (:emilio)]

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

Comment 93

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 94

[Emilio Cobos Álvarez (:emilio)]

(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...

Comment 95

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 96

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 97

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 98

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 99

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 100

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack where we destroy the pres shell before DOMContentLoaded, (potentially) causing a browser window leak.

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

Comment 101

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch used for debugging, as crappy as one might expect, for reference.

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 102

[Emilio Cobos Álvarez (:emilio)]

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).

Comment 103

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 104

[Emilio Cobos Álvarez (:emilio)]

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...

Comment 105

[Emilio Cobos Álvarez (:emilio)]

(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)

Comment 106

[Kris Maglione [:kmag]]

(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.

Comment 107

[Kris Maglione [:kmag]]

(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.

Comment 108

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 109

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 110

[Emilio Cobos Álvarez (:emilio)]

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

Comment 111

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 112

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 113

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 114

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 115

[Xidorn Quan [:xidorn] UTC+10]

Attachment: fixup to part 2

Comment 116

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 117

[Ionuț Goldan [:igoldan]]

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 118

[Florian Quèze [:florian]]

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 119

[Dão Gottwald [:dao]]

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 '() =>').

Comment 120

[Pulsebot]

Pushed by [e-mail]:
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

Comment 121

[Noemi Erli[:noemi_erli]]

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

Comment 122

[drJeckyll]

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.

Comment 123

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 124

[drJeckyll]

(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.

Comment 125

[Gian-Carlo Pascutto [:gcp]]

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

Comment 126

[Raul Gurzau (:RaulG)]

Backed out 6 changesets (bug 1439875) for massive breakage for nightly users a=backout

Backout link: https://hg.mozilla.org/mozilla-central/rev/c43177719f368d4316d244cac5382f8cfce3832f

Comment 127

[Emilio Cobos Álvarez (:emilio)]

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 :(.

Comment 128

[Pulsebot]

Pushed by [e-mail]:
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

Comment 129

[Andrei Ciure[:aciure]]

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

Comment 130

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 131

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 132

[Florian Quèze [:florian]]

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?

Comment 133

[Ionuț Goldan [:igoldan]]

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

Comment 134

[Julien Cristau [:jcristau]]

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

Comment 135

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 136

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 137

[Emilio Cobos Álvarez (:emilio)]

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

Comment 138

[Bobby Holley (:bholley)]

(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?

Comment 139

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 140

[Emilio Cobos Álvarez (:emilio)]

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

Comment 141

[Emilio Cobos Álvarez (:emilio)]

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

Comment 142

[Bobby Holley (:bholley)]

Yeah, pref seems like a good idea.