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

NEW
Assigned to

Status

()

defect
P4
normal
Rank:
45
5 years ago
Last year

People

(Reporter: mkaply, Assigned: msaad, Mentored)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [partnerengine][good first bug][fxsearch])

Attachments

(1 attachment)

Reporter

Description

5 years ago
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.
Reporter

Comment 2

5 years ago
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"?
Reporter

Comment 4

5 years ago
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]
Assignee

Comment 8

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

Comment 11

5 years ago
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]

Updated

4 years ago
Rank: 45
Whiteboard: [good first bug][fxsearch][partnerengine] → [partnerengine][good first bug][fxsearch]
You need to log in before you can comment on or make changes to this bug.