1354121 - Massive test failure in mailnews/db/gloda/test/unit/ - 28 tests fail

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_mime_emitter.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_corrupt_database.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_addressbook.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_bad_messages.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_junk_imap_online.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_compaction.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_junk_imap_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_junk_local.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_cleanup_msf_databases.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_fts3_tokenizer.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_gloda_content_local.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_imap_online.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_imap_online_to_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_folder_logic.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_noun_mimetype.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_msg_search.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_intl.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_local.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_sweep_folder.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_migration.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_query_messages_imap_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_query_messages_imap_online.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_query_messages_imap_online_to_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_query_messages_local.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_startup_offline.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_query_core.js | xpcshell return code: 0

M-C last good: ec8d1d3db50c85037e8077c32c8403570a
M-C first bad: 3c68d659c2b715f811708f043a1e7169d7

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ec8d1d3db50c85037e8077c32c8403570a&tochange=3c68d659c2b715f811708f043a1e7169d7

Sadly this started failing with a Daily run so there is no debug version to inspect the log in detail.

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe that's a clue:
Assertion failure: false (You are trying to use a deprecated mozStorage method.
Check error message in console to identify the method name.), at c:/mozilla-source/comm-central/mozilla/storage/mozStorageStatement.cpp:888

#01: nsFts3Tokenizer::RegisterTokenizer (c:\mozilla-source\comm-central\mailnews\extensions\fts3\src\nsfts3tokenizer.cpp:49)

There has been a problem with this before: Bug 1252937.

Comment 2

[Jorg K (CEST = GMT+2)]

OK, found it, bug 1007034: https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db

I'll have to disable the assert locally. Aceman, can you take a look at this.

Comment 3

[Jorg K (CEST = GMT+2)]

Error console says:
mozilla::storage::Statement::BindUTF8StringParameter is deprecated and will be removed soon.  (unknown)
mozilla::storage::Statement::BindBlobParameter is deprecated and will be removed soon.  (unknown)
mozilla::storage::Statement::BindStringParameter is deprecated and will be removed soon. (unknown)
mozilla::storage::Statement::BindInt64Parameter is deprecated and will be removed soon.  (unknown)
mozilla::storage::AsyncStatement::BindInt64Parameter is deprecated and will be removed soon. (unknown)
mozilla::storage::AsyncStatement::BindStringParameter is deprecated and will be removed soon. (unknown)
mozilla::storage::AsyncStatement::BindInt64Parameter is deprecated and will be removed soon. (unknown)
mozilla::storage::AsyncStatement::BindStringParameter is deprecated and will be removed soon. (unknown)
mozilla::storage::AsyncStatement::BindInt64Parameter is deprecated and will be removed soon. (unknown)

Gloda is using those all over the place:
https://dxr.mozilla.org/comm-central/search?q=indInt64Parameter&redirect=false

And it's all deprecated:
https://dxr.mozilla.org/comm-central/rev/2a593ea93f6637df49eedc998b1f5ae4781a0f56/mozilla/storage/mozIStorageBaseStatement.idl#55

Comment 4

[Jorg K (CEST = GMT+2)]

Looks like bug 645049 has the information we need:
https://hg.mozilla.org/mozilla-central/rev/046c166f999f

-  stmt.bindStringParameter(0, "foo%20" + LATIN1_ae + "/_bar");
+  stmt.bindByIndex(0, "foo%20" + LATIN1_ae + "/_bar");

 function test_add_data()
 {
   var stmt = makeTestStatement(
     "INSERT INTO test (id, string, number, nuller, blober) " +
     "VALUES (?, ?, ?, ?, ?)"
   );
-  stmt.bindBlobParameter(4, BLOB, BLOB.length);
-  stmt.bindNullParameter(3);
-  stmt.bindDoubleParameter(2, REAL);
-  stmt.bindStringParameter(1, TEXT);
-  stmt.bindInt32Parameter(0, INTEGER);
+  stmt.bindBlobByIndex(4, BLOB, BLOB.length);
+  stmt.bindByIndex(3, null);
+  stmt.bindByIndex(2, REAL);
+  stmt.bindByIndex(1, TEXT);
+  stmt.bindByIndex(0, INTEGER);

Comment 5

[Marco Bonardo [:mak]]

yep, in cpp please either use bindXXXByIndex or bindXXXByName, don't use bindMMMParameter.
in js please use bindByIndex or bindByName.

Comment 6

[:aceman]

It seems all our uses are in JS. So when to use ByIndex and when ByName?

Also these are very few remaining uses in m-c (in a single file). Then you could remove the functions.

Comment 7

[Jorg K (CEST = GMT+2)]

Looks like bindByIndex has the same inferface as bindXXXParameter. So it's bindByIndex. What am I missing?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1354121-bind-parameter.patch (v1).

Maybe something like this?

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1354121-bind-parameter.patch (v2).

Like this.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1354121-bind-parameter.patch (v3).

Fixed obvious find/replace errors in previous patch.

Comment 11

[:aceman]

Comment on attachment 8855512 [details] [diff] [review]
WIP: 1354121-bind-parameter.patch (v3).

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

OK, after fixing the remaining typo below the gloda xpcshell tests work for me locally (but I do not have the broken m-c version so please push to try).

::: mailnews/db/gloda/modules/datastore.js
@@ +3044,5 @@
>     *     purged.
>     */
>    clearMessageAttributes: function gloda_ds_clearMessageAttributes(aMessage) {
>      if (aMessage.id != null) {
> +      this._deleteMessageAttributesByMessageIDstatement.bindByIndex(0,

IDStatement

Comment 12

[:aceman]

Attachment: 1354121-bind-parameter.patch (v4).

With fixed typo.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1354121-bind-parameter.patch

Another simplification.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1354121-bind-parameter.patch (v6).

Missed some bits in previous version. Also needed C++ changes.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/9ab35896429cdfeb819c699aa80b408aa8ccd9bb
Tests passed locally, so this should be good.

Comment 16

[Marco Bonardo [:mak]]

sorry for late answer, your changes look good.
bindByName is to be used when you want to bind a param by name, for example
SELECT * FROM table WHERE field = :val
you could either use bindByIndex(0, ...) or bindByName("val", ...)

Comment 17

[Marco Bonardo [:mak]]

(In reply to :aceman from comment #6)
> Also these are very few remaining uses in m-c (in a single file). Then you
> could remove the functions.

Yes, that was the scope of the change, the only remaining thing is the definitions in mozIStorageBaseStatement.idl that are deprecated. In a couple versions we'll remove them.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from comment #16)
> sorry for late answer, your changes look good.
Marco, thanks for your support, we got it sorted out.