1272322 - TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js | singleWordQuery - [singleWordQuery : 241] Got as many results as expected - 1 == 3

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[aleth [:aleth]]

Looks like there's a test failure remaining from the work in bug 1271241.

Comment 1

[Frank-Rainer Grahl (:frg)]

Can't help here much. I only removed the installer line. Will check if the test is used in suite. If the test itself is faulty it might be a toolkit bug.

Comment 2

[Richard Marti (:Paenglab)]

Sorry, I also removed only the files from the installer list. And with test I can't help.

Comment 3

[:aceman]

The test is in unifiedcomplete/ , maybe it is testing the unifiedcomplete functionality that I understood was removed/not included (we have the .js file but not the manifest now) into TB yet?

Comment 4

[aleth [:aleth]]

(In reply to :aceman from comment #3)
> The test is in unifiedcomplete/ , maybe it is testing the unifiedcomplete
> functionality that I understood was removed/not included (we have the .js
> file but not the manifest now) into TB yet?

The error is
"CONSOLE_MESSAGE: (error) [JavaScript Error: "PlacesRemoteTabsAutocompleteProvider is not defined" {file: "resource://gre/components/UnifiedComplete.js" line: 1240}]
17:47:27 INFO - _matchRemoteTabs@resource://gre/components/UnifiedComplete.js:1240:9

TB doesn't have that file because it doesn't ship sync.

Comment 5

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #4)
> TB doesn't have that file because it doesn't ship sync.

Having said that, while this would make sense, I don't quite see why it fails locally, given the moz.build.

Comment 6

[aleth [:aleth]]

MOZ_PLACES is set, and the file is packaged.

The failure is actually this import, https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm#18.

Comment 7

[aleth [:aleth]]

Unifiedcomplete.js should work even if Sync is not built (MOZ_SERVICES_SYNC not set).

One way to ensure this is to ensure PlacesRemoteTabsAutocompleteProvider does not load resource://services-sync URIs unless sync is actually present.

Comment 8

[aleth [:aleth]]

Attachment: Ensure PlacesRemoteTabsAutocompleteProvider doesn't fail when sync isn't built

Comment 9

[aleth [:aleth]]

An alternative would be to add a suitable check to Unifiedcomplete.js, if there's a good way to check if sync is present at runtime.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8756981 [details] [diff] [review]
Ensure PlacesRemoteTabsAutocompleteProvider doesn't fail when sync isn't built

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

oh wait, this wouldn't work cause buildItems tries to use Weave.Service

Comment 11

[aleth [:aleth]]

(In reply to Marco Bonardo [::mak] from comment #10)
> oh wait, this wouldn't work cause buildItems tries to use Weave.Service

That only gets called by getMatches, though?

Comment 12

[Marco Bonardo [:mak]]

(In reply to aleth [:aleth] from comment #11)
> (In reply to Marco Bonardo [::mak] from comment #10)
> > oh wait, this wouldn't work cause buildItems tries to use Weave.Service
> 
> That only gets called by getMatches, though?

yes, but your patch seems to break the case where Sync IS built. Since you are not importing the Weave object into buildItems.
I think you could make "Weave" a global lazy getter that is defined if Sync is build, null otherwise, and null check it.

Comment 13

[Marco Bonardo [:mak]]

Maybe something like this?

XPCOMUtils.defineLazyGetter(this, "Weave", () => {
  try {
    let {Weave} = Cu.import("resource://services-sync/main.js", {});
    return Weave;
  } catch (ex) {
    // The app didn't build Sync.
  }
  return null;
});

Comment 14

[aleth [:aleth]]

Attachment: Patch using lazy getter

Comment 15

[aleth [:aleth]]

(In reply to Marco Bonardo [::mak] from comment #12)
> (In reply to aleth [:aleth] from comment #11)
> > (In reply to Marco Bonardo [::mak] from comment #10)
> > > oh wait, this wouldn't work cause buildItems tries to use Weave.Service
> > 
> > That only gets called by getMatches, though?
> 
> yes, but your patch seems to break the case where Sync IS built. Since you
> are not importing the Weave object into buildItems.

iirc Cu.import always imports into the global object, so it should be fine.

> I think you could make "Weave" a global lazy getter that is defined if Sync
> is build, null otherwise, and null check it.

This is probably cleaner. I added a line that overwrites the getter after the first import, as I suspect otherwise Cu.import has to do actual work every time Weave is accessed (as it's importing into a fresh scope).

Comment 16

[Marco Bonardo [:mak]]

(In reply to aleth [:aleth] from comment #15)
> I added a line that overwrites the getter after
> the first import, as I suspect otherwise Cu.import has to do actual work
> every time Weave is accessed (as it's importing into a fresh scope).

defineLazyGetter should already do that for you, the function returned value replaces the getter. Am I missing something?

Comment 17

[Philip Chee]

> iirc Cu.import always imports into the global object, so it should be fine.

No it doesn't always import into the global object. It only imports into the global object if you don't specify a different context.

> This is probably cleaner. I added a line that overwrites the getter after
> the first import, as I suspect otherwise Cu.import has to do actual work
> every time Weave is accessed (as it's importing into a fresh scope).
That's what "defineLazyGetter" already does!
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyGetter%28%29

Comment 18

[aleth [:aleth]]

Attachment: Patch using lazy getter

Comment 19

[aleth [:aleth]]

(In reply to Marco Bonardo [::mak] from comment #16)
> (In reply to aleth [:aleth] from comment #15)
> > I added a line that overwrites the getter after
> > the first import, as I suspect otherwise Cu.import has to do actual work
> > every time Weave is accessed (as it's importing into a fresh scope).
> 
> defineLazyGetter should already do that for you, the function returned value
> replaces the getter. Am I missing something?

Right, thanks!

(In reply to Philip Chee from comment #17)
> > iirc Cu.import always imports into the global object, so it should be fine.
> 
> No it doesn't always import into the global object. It only imports into the
> global object if you don't specify a different context.

The original patch didn't specify a different context, as far as I can tell.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8758028 [details] [diff] [review]
Patch using lazy getter

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

Thanks!

you are right that the previous solution would have probably worked, but I feel like this is more explicit.

Comment 21

[aleth [:aleth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37c7fec8a0c130033c7e8462b058ea360c6d36d9
Bug 1272322 - Ensure PlacesRemoteTabsAutocompleteProvider doesn't fail when sync isn't built. r=mak

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37c7fec8a0c1

Comment 24

[Intermittent Failures Robot]

8 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 8

Platform breakdown:
* windowsxp: 3
* windows7-32: 3
* osx-10-10: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1272322&startday=2016-05-30&endday=2016-06-05&tree=all

Comment 25

[aleth [:aleth]]

Comment on attachment 8758028 [details] [diff] [review]
Patch using lazy getter

Approval Request Comment
[Feature/regressing bug #]: Since Bug 1279425 landed on mozilla-beta, these changes are 
[User impact if declined]: Search errors on builds without MOZ_SERVICES_SYNC set
[Describe test coverage new/current, TreeHerder]: This should fix c-a test failures
[Risks and why]: Low - just adds a check whether Weave exists
[String/UUID change made/needed]: -

Comment 26

[Jorg K (CEST = GMT+2)]

Aleth meant to say:
[Feature/regressing bug #]: Since Bug 1279425 landed on mozilla-beta, these changes here are also needed on m-b (mozilla48).
[Describe test coverage new/current, TreeHerder]: This should fix c-beta test failures, for example:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&selectedJob=8229

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8758028 [details] [diff] [review]
Patch using lazy getter

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

This patch fixes a regression. Take it in 48 beta 6.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc80bc0bb6a7