Closed Bug 1428389 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
Flags: needinfo?(tchiovoloni)
Flags: needinfo?(gijskruitbosch+bugs)
I meant to say: But then it should NOT fail locally??
Flags: needinfo?(markh)
Hmm, I think the right solution would be to check !weaveXPCService || !weaveXPCService.ready || !weaveXPCService.enabled instead, in that case. It's admittedly awkward, though...
Flags: needinfo?(tchiovoloni)
Flags: needinfo?(markh)
Flags: needinfo?(gijskruitbosch+bugs)
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...)
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.
> 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.
(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.
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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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).
This fixes the three test failures for me. Thanks for the suggestions.
Assignee: nobody → jorgk
Attachment #8940265 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8940348 - Flags: review?(kit)
Comment on attachment 8940348 [details] [diff] [review]
1428389-weaveXPCService.patch

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

Thanks!
Attachment #8940348 - Flags: review?(kit) → review+
Component: General → Sync
Product: Thunderbird → Firefox
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537f1536010a
handle case gracefully where weaveXPCService is not available. r=kitcambridge
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/537f1536010a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.