Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug]Wrong default search engine displayed on startup #11875

Closed
AndiAJ opened this issue Jun 24, 2020 · 23 comments
Closed

[Bug]Wrong default search engine displayed on startup #11875

AndiAJ opened this issue Jun 24, 2020 · 23 comments
Assignees
Labels
🐞 bug Crashes, Something isn't working, .. E5 Estimation Point: about 5 days eng:qa:verified QA Verified Feature:HomeScreen Feature:Search Feature:Toolbar Address bar, see also Feature:Search Release Blocker Blocks a Release S1 Blocks development/testing, may impact more than 25% of users, causes data loss, potential chemspill

Comments

@AndiAJ
Copy link
Collaborator

AndiAJ commented Jun 24, 2020

Prerequisites

Have a fresh install or a clean profile
Have the Language and input setting set to English - US

Steps to reproduce

  1. Launch Fenix
  2. Check the default search engine icon displayed in Nav bar

Expected behavior

Google's icon should be displayed in the Nav bar

Actual behavior

Amazon's icon is displayed
After you tap "Start browsing" and dismiss the search view the correct default search engine is displayed

Device information

  • Android device:
    • Google Pixel 3a (Android 10)
    • OnePlus A3 (Android 6.0.1)
  • Fenix version: Nightly Build 200624 from 6/24

Notes

► Video
20200624-093243

┆Issue is synchronized with this Jira Task

@AndiAJ AndiAJ added 🐞 bug Crashes, Something isn't working, .. S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist Feature:HomeScreen Feature:Toolbar Address bar, see also Feature:Search Feature:Search labels Jun 24, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Jun 24, 2020
@AndiAJ
Copy link
Collaborator Author

AndiAJ commented Jul 1, 2020

The Amazon engine is set after force close + resume on 79.0.0-beta.3 using the following devices:
• Google Pixel 3a (Android 10)
• OnePlus A3 (Android 6.0.1)

► Video
20200701-112841

@sv-ohorvath
Copy link
Contributor

sv-ohorvath commented Jul 1, 2020

I didn't only see the amazon icon in the search bar but also had it set as default and Google still missing from the engines list. Once I've done a search using amazon, it stays as default even after another restart. Not 100% reproducible, maybe 1/6.
So I'd say this is still the same as #12081
device: Pixel 4 (Android 10)

@sv-ohorvath sv-ohorvath added S1 Blocks development/testing, may impact more than 25% of users, causes data loss, potential chemspill and removed S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist labels Jul 1, 2020
@liuche
Copy link
Contributor

liuche commented Jul 1, 2020

(adding these labels for tracking, but if this is an edge case, we may skip it for a release blocker, and only do this in the fast-follow release that is this current sprint)

@liuche
Copy link
Contributor

liuche commented Jul 2, 2020

Hmm, seems like it could be a timing issue. Does this still happen if the user doesn't restart immediately after? I think @boek investigated it briefly.

Is this something that we could retest on the 79-beta.4 build?

What do you think, @vesta0 ?

@liuche
Copy link
Contributor

liuche commented Jul 2, 2020

@Mugurell is this something you and your team could look at during your day?

@AndiAJ
Copy link
Collaborator Author

AndiAJ commented Jul 2, 2020

Hi @liuche I've just re-checked this on 79.0.0-beta.4 from 7/1.

✔️ Can't reproduce on: Huawei Mate 20 Lite (Android 9)
❌ Reproducible on :
• Google Pixel 3a (Android 10)
• OnePlus A3 (Android 6.0.1)

Each time the user opens Fenix after previously closing it Amazon will get displayed.
► Video
20200702-094357

Deleting Amazon from the Search engine will display Bing (and so on) even if the default search engine is set to Google
► Video
20200702-094641

@pocmo
Copy link
Contributor

pocmo commented Jul 2, 2020

I also see this in Nightly for every fresh app start.

@Mugurell
Copy link
Contributor

Mugurell commented Jul 2, 2020

This is really interesting.
Indeed, it happens on Nightly, but we couldn't get it to reproduce on the same device(and configuration) while using a debug build.
@boek Seeing you worked on this recently, can you help tracking down the issue?

@sv-ohorvath
Copy link
Contributor

sv-ohorvath commented Jul 2, 2020

So it looks like there is a difference between the Firefox Preview Beta (PS build) and Fenix.fennec-beta (migration) builds:

Scenario 1:

  • on the PlayStore build, the Amazon icon is just displayed at every fresh start, then it goes away (what happens on Nightly as well).
  • this doesn't happen on the migration build.

Scenario 2:

  • on the fennec-beta migration build, for a first-time install (no fennec app installed previously): if you kill the app while you are on the Fenix onboarding page and then restart, Google is not in the default engines list and Amazon is set as default. => so it's an edge case for first-time users only.
  • this happens on the PS Firefox Preview Beta build, but only after you press Start browsing after the restart...

*If you had Fennec installed and migrate to Fenix, this doesn't happen.

@Mugurell
Copy link
Contributor

Mugurell commented Jul 2, 2020

This is really interesting.
Indeed, it happens on Nightly, but we couldn't get it to reproduce on the same device(and configuration) while using a debug build.

I can reproduce the same behavior as for Nightly if configuring the debug build to use the same MLS here so the issue may be related to the region/country/language MLS returns or how we use that data.

@Mugurell
Copy link
Contributor

Mugurell commented Jul 2, 2020

