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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

({intermittent-failure, regression})

49 Branch
mozilla49
intermittent-failure, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Looks like there's a test failure remaining from the work in bug 1271241.
(Assignee)

Updated

2 years ago
Flags: needinfo?(frgrahl)
(Assignee)

Updated

2 years ago
Blocks: 1271241
(Assignee)

Updated

2 years ago
Blocks: 1271338
(Assignee)

Updated

2 years ago
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)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Updated

2 years ago
No longer depends on: 1271145
(Assignee)

Updated

2 years ago
Depends on: 1271445

Comment 3

2 years ago
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?
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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)

Comment 8

2 years ago
Created attachment 8756981 [details] [diff] [review]
Ensure PlacesRemoteTabsAutocompleteProvider doesn't fail when sync isn't built
Attachment #8756981 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
No longer depends on: 1271445
Keywords: regression
(Assignee)

Updated

2 years ago
Blocks: 1271445
(Assignee)

Comment 9

2 years ago
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.

Updated

2 years ago
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-
(Assignee)

Comment 11

2 years ago
(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?
(Assignee)

Updated

2 years ago
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;
});
(Assignee)

Comment 14

2 years ago
Created attachment 8757986 [details] [diff] [review]
Patch using lazy getter
Attachment #8757986 - Flags: review?(mak77)
(Assignee)

Comment 15

2 years ago
(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?

Comment 17

2 years ago
> 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
(Assignee)

Comment 18

2 years ago
Created attachment 8758028 [details] [diff] [review]
Patch using lazy getter
Attachment #8758028 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Attachment #8757986 - Attachment is obsolete: true
Attachment #8757986 - Flags: review?(mak77)
(Assignee)

Comment 19

2 years ago
(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+
(Assignee)

Comment 21

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37c7fec8a0c1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1271445
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
(Assignee)

Updated

2 years ago
Attachment #8756981 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
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?

Comment 26

2 years ago
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

Updated

2 years ago
status-firefox48: --- → affected
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+

Comment 28

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cc80bc0bb6a7
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.