Android private search fix #15704

wchen342 · 2022-10-28T14:18:39Z

Resolves brave/brave-browser/issues/25821
Resolves brave/brave-browser/issues/26483

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

test 1 (the issue)

Open a private tab.
Search for something.
Change the default search engine for private tabs (Settings -> Search engines -> Private Tab).
Navigate back to existing private tab OR open a new private tab.
Search for something, new search engine shall be used.

test 2 (upgrading from and old version)

When upgraded from old version, the search settings for regular and private tab shall remain then same as before.

test 3 (private tabs initialization)

Clear all tabs.
Open a private tab. Tap on url bar and search anything.
Clear private tabs and go back to a regular tab.
Open a private tab and search in the url bar again.
Repeat for 2-3 times.
Nothing extraordinary shall happen during the process.

test 4 (onboarding - non-Brave default)

Set Android locale to anywhere Brave Search IS NOT default, reset/reinstall Brave.
Get through onboarding and tap url bar twice to get the search onboarding fragment.
Choose any one of the search options.
Check in settings that search settings for both regular and private tabs are set to the search engine that was just being chosen.

test 4 (onboarding - Brave default)

Set Android locale to anywhere Brave Search IS default, reset/reinstall Brave.
Check in settings that Brave search is set for both regular and private tabs.

AlexeyBarabash · 2022-10-28T15:21:09Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

