1278968 - multiple "TypeError: params is null" in ab*AutoComplete*.js when running tests.

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

Some tests produce "TypeError: params is null" JS errors.

Can be seen e.g. in: http://archive.mozilla.org/pub/thunderbird/nightly/2016/06/2016-06-08-03-02-08-comm-central/comm-central_xp_ix_test-mozmill-bm112-tests1-windows-build26.txt.gz

04:39:20     INFO -  TEST-START | C:\slave\test\build\tests\mozmill\folder-display\test-message-commands-on-msgstore.js | test_mark_messages_forwarded
04:39:20     INFO -  JavaScript error: resource://gre/components/nsAbAutoCompleteMyDomain.js, line 22: TypeError: params is null
04:39:20     INFO -  JavaScript error: resource://gre/components/nsAbAutoCompleteSearch.js, line 338: TypeError: params is null
04:39:20     INFO -  JavaScript error: resource://gre/components/nsAbLDAPAutoCompleteSearch.js, line 171: TypeError: params is null
04:39:20     INFO -  JavaScript error: resource://gre/components/nsNewsAutoCompleteSearch.js, line 99: TypeError: params is null
04:39:22     INFO -  JavaScript error: resource://gre/components/nsAbAutoCompleteMyDomain.js, line 22: TypeError: params is null
04:39:22     INFO -  JavaScript error: resource://gre/components/nsAbAutoCompleteSearch.js, line 338: TypeError: params is null
04:39:22     INFO -  JavaScript error: resource://gre/components/nsAbLDAPAutoCompleteSearch.js, line 171: TypeError: params is null
04:39:22     INFO -  JavaScript error: resource://gre/components/nsNewsAutoCompleteSearch.js, line 99: TypeError: params is null
04:39:30     INFO -  TEST-PASS | C:\slave\test\build\tests\mozmill\folder-display\test-message-commands-on-msgstore.js | test-message-commands-on-msgstore.js::test_mark_messages_forwarded

I've tracked it down to tests that type the recipient in compose and then close the window. The autocomplete has no time to kick in and the params argument is null. Adding some sleep time fixed the problem for me (no more error). Alternatively we can accept empty params.

Comment 1

[:aceman]

Attachment: patch

Try run: https://hg.mozilla.org/try-comm-central/rev/4c89fd6d8e503653fdf2e5c7b09509f9be2fa61a

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8761379 [details] [diff] [review]
patch

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

::: mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js
@@ +188,5 @@
>      // use that, otherwise, try the global preference instead.
>      var acDirURI = null;
>      var identity;
>  
> +    if ("idKey" in params) {

Drive-by question for my own education [learning on the job ;-)]:
Is this equivalent? Can "params" not have a property "idKey", yet the value is null or ""? (multiple occasions).
Otherwise this patch looks good to me.

Comment 3

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #2)
> Drive-by question for my own education [learning on the job ;-)]:
> Is this equivalent? Can "params" not have a property "idKey", yet the value
> is null or ""? (multiple occasions).

Yes, if there is no idKey property in params and you ask for params.idKey it returns the special value undefined (or similar). However in gecko JS, ot also throws a warning that you are accessing nonexistent property. On the other hand, it is common pattern to do "if (params.idKey)" to test if the property exists and in this single case I think there is no warning. Anyway, ("idKey" in params) should be clean an warning free, as it is a valid expression and returns true or false, so it does not rely on undefined being handled as false and hoping there is no warning :) Of course params must be an object and not null.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8761379 [details] [diff] [review]
patch

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

r=mkmelin with the below changed at all the places

::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
@@ +17,5 @@
>  
>    applicableHeaders: new Set(["addr_to", "addr_cc", "addr_bcc", "addr_reply"]),
>  
>    startSearch: function(aString, aSearchParam, aResult, aListener) {
> +    let params = JSON.parse(aSearchParam) || {};

Not sure I like this much as it kind of looks like "try parse it, if unsuccessful just assign to {}". Well it only works if aSearchParam is null, not "" or undefined etc. 

Maybe better to have it

  let params = aSearchParam ? JSON.parse(aSearchParam) : {};

Comment 5

[:aceman]

Ok, thanks,
https://hg.mozilla.org/comm-central/rev/7f8a655fe06995a5d196dca3f21843d2791727a2