1533085 - Massive MozMill failure on 2019-03-06: Mostly Calendar but also some mail tests - Toolbar button menus (incl. app/Hamburger menu) not working

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

The Calendar tests all fail in setupModule. Here are some other failures:

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893844/build/tests/mozmill/folder-display/test-mail-views.js | test-mail-views.js::test_save_view_as_folder
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893844/build/tests/mozmill/folder-display/test-mail-views.js | test-mail-views.js::test_verify_saved_mail_view
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893844/build/tests/mozmill/folder-display/test-watch-ignore-thread.js | test-watch-ignore-thread.js::test_view_threads_ignored_threads
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893844/build/tests/mozmill/folder-display/test-watch-ignore-thread.js | test-watch-ignore-thread.js::test_watch_thread

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893839/build/tests/mozmill/folder-pane/test-folder-pane-consumers.js | test-folder-pane-consumers.js::test_virtual_folder_selection_tree
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893839/build/tests/mozmill/folder-pane/test-folder-pane-consumers.js | test-folder-pane-consumers.js::test_offline_sync_folder_selection_tree

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893839/build/tests/mozmill/folder-widget/test-message-filters.js | test-message-filters.js::test_address_books_appear_in_message_filter_dropdown
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893839/build/tests/mozmill/folder-widget/test-message-filters.js | test-message-filters.js::test_can_cancel_quit_on_filter_changes
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893839/build/tests/mozmill/folder-widget/test-message-filters.js | test-message-filters.js::test_can_quit_on_filter_changes

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_go_search
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_open_single_search_result_in_tab
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_open_multiple_search_results_in_new_tabs
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_open_search_result_in_new_window
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_open_search_result_in_existing_window

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e0cf2f77f07&tochange=4ab143dde4dc

This one looks suspicious:
5edf9199407c Emilio Cobos Álvarez — Bug 1450652 - Remove platform support for display="" in XBL bindings. r=mats

Comment 1

[Jorg K (CEST = GMT+2)]

Calendar fails mostly with
EXCEPTION: calendarTree.mCalendarList is undefined

