1444045 - TEST-UNEXPECTED-FAIL | [snip]/mozmill/session-store/test-session-store.js | test-session-store.js::test_message_pane_height_persistence

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1520492215/build/tests/mozmill/session-store/test-session-store.js | test-session-store.js::test_message_pane_height_persistence

First see on try Thu, Mar 8, 2018, 00:04:07

M-C last good: cb3b2b090314e6b1c4bc0bc7ebe7814ba4
M-C first bad: a859a4b2257e6c61f7c2aab456cf551df8
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb3b2b090314e6b1c4bc0bc7ebe7814ba4&tochange=a859a4b2257e6c61f7c2aab456cf551df8

Log
https://taskcluster-artifacts.net/JGCqjj4EQxix-4_v8S1Lkg/0/public/logs/live_backing.log
says:
INFO -  SUMMARY-UNEXPECTED-FAIL | test-session-store.js | test-session-store.js::test_message_pane_height_persistence
INFO -    EXCEPTION: The message pane height should be 248, but is actually 259. The oldHeight was: 396: '248' != '259'.
INFO -      at: test-folder-display-helpers.js line 2913
INFO -         assert_true test-folder-display-helpers.js:2913 11
INFO -         assert_equals test-folder-display-helpers.js:2900 3
INFO -         test_message_pane_height_persistence test-session-store.js:239 3

Looks like it comes from this:
7d40106bafeb Xidorn Quan — Bug 1442521 - Make width/height attr on <window> mean the inner size in nsXULWindow. r=bz

Has that change affected the persistence? Or do we need to adapt our test?

Oh, this only seems to affect Mac:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=47b4430bdfb677b17ee358288b3500ee1be85e30&selectedJob=166704429
which makes it even more puzzling. I checked, the test runs on all platforms.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It changes the persistence of <window>, to make it mean the inner window size rather than the outer size.

Comment 2

[Jorg K (CEST = GMT+2)]

Thanks, but that doesn't really explain the test failure on Mac only. If the test were now wrong, it would fail on all platforms:
https://dxr.mozilla.org/comm-central/rev/0d0988b6c3a1f2aca51d03c7d884ed7f17577e6f/mail/test/mozmill/session-store/test-session-store.js#239

Also, further down we test width, and that test doesn't fail, log says:
INFO -  SUMMARY-PASS | test-session-store.js::test_message_pane_width_persistence

Comment 3

[Boris Zbarsky [:bzbarsky]]

So here is what the test is doing:

  let oldHeight = mc.e("messagepaneboxwrapper").boxObject.height;
  let minHeight = Math.floor(mc.e("messagepaneboxwrapper").getAttribute("minheight"));
  let newHeight = Math.floor((minHeight + oldHeight) / 2);
  let diffHeight = oldHeight - newHeight;
  _move_splitter(mc.e("threadpane-splitter"), 0, diffHeight);
  let actualHeight = mc.e("messagepaneboxwrapper").boxObject.height;

  assert_equals(newHeight, actualHeight,
    "The message pane height should be " + newHeight + ", but is actually " +
    actualHeight + ". The oldHeight was: " + oldHeight);

so far so good.  This part passes.  Then we close the window.  Then we reopen it.  Then we do:

  actualHeight = mc.e("messagepaneboxwrapper").boxObject.height;

  assert_equals(newHeight, actualHeight,
    "The message pane height should be " + newHeight + ", but is actually " +
    actualHeight + ". The oldHeight was: " + oldHeight);

and this is the assert that fails.  So clearly the message pane height is not being restored correctly.

Why this is Mac, specific I can't tell you.  But I can tell you why I suspect width passes: Mac doesn't have any window decorations on left and right by default, so the inner and outer widths are the same.

I think to make progress here we need to figure out what code is actually responsible for restoring the message pane height.  Is it this part:

                <hbox id="messagepaneboxwrapper"
                      flex="1"
                      minheight="100" height="200"
                      minwidth="100" width="200" persist="collapsed height width">

