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

RESOLVED FIXED in Thunderbird 67.0

Status

defect
--
critical
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jorgk, Assigned: emilio)

Tracking

Trunk
Thunderbird 67.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

3 months ago

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

Flags: needinfo?(geoff)
Reporter

Comment 1

3 months ago

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=

Flags: needinfo?(mkmelin+mozilla)

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

Reporter

Comment 3

3 months ago

Emilio, is there any replacement?

Flags: needinfo?(emilio)
Reporter

Comment 4

3 months ago

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.

Assignee

Comment 6

3 months ago

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.

Flags: needinfo?(emilio)
Assignee

Comment 7

3 months ago

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.

Reporter

Comment 8

3 months ago

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

:-(

Reporter

Comment 10

3 months ago

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

3 months ago

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/5e366aafa2d5
Remove extends="xul:" and display="xul:" in XBL bindings; rs=bustage-fix

Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED

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.

Flags: needinfo?(geoff)
Reporter

Comment 13

3 months ago

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Updated

3 months ago
Keywords: leave-open
Reporter

Updated

3 months ago
Status: REOPENED → NEW

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.

Reporter

Comment 15

3 months ago

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

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c44202622511
temporarily disable 6 tests. rs=bustage-fix
Reporter

Updated

3 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:6]
Blocks: 1450652
Reporter

Comment 17

3 months ago

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

Comment 18

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6210f90e0832
temporarily 7 tests. rs=bustage-fix
Reporter

Updated

3 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:6] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:13]

Comment 19

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5f0bde910681
Follow-up: remove blank lines at EOF. rs=white-space-only DONTBUILD
Reporter

Comment 20

3 months ago

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

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?

Flags: needinfo?(emilio)
Reporter

Comment 22

3 months ago

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.

Reporter

Updated

3 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:13] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:16]
Reporter

Comment 23

3 months ago

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

Reporter

Comment 24

3 months ago

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

Reporter

Updated

3 months ago
Attachment #9049122 - Attachment is patch: true
Attachment #9049122 - Attachment mime type: application/octet-stream → text/plain
Reporter

Updated

3 months ago
Attachment #9049466 - Attachment is patch: true
Attachment #9049466 - Attachment mime type: application/octet-stream → text/plain
Reporter

Comment 25

3 months ago

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

Reporter

Updated

3 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:16] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:24]
Assignee

Comment 26

3 months ago

(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;
Flags: needinfo?(emilio)

Comment 27

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1dd0f7372de9
temporarily switch off 3 more tests. rs=bustage-fix
Reporter

Comment 28

3 months ago

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.

Reporter

Comment 29

3 months ago
Posted patch Emilio's suggestion (obsolete) — Splinter Review

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

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

Flags: needinfo?(mkmelin+mozilla)
Severity: normal → critical
Assignee

Comment 31

2 months ago

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

Flags: needinfo?(jorgk)
Reporter

Comment 32

2 months ago

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.

Flags: needinfo?(jorgk)
Reporter

Updated

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

Comment 33

2 months ago

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.

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!

Assignee

Comment 35

2 months ago

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

Flags: needinfo?(emilio)
Reporter

Comment 36

2 months ago

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

Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:24] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:22]
Reporter

Comment 37

2 months ago

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.

Reporter

Updated

2 months ago
Duplicate of this bug: 1534777
Reporter

Updated

2 months ago
Duplicate of this bug: 1534848
Assignee

Comment 40

2 months ago

Ok, looking at this now.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

Comment 42

2 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d23034770a6
Fix some usage of display in XBL bindings in thunderbird. r=npotb
Assignee

Comment 43

2 months ago
Posted patch Fix hamburger menu. (obsolete) — Splinter Review
Attachment #9050716 - Flags: review?(jorgk)
Assignee

Updated

2 months ago
Attachment #9049470 - Attachment is obsolete: true
Reporter

Comment 44

2 months ago

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.

Attachment #9050716 - Attachment is obsolete: true
Attachment #9050716 - Flags: review?(jorgk)
Attachment #9050733 - Flags: review+
Reporter

Comment 47

2 months ago

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

Flags: needinfo?(geoff)
Reporter

Updated

2 months ago
Attachment #9049466 - Attachment description: Disable 8 more tests → Disable 9 more tests
Reporter

Updated

2 months ago
Attachment #9050742 - Attachment description: Enable 6+7+3+8 - 2 + 1 (miscounted, was 9 not 8) = 23 tests → Enable 6+7+3+9 - 2 = 23 tests
Reporter

Updated

2 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:22] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:23]
Reporter

Comment 48

2 months ago

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.

Reporter

Comment 50

2 months ago

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

Reporter

Updated

2 months ago
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:23] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:14]
Reporter

Updated

2 months ago
Keywords: leave-open
Target Milestone: --- → Thunderbird 67.0

Comment 53

2 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f1acfe40ebd1
Re-enabled remaining 14 tests. a=backout

Status: NEW → RESOLVED
Last Resolved: 3 months ago2 months ago
Resolution: --- → FIXED
Reporter

Comment 54

2 months ago

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

Flags: needinfo?(geoff)
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:14] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]

Updated

2 months ago
Duplicate of this bug: 1535152
You need to log in before you can comment on or make changes to this bug.