The issue is that refreshAsync() takes too much while based on MLS we get the location aware identifiers early.
-> we get to filter fallback engines by location identifiers.
-> google-b-1-m which is only available for US is filtered out for other regions

We could come up with a simple patch to also use the fallback identifiers..
but I'm be more interested in reducing the time refreshAsync() takes to complete or somehow synchronize the code/calls/ui updates so that we won't be showing the fallback search engines too often.

@liuche
Copy link
Contributor

liuche commented Jul 2, 2020

We talked this over and decided that we can ship this to Beta (so that we can get the perf experiment #7795 running), but want it to be fixed for Release, so we can take it as an uplift.

@Mugurell Mugurell self-assigned this Jul 3, 2020
@Mugurell
Copy link
Contributor

Mugurell commented Jul 3, 2020

One important detail is that this issue is possible due to Google having it's identifier overridden for US to be "google-b-1-m"
If using the fallback engine identifiers (on a device using English-US) but with another region (based on MLS) based engines which for Google probably use "google-b-m" the Google search engine will be filtered out.

Thought I'd be able to sync the various calls but this leads to a longer startup time, something that we wanted to avoid in the first place - (see #9935)
To avoid the issue from #9935 we are trying to be as fast as possible and use many various conditions to decide whether to use the fallback engines / identifiers or not.
"Synchronizing" the behavior on more such checks leads many times to improper combinations of engines/identifiers and even allows for the fallbackEngines to be saved in the proper localeAwareInstalledEnginesKey and so perpetually have the wrong set of engines (if loadedRegion.isCompleted and baseSearchEngines.isCompleted.not()) for the app. (Users can of course still update the engines)

What I suggest (based on Sebastian's comment - #9935 (comment)) is to:

  • accept that at the first app start (ever) we will have to probably show the fallback engines.
  • refactor our code to choose whether to show the fallback or not engines based on a single check (used for the above) and so avoid having mix-matched engines and identifiers.
  • even if showing the fallback engines, continue in the background to compute the region based list and persist that for the entire Fenix's lifetime.
  • refactor our code so that at the next app start MLS is not used anymore. We already have persisted the region based engines, no need to redo this long time operation. Avoiding having to wait for MLS again and reading the already persisted engines means a faster startup and no need for other complex synchronizations.

Curious about your thoughts @boek

@Mugurell
Copy link
Contributor

This should be ready to test in the next Nightly.

pocmo pushed a commit that referenced this issue Jul 14, 2020
We have two search engine types:
- one based on MLS reported region,
- one based only on Locale.

There are multiple steps involved in returning the default search engine for
example and though at each step we could verify if a certain operation is
completed we are still exposed to concurrency issues.
Simplest and most effective way to make sure the MLS engines do not mix with
Locale based engines is to use the same type of engines for the entire duration
of the app. At the next cold start we'll verify again which engines to use.

Using the Locale based engines (fallbacks) is expected to only happen once, at
the first run of the application after being installed.
@DmitriyFrogo
Copy link

DmitriyFrogo commented Jul 14, 2020

Now, when I add custom DuckDuckGo search, it won't appears until browser restart

Steps:

  1. Clean install
  2. Add DDG with russian locale
  3. "DDG added" hint appears, but no new engine added
  4. Restart Fenix - DDG appears in list

Hmmm... but

  1. Clean install
  2. Restart Fenix
  3. Add DDG and it's appears in list

@Mugurell
Copy link
Contributor

Thank you. Looking into this.

@sv-ohorvath
Copy link
Contributor

sv-ohorvath commented Jul 14, 2020

@Mugurell There is also this issue on 79.0-beta6:

  1. The device language is Turkish or Russian.
  2. Clean install.
  3. On the onboarding screen & first run, until restart, Yandex is the default search engine.
  4. Restart the app and then Google is the default engine.

The difference between beta 5 & 6 is that before, the Yandex logo was only shown on the onboarding screen and disappeared when you tapped the search bar.

On EN-US I didn't see any issues.

@sv-ohorvath
Copy link
Contributor

Talked with @Mugurell and decided to create a new issue for the remaining bugs since this was already merged: #12544
So I'll close this one.

@sv-ohorvath sv-ohorvath added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Jul 14, 2020
@BranescuMihai BranescuMihai added the E5 Estimation Point: about 5 days label Jul 20, 2020
@data-sync-user data-sync-user changed the title [Bug]Wrong default search engine displayed on startup FNX3-14973 ⁃ [Bug]Wrong default search engine displayed on startup Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX3-14973 ⁃ [Bug]Wrong default search engine displayed on startup FNX-12871 ⁃ [Bug]Wrong default search engine displayed on startup Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX-12871 ⁃ [Bug]Wrong default search engine displayed on startup FNX2-13658 ⁃ [Bug]Wrong default search engine displayed on startup Aug 11, 2020
@Mugurell
Copy link
Contributor

Closing as per #11875 (comment)

@data-sync-user data-sync-user changed the title FNX2-13658 ⁃ [Bug]Wrong default search engine displayed on startup [Bug]Wrong default search engine displayed on startup May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. E5 Estimation Point: about 5 days eng:qa:verified QA Verified Feature:HomeScreen Feature:Search Feature:Toolbar Address bar, see also Feature:Search Release Blocker Blocks a Release S1 Blocks development/testing, may impact more than 25% of users, causes data loss, potential chemspill
Projects
None yet
Development

No branches or pull requests

9 participants