test-search-window.js has things like:
EXCEPTION: view does not contain a header that should be present! [msgHdr mailbox://nobody@Local%20Folders/SearchWindowA#3]
EXCEPTION: Thought we would find row 1 at 112,30 but we found -1

and test-message-filters.js has:
EXCEPTION: fec.window.document.getAnonymousNodes(...) is null

Hmm, is that de-XBL?

Related to the bug quote above? We have some of that stuff:
https://searchfox.org/comm-central/search?q=display%3D&case=false&regexp=false&path=

Comment 2

[Richard Marti (:Paenglab)]

TB itself looks good but Calendar is broken. Bug 1450652 seems to be the culprit.

Confirmed by local backout of rev https://hg.mozilla.org/mozilla-central/rev/5edf9199407c

Comment 3

[Jorg K (CEST = GMT+2)]

Emilio, is there any replacement?

Comment 4

[Jorg K (CEST = GMT+2)]

Two more mail failures I missed in comment #0:
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893844/build/tests/mozmill/content-tabs/test-addons-mgr.js | test-addons-mgr.js::test_addon_prefs
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1551893853/build/tests/mozmill/search-window/test-search-window.js | test-search-window.js::test_verify_saved_search

So 16 in total, not counting calendar.

Comment 5

[Jorg K (CEST = GMT+2)]

Switching off Calendar + 16 tests, let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b12043a61d891892226b95017a1533f903e89e2c

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Not a general replacement, no, depends generally on a case by case basis. There are two things you need to remove, which do the same thing:

• extends="xul:foo"
• display="xul:foo"

Each of them need to go away.

Depending on what do any of those do the fix may be different. For example, there's:

https://searchfox.org/comm-central/rev/5bb6edede85343d0fb60d81597af95e1ee284576/calendar/base/content/calendar-daypicker.xml#16

Which applies to all the <daypicker> elements. That gives them a xul:button box. Depending on why they need the XUL button box, course of action could be:

• Nothing, they don't need the button box.
• Some specific fix, like using pointer-events: none on the children or such.
• Actually they need a button box, in which case we need to add an entry like:

https://searchfox.org/comm-central/rev/5bb6edede85343d0fb60d81597af95e1ee284576/mozilla/layout/base/nsCSSFrameConstructor.cpp#3963

That's just an example. Sorry, should've checked comm-central, I assumed usage was not very high, apologies.

I can partially back out that patch so you can have some time to adjust, if you wanted to. I'm also happy to help with the removal.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

You can generally see some examples of how to fix this in bug 1451256 and more generally in all the dependent bugs from bug 1450652.

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, my try has more test failures:

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/folder-widget/test-message-filters.js | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_override
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_noPreview
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_manual_attachment_reminder
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_manual_automatic_attachment_reminder_interaction
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_disabled_attachment_reminder
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_reminder_in_draft
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_disabling_attachment_reminder

:-(

Comment 9

[Jorg K (CEST = GMT+2)]

OK, switching off Calendar and 24 tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a28d3a0a3f53d28e27f9792f5982ce316f4ef7d9

Comment 10

[Jorg K (CEST = GMT+2)]

mozmake mozmill-one SOLO_TEST=composition/test-reply-multipart-charset.js passes locally as does mozmake mozmill-one SOLO_TEST=composition/test-attachment-reminder.js. Strange.

So maybe a fallout from the filter test. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a03328bc5eaf767f395d55c659cbad39abb6ce87

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5e366aafa2d5
Remove extends="xul:" and display="xul:" in XBL bindings; rs=bustage-fix

Comment 12

[Geoff Lankow (:darktrojan)]

Removing the extends="xul:…" fixes most of the problems and I haven't seen any side effects yet. display="xul:…" is a different matter. Toolbar buttons with menus (including the appmenu) act as though they don't have menus. This is the cause of the remaining failing mozmill tests.

Comment 13

[Jorg K (CEST = GMT+2)]

I don't think we're finished here :-(

Comment 14

[Geoff Lankow (:darktrojan)]

Oh, I didn't notice the auto-close, wasn't intentional.

I don't think we're finished here :-(

I am, in case anyone wants to assume otherwise.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Disable six tests

OK, let's disable six tests that fail on all platforms unless disabled already (one already disabled on Mac). Totally disconcerting that MozMill's "Z1" fails with
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
on all platforms.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c44202622511
temporarily disable 6 tests. rs=bustage-fix

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Needed to switch off 7 more

Looks like comment #10 was wrong and these fail after all.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6210f90e0832
temporarily 7 tests. rs=bustage-fix

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f0bde910681
Follow-up: remove blank lines at EOF. rs=white-space-only DONTBUILD

Comment 20

[Jorg K (CEST = GMT+2)]

Grrr, wrong bug number, this was for bug 1532710, never mind for the removal of two empty lines.

Comment 21

[Geoff Lankow (:darktrojan)]

Emilio, I think we need some help here. We still use <toolbarbutton type="menu-button">, where one side is a button and the other side is a dropmarker for a menu. I suspect we need a fix like the one in bug 1512993. Advice?

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: Switched off 3 more

08:56:08 - jorgk: and this testEventDialog.js
08:56:23 - jorgk: and this testEventDialogModificationPrompt.js
08:56:58 - darktrojan: the event dialog tests are busted by the toolbar button, as I'm sure I've told you

I'm not sure why test_toggling_modes fails, but it's perma-red.

That should get all "Z's" green apart from "Z1" which is a total failure which I wasn't able to get going by disabling certain tests.

Comment 23

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b740b113b39428a6c1ed0ecbc0a6ca8919fe4047
Fix linting errors in test disabling. rs=bustage-fix

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: Disable 9 more tests

So this bustage affects 24 tests in total. From IRC:

09:25:25 - darktrojan: last I checked it was because a file picker opens
09:29:09 - jorgk: I am sure we had a mock picker
09:30:52 - darktrojan: we would have mocked the picker except we're not expecting it
09:31:09 - darktrojan: that's the default action of the button but we want one of the dropdown menu actions

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/67022d52f8ed7b8e270f29f2b774ce5ec236f753
temporarily disable 9 attachment tests. rs=bustage-fix DONTBUILD

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(In reply to Geoff Lankow (:darktrojan) from comment #21)

Emilio, I think we need some help here. We still use <toolbarbutton type="menu-button">, where one side is a button and the other side is a dropmarker for a menu. I suspect we need a fix like the one in bug 1512993. Advice?

Hmm, so looking at the binding hierarchy in comm-central, looks like Thunderbird is overriding the default toolbarbutton binding in this case to not generate a box using xul:extends...

Can you confirm that these elements are still broken without the patch from bug 1450652? I think bug 1512993 is what broke this. Also, does something like this fix it?

diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
index a737e136942c..d0574825a519 100644
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -4031,6 +4031,15 @@ nsCSSFrameConstructor::FindXULButtonData(const Element& aElement,
     return &sXULMenuData;
   }
 
+#ifdef MOZ_THUNDERBIRD
+  // TB relies on these generating a box based on regular `display`.
+  if (aElement.IsXULElement(nsGkAtoms::toolbarbutton) &&
+      aElement.AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
+                           NS_LITERAL_STRING("menu-item"), eCaseMatters)) {
+    return nullptr;
+  }
+#endif
+
   static const FrameConstructionData sXULButtonData =
       SCROLLABLE_XUL_FCDATA(NS_NewButtonBoxFrame);
   return &sXULButtonData;

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1dd0f7372de9
temporarily switch off 3 more tests. rs=bustage-fix

Comment 28

[Jorg K (CEST = GMT+2)]

Looking at
mozmake mozmill-one SOLO_TEST=composition/test-reply-multipart-charset.js
test_replyEditAsNewForward_noPreview passes but test_replyEditAsNewForward_override doesn't. I can see it stalling and it fails with: EXCEPTION: Popup never opened! id=appmenu-popup, state=closed
So it does look like a toolbar button issue since the (sub)test operates the app menu. Hence it was OK to disable it here.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Emilio's suggestion

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)

Can you confirm that these elements are still broken without the patch from bug 1450652? I think bug 1512993 is what broke this. Also, does something like this fix it?

It doesn't, at least not the test from #28 which I tried.

Comment 30

[Magnus Melin [:mkmelin]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)

Can you confirm that these elements are still broken without the patch from
bug 1450652? I think bug 1512993 is what broke this.

Backing out bug 1450652 does seem to make things work again. Backing out bug 1512993 does not.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jorg K (GMT+1) from comment #29)

It doesn't, at least not the test from #28 which I tried.

Blerg, that's because I typoed. What about s/menu-item/menu-button on that patch?

Comment 32

[Jorg K (CEST = GMT+2)]

I'm afraid not. This time I watched the test a bit closer. The app menu button appears to be clicked, but no menu appears. I confirmed this trying the button manually.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Do you know how that menu is supposed to be open usually? Do you have example of the markup that its using? I can try to take a look when I have a bit of free time.

Comment 34

[Magnus Melin [:mkmelin]]

For one of these buttons in general? The "New... v" button in the Filter Dialog is one easily testable. Code is here: https://searchfox.org/comm-central/rev/dbc2627276adff44df57ec3c20f63e6fa463287f/mail/base/content/FilterListDialog.xul#93-106

Thanks for working with us on this!

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Let me add this to my ni? queue so I can try to take a look sometime next week.

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #23)

https://hg.mozilla.org/comm-central/rev/b740b113b39428a6c1ed0ecbc0a6ca8919fe4047
Fix linting errors in test disabling. rs=bustage-fix

Grrr, Pulsebot never logged this:
https://hg.mozilla.org/comm-central/rev/1dd0f7372de94877f6c718e8686df9a9ad838922
temporarily switch off 3 more tests. rs=bustage-fix

Bug 1533967 enables two of them.

Comment 37

[Jorg K (CEST = GMT+2)]

Left from that patch is test-mode-switching.js ...
23:35:59 - aceman: uses the hamburger
23:36:15 - aceman: Popup never opened! id=appmenu-popup, state=closed

So everything disabled here is due to some non-functioning toolbar button menu.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Ok, looking at this now.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

https://github.com/mozilla/releases-comm-central/commit/c04c2f1e7fc10b488909da0df0e5383d3b93c4f8 removed all the stuff that needs fixing.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d23034770a6
Fix some usage of display in XBL bindings in thunderbird. r=npotb

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Attachment: Fix hamburger menu.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 0001-Fix-hamburger-menu.patch

Thank you, Emilio, that in fact fixes the Hamburger menu, so a one liner :-)

I removed the SeaMonkey hunk since we have no way to test this.

From our IRC conversation I understand that we need to add type="menu" or type="menu-button" in the places where it's necessary, not all the ones hit in https://hg.mozilla.org/comm-central/rev/5e366aafa2d5.

Also, your M-C change from comment #42 makes the type="menu-button" work.

I'll check which tests we can re-enable now.

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: Enable 6+7+3+9 - 2 = 23 tests

Comment 46

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23550c36d1596edd3a63118449237df58991cb73

Comment 47

[Jorg K (CEST = GMT+2)]

Can someone please check which buttons still don't work. I tried "Get messages" on the main window, "Spelling", "Security", "Save" and "Attach" on the compose window, all of them work with the M-C change applied, don't work without it.

Maybe something for the night shift ;-)

Comment 48

[Jorg K (CEST = GMT+2)]

test-attachment-reminder.js still fails, most likely using a menu-button. There's also a crash, which was mitigated by switching off 7 tests here: https://hg.mozilla.org/comm-central/rev/6210f90e0832. I think that's the save button.

So let's try enabling 16 instead of 23.

Comment 49

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d369b735fd6c4e1141b76fbe3ca13907ffb7bd6

Comment 50

[Jorg K (CEST = GMT+2)]

Sigh, I got the pea counting wrong. There are 14 attachment tests waiting for the M-C change, so we can switch 23-14 = 9 back on:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6add1c2f378735c384d4a1d70a51f2e5a99fe07

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/64dc6a47eca4
Fix hamburger menu. r=jorgk
https://hg.mozilla.org/comm-central/rev/b644445a3bf9
Re-enabled 9 tests. a=backout

Comment 52

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9d23034770a6

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f1acfe40ebd1
Re-enabled remaining 14 tests. a=backout

Comment 54

[Jorg K (CEST = GMT+2)]

Geoff, see comment #47, does anything else need doing here?