and specifically the persist="collapsed height width" bit?

It would be interesting to look at the boxobject height of the "messengerWindow" before and after the close/reopen.  Because maybe we're not restoring _that_ correctly and the rest follows?

Comment 4

[Boris Zbarsky [:bzbarsky]]

(That quoted bits of XUL comes from https://searchfox.org/comm-central/source/mail/base/content/messenger.xul of course.)

Comment 5

[Jorg K (CEST = GMT+2)]

Houston, we have a problem :-( - We don't have any Mac developer who could try this. Richard has a decent Mac, but he doesn't build and run tests, and the rest of the people have Linux or Windows.

Looking at Boris' question, the "messengerWindow"
https://dxr.mozilla.org/comm-central/rev/0d0988b6c3a1f2aca51d03c7d884ed7f17577e6f/mail/base/content/messageWindow.xul#29
is for single messages. The message pane is part of the 3pane window which has folders, thread pane (message list) and message (preview) pane.

I wanted to see visually what the test does, so first I ran the entire test, which passed, then I disabled all other subtests and just ran the test in question. Lo and behold, it failed on Windows:
EXCEPTION: The message pane height should be 220, but is actually 249. The oldHeight was: 341: '220' != '249'.

Boris, I assume you want to know whether the 3pane window is restored with it's initial size?

Anyway, although I'm confused by the "messengerWindow", I modified the test:
  // The 3pane window is closed.
  dump(mc.e("messengerWindow").boxObject.height+"=============before close\n");
  close_window(new mozmill.controller.MozMillController(mail3PaneWindow));
  // Wait for window close async session write to finish.
  controller.sleep(asyncFileWriteDelayMS);

  mc = open3PaneWindow();
  be_in_folder(folderA);
  assert_message_pane_visible();

  actualHeight = mc.e("messagepaneboxwrapper").boxObject.height;
  dump(mc.e("messengerWindow").boxObject.height+"=============after close\n");

  assert_equals(newHeight, actualHeight,
    "The message pane height should be " + newHeight + ", but is actually " +
    actualHeight + ". The oldHeight was: " + oldHeight);

and got:
762=============before close
762=============after close

Aceman, can you also please take a look. Disable all subtests except test_message_pane_height_persistence and see what it does on Linux.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Looking at Boris' question, the "messengerWindow"

I'm talking about https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#44 to be clear.

> then I disabled all other subtests and just ran the test in question. Lo and behold, it failed on Windows

Oh, interesting.  Does that still happen without the patch for bug 1442521?

> Boris, I assume you want to know whether the 3pane window is restored with it's initial size?

Yep.

>762=============before close
>762=============after close

OK.  For now I'm out of ideas as to what's going on here...

Comment 7

[Jorg K (CEST = GMT+2)]

Oh, the stand-alone window (messageWindow.xul) and the 3pane window (messenger.xul) have the same ID. OK, then my test was right and I dumped out the correct heights. I'll do a backout of bug 1442521 and report back.

Comment 8

[Jorg K (CEST = GMT+2)]

The single test still fails with rev 7d40106bafeb from bug 1442521 backed out :-(

I'm also out of ideas, or better, I never had one to start with. This is very weird, single test fails, if some others run before, it passes, but recently not on Mac any more. This looks like a case for Aceman.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1444045-disable-tests-but-one.patch

What happens when you run with this patch on Linux. Does the test fail?

Comment 10

[Jorg K (CEST = GMT+2)]

OK, instead of running tests, I actually tested the function.

- Open a folder with lots of messages
- slide the slider to after the 4th message, so the space below is preview pane
- open the address book
- close the main window
- open the main window (Tools > Options on the address book window),
  result: the slider is where is was.
- close TB and restart it,
  result: the slider is where is was.

So regardless of what the test says, this still seems to work, at least on Windows.

Also, looking at the test again there are beauties like:
  // FIXME: For whatever reasons the new width is off by one pixel on Mac OSX
  // FIXME: For whatever reasons the new width is off by two pixels on Mac OSX
Well, now we see 11px difference on Mac according to the test and 29px in Windows, which I don't see in manual testing.

Richard, could you try the STR above on Mac an see whether the message pane slider stays in the same position on Mac after closing and re-opening the main window.

Comment 11

[Richard Marti (:Paenglab)]

Tried with today Daily. The window grows at every opening for 22px (the titlebar height).

Comment 12

[Jorg K (CEST = GMT+2)]

So Boris, regardless of the strange effects we see, your suspicion seems to have been correct that the overall window grows on Mac. So where from here?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> our suspicion seems to have been correct that the overall window grows on Mac.

It does?  Where do we see that?  Comment 5 had the same before/after height...

Comment 14

[Jorg K (CEST = GMT+2)]

That was on Windows where where the test passed before I started messing with this.

Boris, I know your time is precious, but if you can spare 3.5 minutes, download a Daily for Mac from:
http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/thunderbird-60.0a1.en-US.mac.dmg
and do the steps in comment #10. Or maybe opening the app a few times will already show a problem.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

If it's Mac-only, is it possible that somewhere has a Mac-specific hack to increase the window size to take titlebar into consider?

Comment 16

[Jorg K (CEST = GMT+2)]

What about FF on Mac, does that grow?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

No, as far as I can see, no window grows when open multiple times.

Comment 18

[:aceman]

Comment on attachment 8957317 [details] [diff] [review]
1444045-disable-tests-but-one.patch

With the patch it fails for me too on Linux:
EXCEPTION: The message pane height should be 222, but is actually 251. The oldHeight was: 344: '222' != '251'

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #18)
> With the patch it fails for me too on Linux:
> EXCEPTION: The message pane height should be 222, but is actually 251. The
> oldHeight was: 344: '222' != '251'
Hmm, 29px more than to start with like in Windows.

Anyway, let's get back to the Mac issue. There is a lot of special casing in
https://dxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js
for "macosx" so perhaps some of this special processing is not working any more.

Comment 20

[Jorg K (CEST = GMT+2)]

Richard, could you experiment a bit to see where the "window growth" on Mac comes from, maybe from the special case Mac code in msgMail3PaneWindow.js. Since you don't do debug builds, I suggest to add Services.console.logStringMessage("xxx") for debugging to check what's happening.

Comment 21

[Richard Marti (:Paenglab)]

I first checked if this still happens with drawInTitlebar disabled and it stopped to grow. With bug 1442521 backed out it also stopped to grow. Then I disabled all special Mac treatment in msgMail3PaneWindow.js for drawInTitlebar but it still grows.

Philipp, you know Mac very good. Please could you look into this problem and help to fix it?

Comment 22

[Boris Zbarsky [:bzbarsky]]

So one plausible hypothesis is that GetWindowOuterInnerDiff() is ending up with the wrong value on Mac when drawInTitlebar is true.  Most simply this could happen if the code in nsXULWindow::OnChromeLoaded runs before the code in TabsInTitlebar._update that applies the pref?

In any case, Xidorn, it seems like the new setup really does not play well with _changing_ the pref, or indeed anything about the size of the window chrome (like OS theme).  In particular, if a window's dimensions are persisted when the pref has one value, then restored when it has a different value, the inner size will stay but the outer will change.  Then if the pref is changed the outer size stays but the inner size changes, etc.

Or put another way, say the OS provides two themes, one with 10px titlebar and one with a 20px one.  The user does the following:

1)  Change to 10px theme.
2)  Start Firefox.
3)  Change to 20px theme.
4)  Quit Firefox.

