1428389 - Xpcshell on 2018-01-05: 3 failing Xpcshell tests (Friday bustage) -; leftover from bug 1428341

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Jorg K (CEST = GMT+2)]

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

After landing bug 1428341, three failures are still left:

TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unifiedcomplete/test_visit_url.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | toolkit/components/places/tests/unifiedcomplete/test_search_suggestions.js | Test timed out

M-C last good: 3acb14b949150529ec761f845f9a3d61ee
M-C first bad: 81362f7306fe413b19fdba27cd0e9a5525

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3acb14b949150529ec761f845f9a3d61ee&tochange=81362f7306fe413b19fdba27cd0e9a5525

mozilla/mach xpcshell-test toolkit/components/places/tests/unifiedcomplete/test_visit_url.js
gives
[JavaScript Error: "weaveXPCService is not defined" {file: "resource://gre/modules/PlacesRemoteTabsAutocompleteProvider.jsm" line: 119}]

toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js also has this error.

Looks like this comes from bug 1427850:
https://hg.mozilla.org/mozilla-central/rev/fcec2e929c2c#l2.14

    if (!weaveXPCService.ready || !weaveXPCService.enabled) {

weaveXPCService comes from a lazy getter:

XPCOMUtils.defineLazyGetter(this, "weaveXPCService", function() {
  return Cc["@mozilla.org/weave/service;1"]
           .getService(Ci.nsISupports)
           .wrappedJSObject;
});

Maybe we need to package Weave as SeaMonkey does:
suite/installer/package-manifest.in
@RESPATH@/components/Weave.js

But then it should fail locally??

Gijs, Thom or Mark, could you please enlighten me here.

Comment 1

[Jorg K (CEST = GMT+2)]

I meant to say: But then it should NOT fail locally??

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Hmm, I think the right solution would be to check !weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled instead, in that case. It's admittedly awkward, though...

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1428389-package-weave.patch

See whether this helps:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=40f897eda8c58f02f81a176c259994b1d8fb4e74

Comment 4

[:Gijs (he/him)]

Does Thunderbird or SeaMonkey actually support sync or remote tab matches in any way?

Seems like these tests just shouldn't be run for Thunderbird. The urlbar unifiedcomplete tests generally don't seem like they'd be all that useful unless SM's browser uses it (which I didn't think it did, but perhaps it's following Firefox in that respect...)

Comment 5

[Jorg K (CEST = GMT+2)]

Gijs, thanks for the comment, but I have no idea, I don't even know what "support sync or remote tab matches" are.

We generally run all M-C xpcshell tests, which a select few that are switched off.

Not packaging Weave.js isn't a good start, is it?

So do you prefer to switch off these three tests or just add the
  !weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled 
as suggested in comment #2?

Apparently we have always run the tests, only that after bug 1427850 they don't work any more.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

> I don't even know what "support sync or remote tab matches" are.

The code in question is for Sync users (e.g. users logged in with a Firefox Account) who have synced tabs enabled. It allows pages open in tabs on remote devices (which are connected to the same Firefox Account) to view those in awesome bar results.

Like Gijs, I don't *think* this is relevant to Thunderbird or Seamonkey. I could be wrong, though.

Comment 7

[Jorg K (CEST = GMT+2)]

Mac builtbot is busted, so trying this again on TC:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73c09533c0561c2d95d295bfb2b1ea4af8d96589

Comment 8

[:Gijs (he/him)]

(In reply to Thom Chiovoloni [:tcsc] from comment #6)
> > I don't even know what "support sync or remote tab matches" are.
> 
> The code in question is for Sync users (e.g. users logged in with a Firefox
> Account) who have synced tabs enabled. It allows pages open in tabs on
> remote devices (which are connected to the same Firefox Account) to view
> those in awesome bar results.
> 
> Like Gijs, I don't *think* this is relevant to Thunderbird or Seamonkey. I
> could be wrong, though.

Right. I think the correct fix here is likely to remove/disable sync/weave primitives/things, not add them. Thunderbird and SeaMonkey, I'm pretty sure, don't support Firefox Account type sync, and so I don't think they should ship or test this stuff.

Comment 9

[Jorg K (CEST = GMT+2)]

Hmm, the attempt to package Weave.js led to:
Error: /builds/worker/workspace/build/src/comm/mail/installer/package-manifest.in:632: Missing file(s): Thunderbird Daily.app/Contents/Resources/components/Weave.js

I wonder how SeaMonkey manage to package it.

So the question again:
Should I send a patch disabling those three tests for TB or should I do this:
  !weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled 
Are all the three tests about sync or remote tab matches?

I just don't want to get into a situation where we need to keep switching off tests every week.

Comment 10

[Lina Butler [:lina]]

I think adding a `try...catch` to the `weaveXPCService` lazy getter, as we do for the `Weave` getter, then following Thom's suggestion from comment 2 should do the trick.

Comment 11

[:Gijs (he/him)]

(In reply to Jorg K (GMT+1) from comment #9)
> Hmm, the attempt to package Weave.js led to:
> Error:
> /builds/worker/workspace/build/src/comm/mail/installer/package-manifest.in:
> 632: Missing file(s): Thunderbird
> Daily.app/Contents/Resources/components/Weave.js
> 
> I wonder how SeaMonkey manage to package it.
> 
> So the question again:
> Should I send a patch disabling those three tests for TB or should I do this:
>   !weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled 
> Are all the three tests about sync or remote tab matches?

The two are intimately related. You can't have remote tab matches without sync, much like you can't have tab matches without tabs...

Kit and Thom are on the money here.

> I just don't want to get into a situation where we need to keep switching
> off tests every week.

I understand that, but the long term solution here is to disentangle this inasmuch as necessary (right now, with the proposed try catches and/or nullchecks) and totally avoiding shipping/including/testing this. Thunderbird and SeaMonkey aren't using it, shouldn't ship it, and don't need to test it.

If unifiedcomplete can't function without the remote tab autocomplete provider thingy then either that needs fixing (ie perhaps there's tight coupling that needs to be loosened, I'm not sure offhand), and/or you should stop shipping/testing unifiedcomplete with Thunderbird/SeaMonkey if it is unused (which I am not sure about offhand, either).

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1428389-weaveXPCService.patch

This fixes the three test failures for me. Thanks for the suggestions.

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8940348 [details] [diff] [review]
1428389-weaveXPCService.patch

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

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537f1536010a
handle case gracefully where weaveXPCService is not available. r=kitcambridge

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/537f1536010a