1362466 - Massive Xpcshell test failure on 2017-05-05

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Jorg K (CEST = GMT+2)]

Various test failures including crashes in debug builds:

First seen: Fri May 5, 2017, 15:47:11

M-C last good: 0b255199db9d6a6f189b89b7906f99155b
M-C first bad: 9348b76977e833f108cf77dff75b0fab88

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155b&tochange=9348b76977e833f108cf77dff75b0fab88

TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_detachToFile.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_junkingWhenDisabled.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.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_cleanup_msf_databases.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_gloda_content_imap_offline.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_bad_messages.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_migration.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_mime_emitter.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapAttachmentSaves.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_query_messages_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_online_to_offline.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_messages_imap_offline.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_imap_online.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/mime/test/unit/test_hidden_attachments.js | xpcshell return code: 0

Comment 1

[Jorg K (CEST = GMT+2)]

Crashing test:

TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js | xpcshell return code: 1

PID 8066 | [8066] ###!!! ASSERTION: tried to add duplicate listener: 'index == size_t(-1)', file /builds/slave/tb-c-cen-l64-d-000000000000000/build/mailnews/base/src/nsMsgMailSession.cpp, line 69
PID 8066 | Hit MOZ_CRASH() at /builds/slave/tb-c-cen-l64-d-000000000000000/build/mozilla/memory/mozalloc/mozalloc_abort.cpp:33

Comment 2

[Jorg K (CEST = GMT+2)]

I also see:
"CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/components/nsHandlerService-json.js :: _injectDefaultProtocolHandlersIfNeeded :: line 127"  data: no]"]

Looking at nsHandlerService-json.js:127 I see:
      let prefsDefaultHandlersVersion = Number(Services.prefs.getComplexValue(
        "gecko.handlerService.defaultHandlersVersion",
        Ci.nsIPrefLocalizedString).data);

TB doesn't have that preference.

Looking at nsHandlerService-json.js via
https://hg.mozilla.org/mozilla-central/log/tip/uriloader/exthandler/nsHandlerService-json.js
I come to https://hg.mozilla.org/mozilla-central/rev/97a69c3ff07c and bug 1287658.

Checkin comment reads:
Bug 1287658 - Migrate from "mimeTypes.rdf" to "handlers.json". r=mak
The default nsIHandlerService implementation is switched to the one that uses the JSON data store, and any previously configured handlers are imported from the RDF data store if the "gecko.handlerService.migrated" preference is not set.

Bingo!

So with some luck, this might come down to a few missing preferences, although Migrate from "mimeTypes.rdf" to "handlers.json" sounds pretty scary.

Marco, can you advise us on what to do here?

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-gecko-handlerservice.patch

This fixes the Xpcshell tests I ran manually.

Review will have to be post landing. I'm happy to land a follow-up but let's get some failures fixed for now.

I'm sure Marco will also advise us.

Comment 4

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4ee087659e876d2a372638e8c8d9ecab2c1bdb59

Let's see how far this gets us. There is also bug 1362461 which popped up in the same regression range, so maybe it's related.

Comment 5

[Richard Marti (:Paenglab)]

This comment https://dxr.mozilla.org/comm-central/source/mozilla/browser/locales/en-US/chrome/browser-region/region.properties#18-22 should also be added to explain for what it is.

Comment 6

[Jorg K (CEST = GMT+2)]

Sure, the comment says:
# increment this number when anything gets changed in the list below.

But there is no list below. And the one that FF has, I don't understand. So right now such a comment would only confuse.

What I landed is a bustage fix to see how much bustage will go away now. We need to get educated about this stuff since copying/imitating FF will only go so far.

Comment 7

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #5)
> This comment
> https://dxr.mozilla.org/comm-central/source/mozilla/browser/locales/en-US/
> chrome/browser-region/region.properties#18-22 should also be added to
> explain for what it is.

It seems TB does not have the prefs this pref is "guarding" so maybe we need a different comment. Also jorg used a value of 1, while FF is at 4. Maybe we never need to advance from 1, only need the pref to be defined.

Comment 8

[Jorg K (CEST = GMT+2)]

Well, the comment says to increment when something changes. Check (M-C) mobile/locales/en-US/chrome/region.properties, they're at 3.

Comment 9

[Jorg K (CEST = GMT+2)]

Looks like the patch achieved something ;-) All failures from comment #0 are gone, but instead three M-C Xpcshell tests fail:

TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService.js | run_test - [run_test : 162] 2 == 0
TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService_json.js | test_default_protocol_handlers - [test_default_protocol_handlers : 182] false == true
TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService_rdf.js | test_default_protocol_handlers - [test_default_protocol_handlers : 182] false == true

The failure from bug 1362461 is still there, so that seems to be unrelated.

Comment 10

[Jorg K (CEST = GMT+2)]

Looking into this a little further:

Maybe adding the gecko.handlerService.defaultHandlersVersion preference was wrong and maybe nsHandlerService-json.js should *not* rely on that preference.

In test_handlerService.js
https://dxr.mozilla.org/mozilla-central/rev/0b255199db9d6a6f189b89b7906f99155bde3726/uriloader/exthandler/tests/unit/test_handlerService.js#126
we read:
  // If we have a defaultHandlersVersion pref, then assume that we're in the
  // firefox tree and that we'll also have default handlers.

This implies that nothing should unconditionally retrieve that pref.

I also found:
https://hg.mozilla.org/mozilla-central/rev/97a69c3ff07c#l6.116
+  if (!Services.prefs.getPrefType("gecko.handlerService.defaultHandlersVersion")) {
+    do_print("This platform or locale does not have default handlers.");
+    return;

So some parts of M-C cater for gecko.handlerService.defaultHandlersVersion not existing.

Comment 11

[Jorg K (CEST = GMT+2)]

Adding NI for the author of bug 1287658 as well.

Comment 12

[:Paolo Amadini]

(In reply to Jorg K (GMT+2) from comment #2)
> "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Component
> returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
> [nsIPrefBranch.getComplexValue]"  nsresult: "0x8000ffff
> (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
> file:///c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/
> components/nsHandlerService-json.js ::
> _injectDefaultProtocolHandlersIfNeeded :: line 127"  data: no]"]

This message should be harmless, and the code should work fine without the preference, but if it makes the tests fail because they listen to console errors, I'll take a patch that adds a try-catch around the code that reads the preference, so the error is not reported.

Comment 13

[Jorg K (CEST = GMT+2)]

I've already done something similar here:
https://hg.mozilla.org/mozilla-central/rev/d913a0c7d25a
Patch coming.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-handlerService.patch (v1)

I made it more like https://hg.mozilla.org/mozilla-central/rev/d913a0c7d25a.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-handlerService.patch (v1b).

Sorry about the noise, fixed print statement.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864981 [details] [diff] [review]
1362466-gecko-handlerservice.patch

Nothing to review here, this was the wrong fix, I'll back it out.

Comment 17

[:Paolo Amadini]

Comment on attachment 8865134 [details] [diff] [review]
1362466-handlerService.patch (v1b).

This being production code, it's better to have just one call to Services.prefs.getComplexValue, and check for the expected exception NS_ERROR_UNEXPECTED. Also, do_print does not exist, but is unnecessary anyways.

I think the condition to check is the following, but please test it:

if (ex instanceof Components.Exception && ex.result == Cr.NS_ERROR_UNEXPECTED) {
  // This platform does not have any default protocol handlers configured.
  return;
}
throw ex;

Note that you can place this new try-catch block at the beginning of the function and out of the existing generic try-catch block, because the "locale" variable is not used until later. Also, the conversion to Number() and reading .data can still go in the second try-catch block.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-handlerService.patch (v2).

This should be what you suggested. With this patch, our tests pass.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-handlerService.patch (v2).

Forgot to refresh the patch before attaching, sorry about the noise.

Comment 20

[:Paolo Amadini]

Comment on attachment 8865207 [details] [diff] [review]
1362466-handlerService.patch (v2).

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

Thanks!

::: uriloader/exthandler/nsHandlerService-json.js
@@ +137,5 @@
> +    }
> +
> +    try {
> +      prefsDefaultHandlersVersion =
> +        Number(prefsDefaultHandlersVersion.data);

nit: this doesn't need to wrap.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1362466-handlerService.patch (v2) - nit fixed.

Carrying forward Paolo's r+. Thanks for the quick turn-around.

Comment 22

[Jorg K (CEST = GMT+2)]

Moving this to Firefox::File handling (like bug 1287658) to get sheriff's attention.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/844ef25e35bb
Skip default handler injection if pref gecko.handlerService.defaultHandlersVersion not defined. r=paolo

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/844ef25e35bb

Comment 25

[Jorg K (CEST = GMT+2)]

Backed out incorrect C-C fix from comment #4:
https://hg.mozilla.org/comm-central/rev/d40a564cba942e2b291513bb7f4233d1d5ac3eb6