Search suggestions being queried despite search.suggest.enabled=false

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: carlosmelero, Assigned: JanH)

Tracking

(Depends on: 1 bug)

51 Branch
Firefox 54
All
Android
Points:
---

Firefox Tracking Flags

(fennec53+, firefox51 wontfix, firefox52- wontfix, firefox53+ fixed, firefox54+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161213225041

Steps to reproduce:

- Prepare mitm to monitor traffic
- Set search.suggest.enabled = false
- Write something in the search bar


Actual results:

- No suggestions being shown
- Suggestions being queried


Expected results:

- No suggestions being shown
- No queries.
(Reporter)

Comment 1

a year ago
Reviewing the source code you can see that in SearchEngineRow.updateSuggestions (https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java#l384) the searchSuggestionsEnabled is checked after calling searchEngine.getSuggestions() (https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java#l411).
(Assignee)

Comment 2

a year ago
Ooops, that shouldn't happen of course. Thanks for figuring this out.

[Tracking Requested - why for this release]:
We're apparently still querying search engine suggestion even if the user disabled them.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Component: General → Awesomescreen
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
(Assignee)

Comment 4

a year ago
Argh, wrong bug (and I forgot to assign myself here).
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
Can you confirm that this build (https://queue.taskcluster.net/v1/task/GEqzACs4TjeFjNx4yeE2Hw/runs/0/artifacts/public/build/target.apk) works as expected?
Flags: needinfo?(carlosmelero)
(Reporter)

Comment 7

a year ago
I installed the nightly build and sadly I'm still able to see queries being made even with search suggestions disabled.
Flags: needinfo?(carlosmelero)
(Reporter)

Comment 8

a year ago
Reading more into the source code I can see that getSuggestions() function is just a wrapper that retrieves the suggestions that have been saved asynchronously (?). Don't have much time now to track where they're being saved.

By the way, thank you for the quick response!
(Assignee)

Comment 9

a year ago
Comment on attachment 8841330 [details]
Bug 1342718 - Don't query for search engine suggestions if we're not displaying them.

Right, so this bit needs changing anyway, but apparently the search suggestion are gathered from somewhere else.

@carlos:
Thanks for giving this a try and no problem - I'll look into it further.
Attachment #8841330 - Flags: review?(s.kaspari)
(Assignee)

Comment 10

a year ago
Okay, this one should hopefully work: https://queue.taskcluster.net/v1/task/RKxrev8VTzyMRcwjegVvdA/runs/0/artifacts/public/build/target.apk
Flags: needinfo?(carlosmelero)
(Reporter)

Comment 11

a year ago
Yes, now it works as expected by default.

However I've noticed that after enabling and disabling there are few queries that get leaked. If you have suggestions enabled and then disable them there are a few queries that get sent regardless. I guess it hsa to do with how often suggestion settings are read.
Flags: needinfo?(carlosmelero)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
@carlosmelero

Third time lucky, I hope :-)
https://queue.taskcluster.net/v1/task/baQvKjSxRjCBKle_E0VSaQ/runs/0/artifacts/public/build/target.apk

As long as you use the normal preferences menu instead of about:config, the changed setting should take effect immediately now.
(Reporter)

Comment 14

a year ago
Now works great :)

Thank you for your work.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 15

a year ago
Hold it ;-) - this'll be marked fixed when the patch actually lands. What you've tested was only a custom try build.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Updated

a year ago
Status: REOPENED → NEW
Tracking 53/54 for this regression since we should respect what the user does. Not sure whether this is worth tracking for 52 since we are at the end of the cycle.
tracking-firefox53: ? → +
tracking-firefox54: ? → +

Comment 17

a year ago
mozreview-review
Comment on attachment 8841330 [details]
Bug 1342718 - Don't query for search engine suggestions if we're not displaying them.

https://reviewboard.mozilla.org/r/115580/#review117772
Attachment #8841330 - Flags: review?(s.kaspari) → review+

Comment 18

a year ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/f1b38b06e80c
Don't query for search engine suggestions if we're not displaying them. r=sebastian
(Assignee)

Comment 19

a year ago
Comment on attachment 8841330 [details]
Bug 1342718 - Don't query for search engine suggestions if we're not displaying them.

Approval Request Comment
[Feature/Bug causing the regression]: Awesomebar search
[User impact if declined]: If the user has deactivated live search suggestions from the search engine but not suggestions based on search history, then any search terms entered will be still transmitted to the selected search engine even though the resulting suggestions won't be shown. Also, the display of local search history results may be incomplete in this case.
[Is this code covered by automated tests?]: Partially.
[Has the fix been verified in Nightly?]: Verified by reporter on a try build incorporating this patch.
[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?]: No major changes - we already skip getting this data in private mode or if both live *and* search history suggestions are disabled - this patch skips them individually if *either* live or search history suggestions have been disabled.
[String changes made/needed]: none
Attachment #8841330 - Flags: approval-mozilla-beta?
Attachment #8841330 - Flags: approval-mozilla-aurora?
Comment on attachment 8841330 [details]
Bug 1342718 - Don't query for search engine suggestions if we're not displaying them.

Too late for 52, sorry.
Attachment #8841330 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox52: affected → wontfix
tracking-firefox52: ? → -

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1b38b06e80c
Status: NEW → RESOLVED
Last Resolved: a year agoa year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8841330 [details]
Bug 1342718 - Don't query for search engine suggestions if we're not displaying them.

Better behavior for search privacy and perf, let's take this in aurora 53.
Attachment #8841330 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3dadcef3629a
status-firefox53: affected → fixed
tracking-fennec: ? → 53+
(Assignee)

Updated

a year ago
Depends on: 1359363
You need to log in before you can comment on or make changes to this bug.