+
+        // Only need last used profile because we are in settings
+        if (mProfile == null) {
+            if (!mIsPrivate)


nit: use curly braces
(https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/java/java.md#Curly-braces)

AlexeyBarabash · 2022-10-28T15:32:43Z

browser/search_engines/brave_template_url_service_factory_android.cc

+    JNI_BraveTemplateUrlServiceFactory_GetTemplateUrlService(JNIEnv* env, const
+    base::android::JavaParamRef<jobject>& j_profile) {
+  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+  return GetTemplateUrlService(profile)->GetJavaObject();


if this causes to use browser/search_engines/private_window_search_engine_provider_service.cc then it's awesome 👍

I actually asked Simon about this, and it is a mimic of private_window_search_engine_provider_service. That class itself is only applicable to extensions though, maybe useful in the future.

SergeyZhukovsky · 2022-10-28T16:25:43Z

@wchen342 is it ready for review or draft yet?

wchen342 · 2022-10-28T16:29:12Z

@SergeyZhukovsky It's draft, still having a problem with bytecode, but the rest is finished.

SergeyZhukovsky · 2022-10-28T17:56:11Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

+    }
+
+    static public void setDSEPrefs(TemplateUrl templateUrl, Profile profile) {
+        Log.d("BraveSearchEngineAdapter", "setDSEPrefs: isPrivate: " + String.valueOf(profile.isOffTheRecord()) + " ShortName: " + templateUrl.getShortName());


do we need logs?

I'll do a clean up after fixed other problems. Same for the other logs.

SergeyZhukovsky · 2022-10-28T17:56:21Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

+    static public void updateActiveDSE(Profile profile) {
+        String shortName = getDSEShortName(profile);
+        TemplateUrl templateUrl = getTemplateUrlByShortName(profile, shortName);
+        Log.d("BraveSearchEngineAdapter", "updateActiveDSE: isPrivate: " + String.valueOf(profile.isOffTheRecord()) + " ShortName: " + shortName);


SergeyZhukovsky · 2022-10-28T17:56:42Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

            BraveSearchEnginePrefHelper.getInstance().setFetchSEFromNative(false);
        }

+        Log.d("BraveSearchEngineAdapter", "getDSEShortName: isPrivate: " + String.valueOf(profile.isOffTheRecord()) + " defaultSearchEngineName: " + defaultSearchEngineName + " DSE_SHORTNAME: " +
+            ContextUtils.getAppSharedPreferences().getString(profile.isOffTheRecord() ? PRIVATE_DSE_SHORTNAME : STANDARD_DSE_SHORTNAME, ""));


SergeyZhukovsky · 2022-10-28T17:57:53Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

+    private void runTemplateUrlServiceWithProfile(Runnable r) {
+        BraveTemplateUrlServiceFactory.setCurrentProfile(mProfile);
+        r.run();
+        BraveTemplateUrlServiceFactory.setCurrentProfile(null);


not sure I understand what we do here

So what happens is the original call in chromium doesn't have a parameter, and bytecode cannot rewrite calls to add it. So I use a workaround that I add a static field in the BraveTemplateUrlServiceFactory class and set it before any function call to it. Otherwise we need to overwrite every function in chromium's SearchEngineAdapter.java.

why do we need to call it twice, one time before the r.run() with a profile and another one after that?

I don't see as well how sProfile is used, do we need it at all?

why do we need to call it twice, one time before the r.run() with a profile and another one after that?

It is once for setting the static field and once for clearing it so other calls to get() doesn't get stuck with the profile. I accidentally deleted the sProfile part when doing the other fix for getting tab models in native, thanks for pointing that out.

ok, got it, could you please make another pass and make sure everything works as intended before the merge?

Yeah I'll rebase, fix the formatting etc. and check everything is fine.

SergeyZhukovsky · 2022-10-28T17:59:29Z

android/java/org/chromium/chrome/browser/settings/SearchEngineTabModelSelectorObserver.java

@@ -31,7 +32,8 @@ public void onNewTabCreated(Tab tab, @TabCreationState int creationState) {}

    @Override
    public void onTabModelSelected(TabModel newModel, TabModel oldModel) {
-        BraveSearchEngineUtils.updateActiveDSE(mTabModelSelector.isIncognitoSelected());
+        Log.w("SearchEngineTabModelSelectorObserver", "onTabModelSelected: isIncognito: " + String.valueOf(newModel.getProfile().isOffTheRecord()));


android/java/org/chromium/chrome/browser/search_engines/BraveTemplateUrlServiceFactory.java

android/java/org/chromium/chrome/browser/search_engines/BUILD.gn

android/java/org/chromium/chrome/browser/settings/BraveSearchEngineUtils.java

AlexeyBarabash

lgtm

samartnik · 2022-11-02T16:11:40Z

build/android/bytecode/java/org/brave/bytecode/BraveTemplateUrlServiceFactory.java

+
+import org.objectweb.asm.ClassVisitor;
+
+public class BraveTemplateUrlServiceFactory extends BraveClassVisitor {


Please name it BraveTemplateUrlServiceFactoryClassAdapter for consistency

samartnik · 2022-11-02T17:12:51Z

android/java/org/chromium/chrome/browser/search_engines/BraveTemplateUrlServiceFactory.java

+    /**
+     * @return The singleton instance of {@link TemplateUrlService} for each profile, creating it if necessary.
+     */
+    public static TemplateUrlService get(Profile profile) {


We should rename it to something like getForProfile to clearly indicate this is not the method we are changing ownership to.

Good suggestion, changed the method name.

samartnik · 2022-11-02T17:40:24Z

android/java/org/chromium/chrome/browser/search_engines/BraveTemplateUrlServiceFactory.java

+
+        Profile profile = null;
+
+        // Cannot import BraveActivity here due to circular dependency, need reflection all the way


This indicates that we are misusing it.

Aside from this, in general I don't really get it, previously we had quite simple logic to apply current search engine depending on whether we are currently on private or regular tab. What has changed? How closing all private tabs fixing it? Frankly I would rather require user to relaunch browser to apply the setting instead of these substantial changes. As it's really hard to predict all unintended issues we may get going forward.

Please check DM for a brief explanation of the issue. Also in addition:

What has changed?

There are now two services instead of one, one tied to regular profile and one tied to OTR profile. Also the service tied to OTR profile will be destroyed time to time (e.g. when user closes all private tabs and open one again later). And these all happens in native and share codes with desktop so it is not possible to override.

How closing all private tabs fixing it?

The service tied to OTR profile got terminated, and the next time a new private tab is opened, a new service is started, copying prefs from regular profile's service.

require user to relaunch browser

That is rather a product/design decision I cannot make. But as far as the current implementation goes, I see no other way to make things work because of the desktop/native changes, so this is basically the best we can do.

Profile* GetOriginalProfile() { return ProfileManager::GetActiveUserProfile()->GetOriginalProfile(); } } // namespace static TemplateURLService* GetTemplateUrlService() { return TemplateURLServiceFactory::GetForProfile(GetOriginalProfile()); }

won't that give us a current active tab profile?

@SergeyZhukovsky Is that the code in template_url_service_factory_android.cc? That function is never called. The service is acquired with static TemplateURLService* GetTemplateUrlService(Profile* profile) in brave_template_url_service_factory_android.cc instead.

Also it doesn't give us the service linked to active tab. GetOriginalProfile only return regular profile. If I remember correctly GetActiveUserOrOffTheRecordProfile() dpesn't work the same way on Desktop and Android, that was why Brian said we shall use Profile from Java whenever possible.

Edit: Just checked the code, GetActiveUser basically returns initial profile for non ChromeOS builds. It is mostly used for getting the current user on ChromeOS.

Maybe there is a way to get it done without the layering violation? I also don't like that part much. I just saw that there is an access to profiles from native, is there a way to get current active tab from native?

Updated with a way to iterate through all tab models and find the active one.

samartnik · 2022-11-07T14:20:20Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

+        runTemplateUrlServiceWithProfile(() -> { super.onTemplateURLServiceChanged(); });
+    }
+
+    // private boolean locationEnabled(TemplateUrl templateUrl) is private and unused


Not sure what this comment indicates

There is a method in the original adapter but it is not called anywhere so no need to override it. Maybe I shall just delete the line since it confuses others.

samartnik · 2022-11-07T14:31:46Z

android/java/org/chromium/chrome/browser/search_engines/settings/BraveSearchEngineAdapter.java

    }

-    static public String getDSEShortName(boolean isPrivate) {
+    static public String getDSEShortName(Profile profile, boolean javaOnly) {


Could you explain this javaOnly parameter? It's not obvious when it should be false and when true.

I renamed it to readJavaPrefOnly and added a comment. Basically when it is true the native service will not be used and only Java side preference will be used.

SergeyZhukovsky · 2022-11-07T14:34:10Z

@wchen342 perhaps it's good to mention in test plan about testing upgrade scenarios as well that common tab engine and private should remain on upgrades and etc.

SergeyZhukovsky

++

samartnik

lgtm

kjozwiak · 2022-12-20T06:42:57Z

Verification PASSED on Pixel 6 running Android 13 using the following build(s):

Brave | 1.48.70 Chromium: 109.0.5414.46 (Official Build) canary (32-bit)
---- | ----
Revision | 6e36b77363ef3febbe792af680fa1367993ddcf0-refs/branch-heads/5414@{#709}
OS | Android 13; Build/TQ1A.221205.011

`Test Case #1 - https://github.com/brave/brave-browser/issues/25821`

As mentioned #15704 (comment), this is the original issue that was mentioned/outlined via brave/brave-browser#25821. Went through the following STR/Cases:

launched Brave (skipped all the onboarding as it's not needed)
launched a Private Tab and did a quick search and ensured that Brave was the default search engine
changed the default Private Tab SE to Startpage via Hamburger Menu -> Settings -> Search engine via Private Tab
switched back to Standard Tab and did a quick search via the omnibox and ensured Startpage was being used
within Standard Tab, switch the Private Tab SE to Qwant via Hamburger Menu -> Settings -> Search engine
switched back to Private Tab and did another search via the omnibox and ensured that Qwant was being used
within the Private Tab, switched the default Standard Tab SE to Bing via Hamburger Menu -> Settings -> Search engine
switched back to Standard Tab and ensured that Bing was the default browser and is being used via the omnibox

`Test Case #2 - Updating from an old profile (ensuring default SE weren't changed`

installed 1.47.54 Chromium: 107.0.5304.91 (before the above uplift)
switched the Standard Tab SE to DDG and switched the Private Tab SE to Starpage
updated to 1.48.70 Chromium: 109.0.5414.46
ensured that Standard Tab is still set as DDG and Private Tab is set as Startpage

Also ran through the Test Case #1 mentioned above after upgrading and ensured everything was working as expected.

`Test Case #3 - Private Tab initialization`

launched Brave (skipped all the onboarding as it's not needed)
launched a Private Tab and did a quick search via the omnibox and ensured everything worked as exepcted
closed the Private Tab via Close Private tabs and returned to the Standard Tab
opened a Private Tab once again and did another search via the omnibox

Repeated the above several times as the STR/Cases mentioned via #15704 (comment).

`Test Case #4 - Clean profile using locale without Brave as default - Japan`

launched Brave (skipped all the onboarding as it's not needed)
tapped on the omnibox and ensured that the search onboarding appeared without any issues
selected Startpage as the default SE via the onboarding
ensured that both Standard Tab & Private Tab both had Startpage as the default SE

`Example`	`Example`

`Test Case #5 - Clean profile using locale without Brave as default - Canada`

Went through this case several times while running through the above cases and confirmed that it's working as expected

wchen342 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Oct 28, 2022

wchen342 requested review from SergeyZhukovsky and AlexeyBarabash October 28, 2022 14:18

wchen342 self-assigned this Oct 28, 2022

wchen342 force-pushed the android_private_search branch from 390e834 to cdf90e2 Compare October 28, 2022 14:21

wchen342 added the CI/run-draft label Oct 28, 2022

wchen342 force-pushed the android_private_search branch from cdf90e2 to 3fbefe3 Compare October 28, 2022 14:23

AlexeyBarabash reviewed Oct 28, 2022

View reviewed changes

wchen342 force-pushed the android_private_search branch from 3fbefe3 to d2f8f78 Compare October 28, 2022 16:23

wchen342 force-pushed the android_private_search branch 3 times, most recently from 2df0740 to 6c03cfa Compare October 28, 2022 17:05

SergeyZhukovsky reviewed Oct 28, 2022

View reviewed changes

wchen342 force-pushed the android_private_search branch 3 times, most recently from cbe875e to ecc5d0f Compare November 2, 2022 15:51

wchen342 marked this pull request as ready for review November 2, 2022 15:53

wchen342 requested a review from samartnik as a code owner November 2, 2022 15:53

wchen342 requested a review from SergeyZhukovsky November 2, 2022 15:54

wchen342 force-pushed the android_private_search branch from ecc5d0f to 404df99 Compare November 2, 2022 16:03

AlexeyBarabash reviewed Nov 2, 2022

View reviewed changes

android/java/org/chromium/chrome/browser/search_engines/BraveTemplateUrlServiceFactory.java Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Nov 2, 2022

View reviewed changes

android/java/org/chromium/chrome/browser/search_engines/BUILD.gn Show resolved Hide resolved

AlexeyBarabash reviewed Nov 2, 2022

View reviewed changes

android/java/org/chromium/chrome/browser/settings/BraveSearchEngineUtils.java Show resolved Hide resolved

AlexeyBarabash approved these changes Nov 2, 2022

View reviewed changes

samartnik reviewed Nov 2, 2022

View reviewed changes

wchen342 force-pushed the android_private_search branch 3 times, most recently from a033a5f to a2eee33 Compare November 4, 2022 15:44

wchen342 requested a review from samartnik November 4, 2022 15:45

samartnik reviewed Nov 7, 2022

View reviewed changes

wchen342 force-pushed the android_private_search branch from a2eee33 to 05e2e08 Compare November 7, 2022 15:43

SergeyZhukovsky approved these changes Nov 7, 2022

View reviewed changes

samartnik approved these changes Nov 7, 2022

View reviewed changes

wchen342 force-pushed the android_private_search branch 5 times, most recently from 8c3321a to 5d2ba06 Compare November 7, 2022 19:35

Add separate JNI getter for OTR profile based TemplateUrlService

212ac0c

wchen342 force-pushed the android_private_search branch from 5d2ba06 to 212ac0c Compare November 8, 2022 04:07

wchen342 added this to the 1.47.x - Nightly milestone Nov 8, 2022

wchen342 enabled auto-merge November 8, 2022 04:14

wchen342 merged commit a20e9b9 into master Nov 8, 2022

wchen342 deleted the android_private_search branch November 8, 2022 04:43

This was referenced Dec 9, 2022

[Android] Remove getOriginalProfile() from search utils #16311

Merged

Remove getOrignalProfile() from search utils brave/brave-browser#27273

Closed


		import org.objectweb.asm.ClassVisitor;

		public class BraveTemplateUrlServiceFactory extends BraveClassVisitor {


		Profile profile = null;

		// Cannot import BraveActivity here due to circular dependency, need reflection all the way

Android private search fix #15704

Android private search fix #15704

Conversation

wchen342 commented Oct 28, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

test 1 (the issue)

test 2 (upgrading from and old version)

test 3 (private tabs initialization)

test 4 (onboarding - non-Brave default)

test 4 (onboarding - Brave default)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Oct 28, 2022

wchen342 commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchen342 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

wchen342 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Nov 7, 2022

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

kjozwiak commented Dec 20, 2022

Test Case #1 - https://github.com/brave/brave-browser/issues/25821

Test Case #2 - Updating from an old profile (ensuring default SE weren't changed

Test Case #3 - Private Tab initialization

Test Case #4 - Clean profile using locale without Brave as default - Japan

Test Case #5 - Clean profile using locale without Brave as default - Canada

wchen342 commented Oct 28, 2022 •

edited

Loading

wchen342 Nov 3, 2022 •

edited

Loading

wchen342 Nov 3, 2022 •

edited

Loading

`Test Case #1 - https://github.com/brave/brave-browser/issues/25821`

`Test Case #2 - Updating from an old profile (ensuring default SE weren't changed`

`Test Case #3 - Private Tab initialization`

`Test Case #4 - Clean profile using locale without Brave as default - Japan`

`Test Case #5 - Clean profile using locale without Brave as default - Canada`