Open Bug 1096922 Opened 6 years ago Updated 7 months ago

search engine cache should be invalidated when removing engines (built-In DuckDuckGo doesn't come back after uninstall of previous DuckDuckGo)

Categories

(Firefox :: Search, defect, P4)

defect

Tracking

()

People

(Reporter: mkaply, Unassigned, Mentored)

References

Details

(Whiteboard: [partnerengine] [fxsearch])

Attachments

(1 file)

If someone has DuckDuckGo installed before Firefox 33.1, then upgrades to Firefox 33.1 and then uninstalls their DuckDuckGo, manage search engines doesn't bring back DDG as a built in engine with restore Defaults.

To recreate.

1. Install Firefox 33.
2. Install DDG here: https://duckduckgo.com/
3. Upgrade to Firefox 33.1
4. Go to search settings and remove DDG and some other engines.
5. Click "Restore Defaults"

What should happen:

The built in Firefox DDG should come back.

Even restarting Firefox does not fix this.
Anything interesting in the Browser Console with browser.search.log? In search-metadata.json, what properties does the DDG engine have? Attaching the whole file here may be good if you don't mind.
Nothing interesting in the console, but the search-metadata is most interesting.

{
    "[app]/google.xml": {
        "order": 1
    },
    "[app]/yahoo.xml": {
        "order": 2
    },
    "[app]/bing.xml": {
        "order": 3
    },
    "[app]/amazondotcom.xml": {
        "order": 4
    },
    "[app]/eBay.xml": {
        "order": 5,
        "hidden": false,
        "alias": null
    },
    "[app]/twitter.xml": {
        "order": 6,
        "hidden": false,
        "alias": null
    },
    "[app]/wikipedia.xml": {
        "order": 7,
        "hidden": false,
        "alias": null
    },
    "[profile]/duckduckgo.xml": {
        "order": 8
    }
}

DuckDuckGo is still stored as the profile version with a different name.

Was any testing done of DDG upgrade scenarios since it already existed in the wild?
I didn't see mentions of QE explicitly testing this scenario but I know the topic of existing DDG search plugins was discussed but this problem didn't come up.

It seems like maybe we aren't invalidating the search-metadata when you remove an engine or click "restore defaults"?
Blocks: 1061736
Can 1061736 be opened now that DDG has shipped?
That was already discussed and it is going to be opened up this week.
What happens here is:
- when you update to Firefox 33.1, we invalidate the cache (due to the update), but we don't load the newly added default engine, because the previously installed (user) copy has the same name and is preferred
- we therefore write the cache out with the user profile version and no default version
- when you remove the user search plugin, we don't invalidate the cache

The cache will be invalidated on the next update. We should probably just invalidate the cache any time user engines are removed.
Flags: firefox-backlog+
Summary: Built-In DuckDuckGo doesn't come back after uninstall of previous DuckDuckGo → search engine cache should be invalidated when removing engines (built-In DuckDuckGo doesn't come back after uninstall of previous DuckDuckGo)
This should just be a matter of adding a call to this._buildCache() to the end of removeEngine:

https://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/toolkit/components/search/nsSearchService.js#l4090
Mentor: gavin.sharp
Whiteboard: [good first bug]
Hope this patch addresses your concerns Gavin.
Assignee: nobody → mv.nsaad
Attachment #8537476 - Flags: review?(gavin.sharp)
Comment on attachment 8537476 [details] [diff] [review]
0001-Bug-1096922-Invalidating-cache-after-search-engine-i.patch

Sorry for the delayed review response, Marcus - I had to find some time to page in some old context here.

Looking into this further, it appears that we do try to invalidate the cache any time an engine is removed, via this code:

https://hg.mozilla.org/mozilla-central/annotate/cb8ad2251c09/toolkit/components/search/nsSearchService.js#l4490

So we probably don't need this patch, but we might need to investigate why that code isn't preventing this bug. It's also possible that it _is_ preventing this bug, after a restart, but comment 0 suggests otherwise. Are you interested in digging into this further?
Flags: needinfo?(mv.nsaad)
Attachment #8537476 - Flags: review?(gavin.sharp)
Gavin, I surely would like to. I'll make sure to fix the bugs I'm already signed to and will come back to this one. Some guidance will be good, since I have no intimacy with this part of the code.
Flags: needinfo?(mv.nsaad)
Priority: -- → P4
Whiteboard: [good first bug] → [good first bug][fxsearch][partnerengine]
Rank: 45
Whiteboard: [good first bug][fxsearch][partnerengine] → [partnerengine][good first bug][fxsearch]
Keywords: good-first-bug
Whiteboard: [partnerengine][good first bug][fxsearch] → [partnerengine] [fxsearch]

With where we are currently heading with search, I'm not sure we'll still need to do this, though we should check at some stage that everything works correctly. As it stands, I think this is no longer a good bug to have as a good-first-bug.

Assignee: mv.nsaad → nobody
Keywords: good-first-bug
You need to log in before you can comment on or make changes to this bug.