"'Restore defaults" option does not bring back removed search engines

VERIFIED FIXED in Firefox 45

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: TeoVermesan, Assigned: florian)

Tracking

(Depends on: 1 bug, {regression})

45 Branch
Firefox 45
ARM
Android
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45+ verified, fennec45+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Steps to reproduce:
1. Go to Menu -> Settings -> Customize -> Search
2. Tap on a default search engine and choose "Remove"
3. Tap the three-dots-menu from the top right and choose "Restore defaults"

Expected results:
- The search engine removed from step 2 is re-added to the "Installed search engines" list

Actual results:
- The search engine removed from step 2 is not added to the list

Note:
-regression:
good build: 30-10
bad build: 31-10
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b41b92c09fcf94d077a54297aea1dc675b161a9d&tochange=c1a94d5365e48f86de6a7bb6a3c4c7d82b9d660e

Bug 1219416 - Display "Search" title when opening search preferences with the magnifying glass ?
[Tracking Requested - why for this release]: regression, affects 44 since 1219416 was uplifted.
status-firefox44: unaffected → affected
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
Blocks: 1219416
I don't think this is related to Bug 1219416. It only changes the title to the value it should have. Did a backout without getting any improvements.
Flags: needinfo?(margaret.leibovic)

Comment 3

3 years ago
I suspect this may have been caused by bug 1203167, which is also in that regression range.
Flags: needinfo?(margaret.leibovic) → needinfo?(florian)
(Assignee)

Comment 4

3 years ago
Is this android-specific?
Andrei would someone on the SV desktop team have time to check out desktop behavior.
Flags: needinfo?(andrei.vaida)
(Assignee)

Comment 6

3 years ago
I looked quickly, it seems Android uses Services.search.restoreDefaultEngines(); (at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6869) which the desktop preference UI doesn't use for some reason, so restoreDefaultEngines may be slightly broken in the search service (I thought it was covered by automated tests though :-/). I'll investigate a bit tomorrow.

Updated

3 years ago
Blocks: 1203167
No longer blocks: 1219416
(Assignee)

Comment 7

