1418063 - Fallout from bug 1416343: TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_alertHook.js and TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_nsMsgMailSession_Alerts.js

Status: ASSIGNED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_alertHook.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_nsMsgMailSession_Alerts.js | xpcshell return code: 0 [log…] 

Run locally we get:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_nsMsgMailSession_Alerts.js
ERROR NS_ERROR_FAILURE: Failure arg 1 [nsIMsgMailSession.alertUser]

mozilla/mach xpcshell-test mail/base/test/unit/test_alertHook.js
ERROR NS_ERROR_FAILURE: Failure arg 1 [nsIMsgMailSession.alertUser]

First seen Thu Nov 16, 2017, 5:01:20 after recovering from bustage:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=4fd7237c5c2f1569e2d1ebbdddf3be2f8f572f73

M-C last good: e070277ec199fa96fa490ed52d33646a37 (on try)
M-C first bad: f41930a869a84af81df1a88d8e82323ff3

Comment 1

[Jorg K (CEST = GMT+2)]

I've looked at it. The problem is that
nsMsgMailSession::AlertUser(const nsAString &aMessage, nsIMsgMailNewsUrl *aUrl)
takes a nsIMsgMailNewsUrl which is not "built-in".

Both tests declare:
var mailnewsURL = {
  get msgWindow() {
    if (gMsgWindow)
      return gMsgWindow;

    throw Cr.NS_ERROR_INVALID_POINTER;
  }
};

or

var msgUrl = {
  _msgWindow: null,

  get msgWindow() {
    return this._msgWindow;
  },

  QueryInterface: XPCOMUtils.generateQI([Ci.nsIMsgMailNewsUrl])
};

with a method msgWindow() since the C++ code calls
  aUrl->GetMsgWindow(getter_AddRefs(msgWindow));

This just stopped working, so all I can do is disable the tests.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1418063-disable-tests.patch

Comment 3

[:aceman]

I think Kent or jcranmer would know how to rewrite this.

Comment 4

[Jorg K (CEST = GMT+2)]

All URIs/URLs derived from the M-C classes must be in C++ now, so you most definitely cannot pass a JS variable as mailnews URL here. Given Kent's and Joshua's availability, it's best to disable the tests, as I have already prepared.

I think the fix is to create a C++ "test only" URL class derived from nsIMsgMailNewsUrl whose GetMsgWindow() function will hand back a windows that can be initialised with xxx.msgWindow = yy; in the test.

Or maybe something like this can work:
- create an nsIMsgMailNewsUrl object in the test (how?):
  let mailnewsUrl = ...;
- mailnewsUrl.msgWindow = // Thing to be returned.
But if it were that easy, why wasn't it done like this in the first place?

Comment 5

[:aceman]

I haven't looked in detail yet. So you mean just the tests override/add a member into the object via JS? But this is not done in normal code and there is actually no bug/no missing functionality in core TB, just the tests can't be written the way they are now.

Comment 6

[Jorg K (CEST = GMT+2)]

The tests provide the object as JS object as quoted in comment #1. TB is fine, only the tests can't be written like this. Bug 1418011 is different. I believe that JS Account is broken since its JS-defined URLs don't work any more.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b089a39f3423
disable test_alertHook.js and test_nsMsgMailSession_Alerts.js. rs=bustage-fix

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1418063-alert-tests.patch - NOT working

This is what I had in mind, but it ain't working, I get:

 0:01.47 LOG: Thread-1 ERROR NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN: Cannot modify properties of a WrappedNative
run_test@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/mail/base/test/unit/test_alertHook.js:42:3
_execute_test@c:\mozilla-source\comm-central\mozilla\testing\xpcshellhead.js:540:7@-e:1:1

Code is:
    // Text, url and window => expect error shown to user
    gAlertShown = false;
42  mailnewsURL.msgWindow = {};
    MailServices.mailSession.alertUser("test error 2", mailnewsURL);
    do_check_true(gAlertShown);

