Closed Bug 1272322 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit :: Places, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(1 file, 2 obsolete files)

Looks like there's a test failure remaining from the work in bug 1271241.
Flags: needinfo?(frgrahl)
Blocks: 1271241
Blocks: 1271338
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(frgrahl)
Depends on: 1271145
Sorry, I also removed only the files from the installer list. And with test I can't help.
Flags: needinfo?(richard.marti)
No longer depends on: 1271145
Depends on: gloda-autocomplete
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?
(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.
(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.
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.
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.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
No longer depends on: gloda-autocomplete
Keywords: regression
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.
Attachment #8756981 - Flags: review?(mak77) → review+
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
Attachment #8756981 - Flags: review+ → review-
(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?
Component: General → Places
Product: Thunderbird → Toolkit
(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.
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;
});
Attached patch Patch using lazy getter (obsolete) — Splinter Review
Attachment #8757986 - Flags: review?(mak77)
(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).
(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?
> 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
Attachment #8758028 - Flags: review?(mak77)
Attachment #8757986 - Attachment is obsolete: true
Attachment #8757986 - Flags: review?(mak77)
(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 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.
Attachment #8758028 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c7fec8a0c130033c7e8462b058ea360c6d36d9
Bug 1272322 - Ensure PlacesRemoteTabsAutocompleteProvider doesn't fail when sync isn't built. r=mak
https://hg.mozilla.org/mozilla-central/rev/37c7fec8a0c1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: gloda-autocomplete
Attachment #8756981 - Attachment is obsolete: true
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]: -
Attachment #8758028 - Flags: approval-mozilla-beta?
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 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.
Attachment #8758028 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.