Every time this is done, the size of the Firefox window will grow by 10px, right?  That seems pretty unexpected to me, and makes me question the premise of bug 1442521, unless we switch to the attrs meaning inner size but the persisted values being outer size or something.

Comment 23

[Jorg K (CEST = GMT+2)]

How about backing out bug 1442521 for now and deal with it in mozilla61. To me it looks like the sort of thing that shouldn't have landed in the "soft freeze period" since it's seems to have some unwanted and yet unfathomed consequences.

Comment 24

[Boris Zbarsky [:bzbarsky]]

That bug is blocking stylo-chrome performance work that is aiming to land in 60.  So just backing it out it not necessarily the right thing.

Comment 25

[Jorg K (CEST = GMT+2)]

OK, fair enough. The question for me is whether you will fix this in M-C or whether we need to find some workaround in C-C. Comment #60 suggests that FF might also be affected. IMHO, this bug should go to Core::XUL and have FF60 tracking.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I spun off comment 22 into bug 1444525.  There's a good chance that fixing it will fix this bug, but no guarantee, so we should keep this bug open to track the Thunderbird problem.

Comment 27

[Philipp Kewisch [:Fallen][☁️📆]]

Is my input still needed here? It looks like the underlying issue was found.

Comment 28

[Richard Marti (:Paenglab)]

 I think we can wait until the other bug lands to see if we still have this issue.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1444045-disable-test-on-mac.patch

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bc5f14d45c1e
temporarily disable test-session-store.js::test_message_pane_height_persistence. rs=bustage-fix

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