Comment 9

[Jorg K (CEST = GMT+2)]

Tooru-san, can you give me a tip here, please take a look at the patch.

Comment 10

[Jorg K (CEST = GMT+2)]

Looks like I forgot the NI? :-(

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8929374 [details] [diff] [review]
1418063-alert-tests.patch - NOT working

Review of attachment 8929374 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/test/unit/test_alertHook.js
@@ +22,5 @@
>    }
>  };
>  
> +var mailnewsURL = Services.io.newURI("about:blank");
> +mailnewsURL instanceof Components.interfaces.nsIMsgMailNewsUrl;

this has no effect.
it just checks prototype.

@@ +38,5 @@
>    do_check_false(gAlertShown);
>  
>    // Text, url and window => expect error shown to user
>    gAlertShown = false;
> +  mailnewsURL.msgWindow = {};

what situation is this testing, and in what situation such URL is created?
can you just emulate the situation to get the URL with such property,
instead of creating it manually here?

@@ +44,5 @@
>    do_check_true(gAlertShown);
>  
>    // Text, url and no window => export no error shown to user
>    gAlertShown = false;
> +  mailnewsURL.msgWindow = null;

same here

Comment 12

[:aceman]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> > +var mailnewsURL = Services.io.newURI("about:blank");
> > +mailnewsURL instanceof Components.interfaces.nsIMsgMailNewsUrl;
> 
> this has no effect.
> it just checks prototype.

We have similar code in alertTestUtils.js that you looked at (for bug 1401528). I was told in the past instanceof does a hidden .QueryInterface on the object so this code changes mailnewsURL so it can have effect.

Comment 13

[:aceman]

Comment on attachment 8929374 [details] [diff] [review]
1418063-alert-tests.patch - NOT working

Review of attachment 8929374 [details] [diff] [review]:
-----------------------------------------------------------------

I played with this and it really is interesting that this doesn't work.
mailnewsURL.QueryInterface(Components.interfaces.nsIMsgMailNewsUrl) returns
ERROR NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]

mailnewsURL instanceof Components.interfaces.nsIMsgMailNewsUrl returns false.

But we have code that does this even in tests, e.g. at https://dxr.mozilla.org/comm-central/rev/4469c475c6b473927956f509a1ef78da8db5e48c/mailnews/local/test/unit/test_verifyLogon.js#29 and https://dxr.mozilla.org/comm-central/rev/4469c475c6b473927956f509a1ef78da8db5e48c/mailnews/news/test/unit/test_uriParser.js#149 . If those tests work, why doesn't this one. Yes, those use different functions to get the initial nsIURI. E.g. nsNntpService::NewURI() returns nsIURI, but creates it as nsCOMPtr<nsIURI> nntpUri = do_CreateInstance(NS_NNTPURL_CONTRACTID, &rv);. Maybe we need to use some c-c function that returns something that is actually a nsIMsgMailNewsUrl already.

Comment 14

[:aceman]

For example this works for me:
let nntpService = Cc["@mozilla.org/messenger/nntpservice;1"]
                      .getService(Components.interfaces.nsIProtocolHandler);
var mailnewsURL = nntpService.newURI("news://localhost/?newgroups");

This mailnewsURL passes instanceof and also QueryInterface. But gAlertShown isn't set where expected so it fails at the second   Assert.ok(gAlertShown);.
But maybe you can continue from here.

Comment 15

[Wayne Mery (:wsmwk)]

Do we know if this still fails?
And if so, remedy?

Comment 16

[u695164]

It's failing on a local basis for one run and with mach xpcshell-test --verify locally.
On test_alertHook.js#68.

Comment 17

[u695164]

Attachment: Bug 1418063 - Fix and reenable test_alertHook.js. r=darktrojan

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5000d662a119
Fix and reenable test_alertHook.js. r=darktrojan