3 years ago
Services.search.restoreDefaultEngines(); works fine on desktop. And I don't see anything obviously broken in it by code inspection. If someone can use a debugger to step through this method while running on android, it would be nice to check that _isDefault is returning the expected value (true).
Flags: needinfo?(florian)
(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> Andrei would someone on the SV desktop team have time to check out desktop
> behavior.

Petruta will follow up on this matter as soon as possible.
Flags: needinfo?(andrei.vaida)
Search engines are correctly restored with desktop Nightly build 45.0a1 20151104030437 under Win 7 64-bit and Mac OS X 10.9.5.

Comment 10

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Services.search.restoreDefaultEngines(); works fine on desktop. And I don't
> see anything obviously broken in it by code inspection. If someone can use a
> debugger to step through this method while running on android, it would be
> nice to check that _isDefault is returning the expected value (true).

I attached a debugger, and I found that _isDefault actually returning false here:
http://hg.mozilla.org/mozilla-central/file/59c648a3f955/toolkit/components/search/nsSearchService.js#l4053

I stepped through the _isDefault implementation, and found we're returning false here:
http://hg.mozilla.org/mozilla-central/file/59c648a3f955/toolkit/components/search/nsSearchService.js#l2298

Does that give you enough to figure out what's going wrong? Let me know if I can help debug more.
Flags: needinfo?(florian)
(Assignee)

Comment 11

3 years ago
We should be returning true at http://hg.mozilla.org/mozilla-central/file/59c648a3f955/toolkit/components/search/nsSearchService.js#l2291
Could you print the value of this._loadPath?
Flags: needinfo?(florian)

Comment 12

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> We should be returning true at
> http://hg.mozilla.org/mozilla-central/file/59c648a3f955/toolkit/components/
> search/nsSearchService.js#l2291
> Could you print the value of this._loadPath?

"jar:[other]/base.apk!browser/google.xml"
(Assignee)

Comment 13

3 years ago
Is base.apk an archive that contains the omni.ja file, or is the packaging of chrome files significantly different on android? If so we should change the code in the getAnonymizedLoadPath method so that it returns something looking like jar:[app]/omni.ja!browser/google.xml for that case.

Currently the check to detect the app folder is done by comparing with NS_XPCOM_CURRENT_PROCESS_DIR.

Comment 14

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> Is base.apk an archive that contains the omni.ja file, or is the packaging
> of chrome files significantly different on android? If so we should change
> the code in the getAnonymizedLoadPath method so that it returns something
> looking like jar:[app]/omni.ja!browser/google.xml for that case.
> 
> Currently the check to detect the app folder is done by comparing with
> NS_XPCOM_CURRENT_PROCESS_DIR.

Nick, can you help answer this question?
tracking-fennec: --- → ?
Flags: needinfo?(nalexander)
(In reply to :Margaret Leibovic from comment #14)
> (In reply to Florian Quèze [:florian] [:flo] from comment #13)
> > Is base.apk an archive that contains the omni.ja file, or is the packaging
> > of chrome files significantly different on android? If so we should change
> > the code in the getAnonymizedLoadPath method so that it returns something
> > looking like jar:[app]/omni.ja!browser/google.xml for that case.
> > 
> > Currently the check to detect the app folder is done by comparing with
> > NS_XPCOM_CURRENT_PROCESS_DIR.
> 
> Nick, can you help answer this question?

I'm having trouble following what's actually happening here, but my guess is that this code is not accounting for the APK file name being in the path to omni.ja, or is not  accounting for OMNIJAR_NAME being special on Android (see https://dxr.mozilla.org/mozilla-central/source/configure.in#7835).

It's not easy to pin down the omni.ja location on disk, 'cuz it varies by APK install: see https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoJarReader.java#249 for what we do on the Java side.  As an example, you might see a path like jar:jar:file:///data/app/org.mozilla.fennec.apk!/omni.ja!/chrome/chrome/content/branding/favicon32.png.

Stepping back, is the goal to save something *outside* of the current profile directory?
Flags: needinfo?(nalexander)

Updated

3 years ago
Flags: needinfo?(florian)
(Assignee)

Comment 16

3 years ago
(In reply to Nick Alexander :nalexander from comment #15)

> Stepping back, is the goal to save something *outside* of the current
> profile directory?

The goal is to identify reliably where search plugins are being loaded from. Identifying exactly the omni.ja file isn't too critical, but it's important to know that the file is coming from something that's part of the application, ie. something we ship, rather than a file that has somehow been dropped on the filesystem.

I said in comment 13 that the loadPath should look like jar:[app]/omni.ja!browser/google.xml
It doesn't have to be exactly that string. If it is jar:[app]/base.apk!browser/google.xml on Android, that's fine. The important part is having the '[app]' prefix instead of the '[other]' currently returned on Android.
Flags: needinfo?(florian)
It possible that getAnonymizedLoadPath comparing file paths with startsWith rather than checking contains() is problematic on Android?

(It is problematic that it uses nsIFile at all, too.)
Spent some time debugging this. On my Fennec Nightly build, I get these values:

> Services.dirsvc.get("XCurProcD", Ci.nsIFile).path:

  "/data/data/org.mozilla.fennec"

> Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIChromeRegistry).convertChromeURL(NetUtil.newURI("chrome://browser/locale/searchplugins/bing.xml")).QueryInterface(Ci.nsINestedURI).innermostURI.QueryInterface(Ci.nsIFileURL).file.path:

  "/data/app/org.mozilla.fennec-2/base.apk"

Since the latter doesn't start with the former, _loadPath gets an [other] value.
The old code just assumed that any JAR engine was a default engine, which seems simpler than trying to come up with a list of all "app paths". I suppose getAnonymizedLoadPath could be adjusted to do something similar. But then you'd also need to cause a re-load of engines somehow so that their cached _loadPaths get refreshed. I guess that might still be possible by bumping CACHE_VERSION?
(Assignee)

Comment 20

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> The old code just assumed that any JAR engine was a default engine

That meant something shipped in an add-on could be reported as default.

> which
> seems simpler than trying to come up with a list of all "app paths".

The simplest solution for this bug (with its steps to reproduce) would be to remove the _isDefault check altogether, as I don't see any harm in setting hidden = false on all engines; user-installed engines are removed rather then hidden.

I think _isDefault being broken on Android isn't ok though, as that would also break MozParam there.

> you'd also need to cause a re-load of engines somehow so that their
> cached _loadPaths get refreshed. I guess that might still be possible by
> bumping CACHE_VERSION?

A change of the buildid should be enough to trigger a cache refresh. Only engines with readonly=false will be kept unmodified.


(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #18)

>   "/data/app/org.mozilla.fennec-2/base.apk"

I wouldn't mind an ifdef android that checked for the '/data/app/org.mozilla.' prefix, and/or a check that the path contains 'base.apk'. Then the loadPath prefix can be set to [app].
(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> > The old code just assumed that any JAR engine was a default engine
> 
> That meant something shipped in an add-on could be reported as default.

Yes, intentionally. For the purpose of "default" then it didn't really matter (and was actually useful, for e.g. enterprise-deployed add-ons).

> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #18)
> 
> >   "/data/app/org.mozilla.fennec-2/base.apk"
> 
> I wouldn't mind an ifdef android that checked for the
> '/data/app/org.mozilla.' prefix, and/or a check that the path contains
> 'base.apk'. Then the loadPath prefix can be set to [app].

This seems fragile and overly complicated.

Comment 22

3 years ago
Florian, can you own this bug? Do you need more help from the Android team to fix this?
Flags: needinfo?(florian)

Updated

3 years ago
Assignee: nobody → florian
tracking-fennec: ? → 44+
(Assignee)

Comment 23

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21)
> (In reply to Florian Quèze [:florian] [:flo] from comment #20)
> > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> > > The old code just assumed that any JAR engine was a default engine
> > 
> > That meant something shipped in an add-on could be reported as default.
> 
> Yes, intentionally. For the purpose of "default" then it didn't really
> matter (and was actually useful, for e.g. enterprise-deployed add-ons).

I don't think this was used. We broke it a while ago when we removed the browser.search.jarURIs pref in bug 1169459 and I haven't seen any complaint.

Unless bug 598697 gets fixed, we don't support search plugins inside packed add-ons, so we can't have a 'jar engine' due to loading it from an xpi file.

> > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #18)
> > 
> > >   "/data/app/org.mozilla.fennec-2/base.apk"
> > 
> > I wouldn't mind an ifdef android that checked for the
> > '/data/app/org.mozilla.' prefix, and/or a check that the path contains
> > 'base.apk'. Then the loadPath prefix can be set to [app].
> 
> This seems fragile and overly complicated.

Which part of this seems fragile?
(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #20)
> > > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> > > > The old code just assumed that any JAR engine was a default engine
> > > 
> > > That meant something shipped in an add-on could be reported as default.
> > 
> > Yes, intentionally. For the purpose of "default" then it didn't really
> > matter (and was actually useful, for e.g. enterprise-deployed add-ons).
> 
> I don't think this was used. We broke it a while ago when we removed the
> browser.search.jarURIs pref in bug 1169459 and I haven't seen any complaint.

No, I meant that administrators that used system add-ons to ship default search engines could benefit from them being treated as "default". Nothing to do with JAR engines. It's certainly not a common use case or large benefit, but it's definitely not a problem, and it leads to simpler code.
Sorry, I see now that you were referring to add-on shipped JAR engines, as opposed to add-on shipped non-JAR engines. In any case, my point is that it doesn't matter if add-on shipped engines (regardless of technique used) get treated as default engines, so rather than complicating the code to avoid it, you should just go back to the older model for determining "defaultness".
(Assignee)

Comment 26

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #25)

> In any case, my point is that it
> doesn't matter if add-on shipped engines (regardless of technique used) get
> treated as default engines

I wouldn't want this test to pass for an add-on shipped engines: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=2d9c58098eb3#4112
But I guess I could make that specific test check the _loadPath value instead of _isDefault.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=2d9c58098eb3#4189 probably shouldn't pass for add-on shipped engines either, as these are user-installed.

> you should just go back to the older model for determining "defaultness".

The old implementation isn't something I would consider going back to, but we could probably get pretty close by using the _readOnly field.
I'm not talking about any specific implementations, I'm talking about the general approach. Look at it this way: it's easy to record and persist the origin of the search plugin when you're actually loading it. If you do that, it's easy to later answer "was it shipped with Firefox?", without having a method that tries to determine a "load path" after the fact based solely on whatever artifacts of the load process remain (file path, URI, etc.), and then having to parse that "load path" with a regex.
(Assignee)

Comment 28

2 years ago
Created attachment 8690059 [details] [diff] [review]
Fix the _loadPath on Android

I tried to change the _isDefault implementation and didn't get quickly to a working patch. I'll file a separate bug to attach my WIP and so that we can discuss whether this change needs to happen or not.

It doesn't change that the loadPath shouldn't be broken on Android, and that fixing that (what the patch I'm attaching does) is enough for bug initially reported here to go away.
Flags: needinfo?(florian)
Attachment #8690059 - Flags: review?(mak77)
(Assignee)

Comment 29

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #28)

> I tried to change the _isDefault implementation and didn't get quickly to a
> working patch. I'll file a separate bug to attach my WIP and so that we can
> discuss whether this change needs to happen or not.

Filed bug 1226595.
This is a basic scenario that should be working in 44. Tracked.
tracking-firefox44: ? → +
Comment on attachment 8690059 [details] [diff] [review]
Fix the _loadPath on Android

Review of attachment 8690059 [details] [diff] [review]:
-----------------------------------------------------------------

I can't test the change, but it looks sane by code inspection.
The fragility here is mostly bound to the Android setSubmission call, if anyone should change that, this will break. That makes me think this needs either a test in mobile or a more robust future plan. Sounds like there's a plan that still needs some refinement, so we can proceed.

::: toolkit/components/search/nsSearchService.js
@@ +2251,5 @@
> +      if (appPath) {
> +        appPath = appPath.spec;
> +        let spec = uri.spec;
> +        if (spec.includes(appPath)) {
> +          let appURI = Services.io.newFileURI(getDir(knownDirs["app"]));

nit: NetUtil.newURI automatically calls newFileURI for nsIFile instances, so you could just use that.
Attachment #8690059 - Flags: review?(mak77) → review+
(Assignee)

Updated

2 years ago
Depends on: 1228319
(Assignee)

Comment 33

2 years ago
The regression is from bug 1203167 which landed on 45 and wasn't uplifted; removing unneeded tracking flags.
tracking-fennec: 44+ → ---
status-firefox44: affected → unaffected
tracking-firefox44: + → ---
(Assignee)

Comment 35

2 years ago
Comment on attachment 8690059 [details] [diff] [review]
Fix the _loadPath on Android

Unfortunately, this is breaking the toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js test on Android 2.3 API9 and Android 4.2 x86 https://treeherder.mozilla.org/#/jobs?repo=try&revision=00800af20265

Updated

2 years ago
tracking-fennec: --- → 45+

Comment 36

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> Comment on attachment 8690059 [details] [diff] [review]
> Fix the _loadPath on Android
> 
> Unfortunately, this is breaking the
> toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js test on
> Android 2.3 API9 and Android 4.2 x86
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=00800af20265

I didn't notice this comment! I want to land a fix here. Can we just disable this test on Android?

We're not currently using TelemetryEnvironment to send telemetry data for your default search engine on Android, so I don't think this bustage actually matters for us in practice. Also, it's strange that this failure only started with this patch.
Flags: needinfo?(florian)

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6a0a7cfd581
https://hg.mozilla.org/mozilla-central/rev/575efef2e8cd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Reporter)

Comment 40

2 years ago
Search engines removed from the list are re-added after choosing "Restore defaults" option, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0) and ZTE x86 (Android 4.0.4)
Build: Firefox for Android 46.0a1 (2015-12-17) and 45.0a2 (2015-12-14)
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
tracking-firefox45: ? → +
(Assignee)

Comment 41

2 years ago
(In reply to :Margaret Leibovic from comment #36)
> (In reply to Florian Quèze [:florian] [:flo] from comment #35)
> > Comment on attachment 8690059 [details] [diff] [review]
> > Fix the _loadPath on Android
> > 
> > Unfortunately, this is breaking the
> > toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js test on
> > Android 2.3 API9 and Android 4.2 x86
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=00800af20265
> 
> I didn't notice this comment! I want to land a fix here. Can we just disable
> this test on Android?

I forgot to clear the needinfo as we discussed this in person in Orlando rather than on bugzilla.
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.