Closed
Bug 1342718
Opened 7 years ago
Closed 7 years ago
Search suggestions being queried despite search.suggest.enabled=false
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(fennec53+, firefox51 wontfix, firefox52- wontfix, firefox53+ fixed, firefox54+ fixed)
People
(Reporter: carlosmelero, Assigned: JanH)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
lizzard
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta-
|
Details |
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•7 years 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•7 years 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
Comment hidden (obsolete) |
Assignee | ||
Comment 4•7 years ago
|
||
Argh, wrong bug (and I forgot to assign myself here).
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Now works great :) Thank you for your work.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Comment 15•7 years 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•7 years ago
|
Status: REOPENED → NEW
Comment 16•7 years ago
|
||
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 17•7 years 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•7 years 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•7 years 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 20•7 years ago
|
||
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-
Updated•7 years ago
|
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1b38b06e80c
Status: NEW → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 22•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3dadcef3629a
Updated•7 years ago
|
tracking-fennec: ? → 53+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•