Closed Bug 1588500 Opened 5 years ago Closed 5 years ago

message-header/test-phishing-bar.js fails and seems to affect other tests in the same directory

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: jorgk-bmo, Unassigned)

References

(Regression)

Details

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

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1570954 +++

As per bug 1570954 comment #13:

Hmm, I tried to run the entire directory message-header and crashed on test-phishing-bar.js with:

TEST-START | c:\mozilla-source\comm-central\comm\mail\test\mozmill\message-header\test-phishing-bar.js | test_no_phishing_warning_for_ip_sameish_text
[5788, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.cpp, line 176
++DOMWINDOW == 84 (000001FC4CD1C000) [pid = 5788] [serial = 95] [outer = 000001FC493B05C0]
[5788, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file c:/mozilla-source/comm-central/parser/html/nsHtml5StreamParser.cpp, line 1142
Assertion failure: (((HRESULT)(hr)) >= 0), at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/ShellHeaderOnlyUtils.h:132
#01: nsExternalHelperAppService::LoadURI (c:\mozilla-source\comm-central\uriloader\exthandler\nsExternalHelperAppService.cpp:966)
#02: XPTC__InvokebyIndex[c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x8dd6c62]
#03: CallMethodHelper::Call (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1183)
#04: XPCWrappedNative::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1149)
#05: XPC_WN_CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNativeJSOps.cpp:946)

I took some subtests of test-phishing-bar.js out and now the entire directory passes.

Here's a try run with that:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=80ec64e7abea4411e3b5a0b075ac13e3c2226046

Looks like more fallout from bug 1578624, like we already have bug 1588256.

Also seen in debug runs as:
Assertion failure: (((HRESULT)(hr)) >= 0), at z:/build/build/src/obj-thunderbird/dist/include\mozilla/ShellHeaderOnlyUtils.h:132

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7b3bbfde0f2e
disable failing subtests of test-phishing-bar.js. rs=bustage-fix

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true

Unless I'm mistaken, the
Assertion failure: (((HRESULT)(hr)) >= 0), at z:/build/build/src/obj-thunderbird/dist/include\mozilla/ShellHeaderOnlyUtils.h:132
is first seen here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=bcc11bb0ae6ca41ec12c720113d6e68937e3ff2e&selectedJob=263920779
on Wed, Aug 28, 2019, 19:29:02.

And it dies in test-phishing-bar.js:
https://taskcluster-artifacts.net/N3KLQhz7SB6-PprndRT9ew/0/public/logs/live_backing.log

Running the test manually, those subtest open tabs in the default browser Firefox, and I don't think that's intended.

So this is a quite long-standing issue which has been ignored :-(

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e52e746c47a89e8ff2e5fc08bfa79b164&tochange=73353f375d1dda3d4ff434da2a4f7bdfd3

It looks like it's from bug 1570845. But I think our test is wrong. Opening the external browser should be dummied up.

Aceman, what does the test do on Linux?

Looking at this again, I guess it was wrong to disable the tests completely, they should have just been disabled on Windows.

Flags: needinfo?(acelists)
Regressed by: 1570845

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b3cd617ddb36
Disable subtests of test-phishing-bar.js on Windows only. rs=bustage-fix DONTBUILD

Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED

Aceman ran the test for me and reported that his default browser got opened. I don't think this is a good thing to do in automation since there might not even be a default browser configured. In automation the test only crashes on Windows 10 debug.

Toshi, why do we assert here
https://searchfox.org/comm-central/rev/bc0f15958bd96e2e92e1b9e739491f91e8f8c9d2/mozilla/widget/windows/ShellHeaderOnlyUtils.h#132
if it's not fatal? Under which circumstances can that happen?

The test simulates a click on a link which launches an external browser.

Flags: needinfo?(acelists) → needinfo?(tkikuchi)

Toshi, why do we assert here
https://searchfox.org/comm-central/rev/bc0f15958bd96e2e92e1b9e739491f91e8f8c9d2/mozilla/widget/windows/ShellHeaderOnlyUtils.h#132
if it's not fatal? Under which circumstances can that happen?

We're not sure when CoAllowSetForegroundWindow fails, and that's why we added MOZ_ASSERT. Let me run message-header/test-phishing-bar.js and see there is anything we should do in ShellExecuteByExplorer.

I'm in the training this week. I'll keep needinfo? until I start looking into this.

Thanks, if you need any assistance with running the test (or anything else related to TB), please let me know.

This directory's been failing like mad on Linux, and it started with the push in comment 4.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/80a194afbb0f Disable subtests of test-phishing-bar.js for Linux as well as Windows. rs=bustage-fix DONTBUILD

Looks like the push in comment #4 closed this inadvertently.

Status: REOPENED → NEW

So if this fails on Linux, can a developer in Linux fix it? According to comment #5 it was launching Aceman's default browser. My suggestion would be to dummy-up launching the browser in the first place since we don't know how automation servers are set up.

Flags: needinfo?(geoff)
Regressions: 1591357
No longer regressions: 1591357

Preventing the browser from (trying) to open fixes the problems at least locally. Let's see about try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7900673511aef930bd02a7e8d73b09715fc47606

Well, if you're not opening in an external browser, you might as well enable Windows, too.

Flags: needinfo?(geoff)
Attached patch bug1588500_phish_link.patch (deleted) — Splinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9104313 - Flags: review?(jorgk)

In cases like this one, an artifact build is much faster:
try: -b o -p linux64-shippable,macosx64-shippable,win64-shippable -u mozmill-1proc --artifact
Or you can also do -u all.

I could have checked the test before landing (as I will anyway).

Comment on attachment 9104313 [details] [diff] [review] bug1588500_phish_link.patch Thanks. That will make Toshihito's life a little harder, but he can just back that out locally.
Attachment #9104313 - Flags: review?(jorgk) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b964b4e0f62f
fix message-header/test-phishing-bar.js by avoiding opening an external browser. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Attachment #9104313 - Flags: approval-comm-beta+

I was a bit foolish landing this. Both try runs show Z4 failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7900673511aef930bd02a7e8d73b09715fc47606
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e62acefaabf5984c2e30ffe82f353f489900dc4c

Yes, message-header/test-phishing-bar.js runs through, but if you run the entire directory like so
mozmake SOLO_TEST=message-header mozmill-one
you get this error:

TEST-UNEXPECTED-FAIL | Z:\task_1572037219\build\tests\mozmill\message-header\test-reply-identity.js | test-reply-identity.js::test_reply_no_matching_identity

That can be reproduced locally.

Status: RESOLVED → REOPENED
Flags: needinfo?(mkmelin+mozilla)
Resolution: FIXED → ---

Backout:
https://hg.mozilla.org/comm-central/rev/08af05c34c82f5367122d2c08661ffbf3b0e192d
Backed out changeset b964b4e0f62f (bug 1588500) for MozMill test failures. a=backout

Sorry, Magnus, your changes for the phishing test seem fine and the test itself passes and you suppress opening the external browser. But there is a gremlin in the directory, see comment #19. Geoff also noticed that, see comment #8 :-(

Status: REOPENED → ASSIGNED
Attachment #9104313 - Flags: approval-comm-beta+

Locally (with and without the patch) the whole directory works for me on linux.

I do see likely unrelated console spew about (xref bug 1446699)
GLib-GObject-WARNING **: 14:39:19.132: ../../../gobject/gsignal.c:3498: signal name 'selection_changed' is invalid for instance '0x7f4eba480110' of type 'MaiAtkType3'

Assignee: mkmelin+mozilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mkmelin+mozilla)

Hi Toshihito, are you sure your try run is want you really wanted to do? The second backout restores the
https://hg.mozilla.org/try-comm-central/rev/03a8e2dacf96103a4ffaa07e9f4e453cb48cd528#l1.13
disabled_test_ ... functions, so the test will still not run. To go back to the beginning, you also need to backout the changeset from comment #2.

Sorry, your try got caught up in total bustage which has been fixed now.

What do you want to achieve with a try? IIRC, the issue is intermittent but fails each time on a local build. If you just want to inspect a log, turn to comment #2.

I want to repro the assert of ShellHeaderOnlyUtils.h. I ran mozmake SOLO_TEST=message-header mozmill-one locally with the latest build (without any backout) , but I didn't get a repro. What should I do to get a repro on top of the latest build?

Flags: needinfo?(jorgk)
Attached patch 1588500-re-enable.patch (deleted) — Splinter Review

Hi Toshihito, thanks for your persistence. With no backout, the test is disabled. You'd have to apply the three backouts from your latest try, or simple use the folded patch I'm providing here for your convenience. It simply removes all the disabling that we tweaked in those three changesets (first on all platforms, then only on Windows, last on Windows and Linux since Linux also seems to have a problem).

In short, with the patch applied running
mozmake SOLO_TEST=message-header/test-phishing-bar.js mozmill-one
I immediately get
Assertion failure: (((HRESULT)(hr)) >= 0), at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/ShellHeaderOnlyUtils.h:132
on Windows 10 x64 Pro (1903).

I believe that in automation I've seen the issue come and go, but locally it fails 100%.

Off-topic: Looks like bug 1567614 has raised its ugly head again :-(

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

Thank you for providing the detail information! I've got the repro and I think I got the root cause as well. I filed a new bug 1592444 to track this assert issue.

Flags: needinfo?(tkikuchi)

Sigh, running this with Magnus patch, attachment 9104313 [details] [diff] [review], still gives:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\message-header\test-reply-identity.js | test-reply-identity.js::test_reply_no_matching_identity
  EXCEPTION: controller(): Window could not be initialized.
    at: utils.jsm line 390
       TimeoutError utils.jsm:390 13
       waitFor utils.jsm:446 11
       MozMillController controller.jsm:353 9
       WindowWatcher_waitForWindowOpen WindowHelpers.jsm:291 13
       wait_for_new_window WindowHelpers.jsm:642 19
       wait_for_compose_window ComposeHelpers.jsm:265 34
       open_compose_with_reply ComposeHelpers.jsm:84 10
       test_reply_no_matching_identity test-reply-identity.js:156 18

Running it on Windows with all the tests enabled, runs through.

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a820213b2122781b9ab4478f08d5d8d33b06b9e0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c4045d7f2eb0
Re-enable test-phishing-bar.js on Linux and Windows. r=me

Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED

We'll see whether it raised its ugly head again on Linux. Windows should be fine after bug 1592444.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: