Closed Bug 1482579 Opened Last year Closed Last year

Deprecation warning for search service API from getShortURLForCurrentSearch

Categories

(Firefox :: New Tab Page, defect, P1, blocker)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Iteration:
63.5 - Sep 3
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: Mardak, Assigned: andreio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

DEPRECATION WARNING: Search service falling back to synchronous initialization. This is generally the consequence of an add-on using a deprecated search service API.
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIBrowserSearchService#async_warning
file:///Users/Ed/Development/mozilla-central/obj/dist/Nightly.app/Contents/Resources/components/nsSearchService.js 2621 SRCH_SVC__ensureInitialized
file:///Users/Ed/Development/mozilla-central/obj/dist/Nightly.app/Contents/Resources/components/nsSearchService.js 4033 get currentEngine
resource://activity-stream/lib/TopSitesFeed.jsm 54 getShortURLForCurrentSearch
resource://activity-stream/lib/TopSitesFeed.jsm 78 init
resource://activity-stream/lib/TopSitesFeed.jsm 619 onAction
resource://activity-stream/lib/Store.jsm 51 _middleware/</<
resource://activity-stream/lib/Store.jsm 29 Store/this[method]
resource://activity-stream/lib/Store.jsm 140 init
See Also: → 1482565
Iteration: --- → 63.4 - Aug 20
Priority: -- → P2
There's an initialized notification:

https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/toolkit/components/search/nsSearchService.js#2694-2697

"browser-search-service" / "init-complete"

The deprecation was added several years ago in bug 722332
Depends on: 722332
Interestingly, if I observe the notification, search init happens early enough when restarting.

notify: browser-search-service find-jar-engines
notify: browser-search-service init-complete
action: PREFS_INITIAL_VALUES
action: INIT
action: NEW_TAB_INIT
action: NEW_TAB_LOAD
action: NEW_TAB_INITIAL_STATE
action: NEW_TAB_STATE_REQUEST

However, on a new profile where I believe it's waiting on the search region before completing init, TopSitesFeed will access search from INIT well before the geo is available.

So the message only shows up for new profiles but also means we probably don't actually need the value either.
Priority: P2 → P3
To clarify a bit more about "don't actually need the value either":

Bug 1479478 https://github.com/mozilla/activity-stream/pull/4287 added this code that runs on INIT:

function getShortURLForCurrentSearch() {
  const url = shortURL({url: Services.search.currentEngine.searchForm});

Most likely we won't need to filter out the current search for new profiles as there wouldn't be frecent top sites anyway (unless imported?). So potentially we can lazily access search to cache this._currentSearchHostname and avoid synchronously loading search for new profiles.
Depends on: 1479478
Assignee: nobody → andrei.br92
Just changing the access to a lazy getter isn't enough because our we filter default topsites and accesses that property (still) too soon [0].

But is is correct that the check against `Services.search.currentEngine.searchForm` is not relevant for default topsites on first run? Meaning a new profile, on first run, does not have a default search engine [1] that matches any of our default sites. Maybe we can use this to skip that filter on first run.

Odd that we get default topsites (that depend on geo) but search is still waiting on region, am I missing something?

[0] https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/browser/components/newtab/lib/TopSitesFeed.jsm#239
[1] https://searchfox.org/mozilla-central/source/browser/components/search/searchplugins/list.json
Activity stream continues to load with the default/global top sites if geo is not available. The next refresh will then display the correct defaults.
Assignee: andrei.br92 → nobody
Iteration: 63.4 - Aug 20 → ---
It's necessary to wait for Search init to finish - the sync init flow causes _synchronous disk I/O_ to load search metadata, which blocks _the main thread_. This is very, very bad.
This happens to me each and every startup, so Ed's observation in comment 2 is wrong or things have simply regressed further the last couple of days.

It's quite simple to circumvent this sync init: 

```js
Services.search.init().then(() => Services.search.currentEngine.searchForm);
// or:
await Services.search.init();
Services.search.currentEngine.searchForm;
```

The sync init flow has remained in the product up until now due to addon compat reasons. It will be removed as soon as possible, but in the meantime, please reach out to our team (mkaply, florian, mak, Standard8, adw, Dale, dao, me) whenever a big fat warning like this shows up in the console.

Thanks!
Severity: normal → blocker
Priority: P3 → P1
See Also: → 1485438
Andrei, will you picking this issue back up? I'm afraid that an issue like this will block release...
Flags: needinfo?(edilee)
Flags: needinfo?(andrei.br92)
Yes, I will try to have a patch up today.
Flags: needinfo?(edilee)
Flags: needinfo?(andrei.br92)
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Wait for Search services to initialize before we try to access any properties
Comment on attachment 9003605 [details]
Deprecation warning for search service API from getShortURLForCurrentSearch

Kate Hudson :k88hudson has approved the revision.
Attachment #9003605 - Flags: review+
Comment on attachment 9003605 [details]
Deprecation warning for search service API from getShortURLForCurrentSearch

Approval Request Comment
[Feature/Bug causing the regression]: call to Services.search causes sync disk I/O on startup, blocking the main thread
[User impact if declined]: significant performance impact
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet, awaiting landed 
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: the bug is clearly identified and affect a small code path 
[String changes made/needed]: none
Attachment #9003605 - Flags: approval-mozilla-beta?
Andrei, great job! Thanks ;-)
I really don't like how the github robot resolves bugs before they hit m-c. Do we have any idea when Nightly will be picking up this change for real?
Assignee: nobody → andrei.br92
Flags: needinfo?(khudson)
Target Milestone: --- → Firefox 63
Pushed by khudson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11e6aa3a116b
Deprecation warning for search service API from getShortURLForCurrentSearch r=k88hudson
Sorry about that, there was a problem with authorship info so we had to merge this morning.

I can bring up the resolve after being on master in github v.s. mc v.s. in nightly in our retrospective
Flags: needinfo?(khudson)
Comment on attachment 9003605 [details]
Deprecation warning for search service API from getShortURLForCurrentSearch

Fixes a pretty nasty perf issue. Approving for 62.0 RC1.
Attachment #9003605 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Flags: qe-verify-
Blocks: 1486631
Blocks: 1486909
No longer blocks: 1486909
Depends on: 1486909
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.