Closed Bug 1342718 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

51 Branch
All
Android
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 54
Tracking Status
fennec 53+ ---
firefox51 --- wontfix
firefox52 - wontfix
firefox53 + fixed
firefox54 + fixed

People

(Reporter: carlosmelero, Assigned: JanH)

References

Details

Attachments

(1 file)

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.
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).
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: --- → ?
Component: General → Awesomescreen
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
Argh, wrong bug (and I forgot to assign myself here).
Assignee: nobody → jh+bugzilla
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)
I installed the nightly build and sadly I'm still able to see queries being made even with search suggestions disabled.
Flags: needinfo?(carlosmelero)
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!
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)
Okay, this one should hopefully work: https://queue.taskcluster.net/v1/task/RKxrev8VTzyMRcwjegVvdA/runs/0/artifacts/public/build/target.apk
Flags: needinfo?(carlosmelero)
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)
@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.
Now works great :)

Thank you for your work.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
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 → ---
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.
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+
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
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-
https://hg.mozilla.org/mozilla-central/rev/f1b38b06e80c
Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
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+
tracking-fennec: ? → 53+
Depends on: 1359363
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.