My patch in bug 1445519 may be enough for fixing this bug. Would you like to have a try?

Comment 32

[Jorg K (CEST = GMT+2)]

Thanks, here we go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad67248b526f526fd86c0ac5a43ebeb31ab0878c

Comment 33

[Jorg K (CEST = GMT+2)]

No idea what happened here, usually the Z2 takes 10 minutes, and now it's not finished after 75 minutes. Someone triggered it again?

Comment 34

[Jorg K (CEST = GMT+2)]

OK, the second one finished, but the test still failed.

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jorg K (GMT+1) from comment #33)
> No idea what happened here, usually the Z2 takes 10 minutes, and now it's
> not finished after 75 minutes. Someone triggered it again?

Yeah, I triggered it again, and it seems to take 10min now. The old one probably just got stuck somehow.

So it seems this bug is not fixed by that patch, given the result.

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jorg K (GMT+1) from comment #34)
> OK, the second one finished, but the test still failed.

But... I suspect whether the patch is really applied properly there.

I downloaded target.zip from that task, and disassemble the function nsCocoaWindow::ClientToWindowSize, from there I see something like:
> XUL[0x2598d86] <+118>: movq   0x36edd2b(%rip), %rdx     ; "frameRectForContentRect:"
while from my local build, I can see these two lines:
> XUL[0x317b048] <+88>:  movq   0x4bb6ac1(%rip), %rsi     ; "styleMask"
> XUL[0x317b096] <+166>: movq   0x4bb794b(%rip), %rdx     ; "frameRectForContentRect:styleMask:"

That indicates that the XUL library there probably isn't patched at all.

Comment 37

[Jorg K (CEST = GMT+2)]

Hmm, I followed the standard process to add M-C patches to a C-C try push:
https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer#Pushing_mozilla-central_patches
However, in the log
https://taskcluster-artifacts.net/USHXZqy-QwOqv9lw2HfSLg/0/public/logs/live_backing.log
I see no evidence of the patch being applied. Strange.

Now all we can do is to get a TB person with a Mac. Richard, can you try this for us and repeat the steps you mentioned in comment #11.

Comment 38

[Richard Marti (:Paenglab)]

Tested the patch from bug 1445519 and it fixes the issue. Many thanks Xidorn.

Comment 39

[Jorg K (CEST = GMT+2)]

Hmm, OK, trying this again, this time with a buildbot try, I hope:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e606fa7d8689cfa88375fa43ce0a770e24d34790

Comment 40

[Jorg K (CEST = GMT+2)]

Test passed on Buildbot now. So perhaps the "add M-C patches to the C-C push" doesn't work on TaskCluster. Thanks for the fix and sorry about the confusion :-(

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e4ee39d1cd0
Backed out changeset bc5f14d45c1e to re-enable test. a=backout

Comment 42

[Jorg K (CEST = GMT+2)]

Fixed by bug 1445519.