Fixed crash with search provider extension #8539

simonhong · 2021-04-14T16:18:47Z

fix: brave/brave-browser#15224
fix: brave/brave-browser#10601
fix: brave/brave-browser#14218

Crash comes from absense of exension search provider in private profile's TemplateURLService because
we are using different TemplateURLService for private profile. but many places in upstream doesn't know about this.

Changed to use same TemplateURLService for normal & private profile.

Previously we created different TemplateURLService instance for normal and private profile because
we want to use different search provider on both. However, this approach added technical debt over time
because it's different with upstream chromium. chromium uses same instance for both.
And it turns out that search provider extension doesnt work well with current approach.
So, decided to use same architecture - using same TemplateURLService.
Instead of using one instance, active search provider is set for profile of current active windows.
With this, we can delete some patches and overrides for using different TemplateURLService.

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
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:

npm run test brave_browser_tests -- --filter=SearchEngineProviderServiceTest*

See above issue's STR.
and below issue's STR because codes for fixing below issues is cleaned up also.
brave/brave-browser#1758
brave/brave-browser#1037

Check normal window/private/private(tor) 's search engine options works properly.

bsclifton · 2021-04-16T07:42:59Z

browser/search_engines/search_engine_provider_service.cc

+  if (ignore_template_url_service_changing_)
+    return;
+
+  base::AutoReset<bool> reset(&ignore_template_url_service_changing_, true);


AutoReset<> is a super nice base library offering 🙂 Had never worked with this before, just looked into it now! Really nice to temporarily change the value and then it resets after going out of scope

browser/search_engines/search_engine_provider_service.h

browser/search_engines/search_engine_provider_service.cc

simonhong · 2021-04-20T11:03:21Z

Kindly ping :)

iefremov · 2021-04-20T12:20:10Z

browser/ui/views/search_engines/active_window_search_provider_manager.h

+// This doesn't handle guest window's provider because guest window only uses
+// its own private profile. Using SearchEngineProviderService for managing guest
+// window's DDG toggle button config is sufficient.
+class ActiveWindowSearchProviderManager : public views::WidgetObserver {


I'm not quire sure this is a way to go. The service is queried for GetDefaultSearchProvider() across the code and the callers can get different results depending on a visible widget, isn't it strange?

With this approach, we can use one service for normal/private like chromium.
Many places & services with single instance service (use one service instance for both profile) refers TemplateURLService. This makes conflict with our needs(we need different TemplateURLService instance).
Search provider extension crash also comes from using different TemplateURLService instance.
Of course, I also think this approach is not the best but this works well and we can archieve our needs w/o patching/overrding anymore.

IMO, we should introduce new complex patches/overriding to fix crash issues(brave/brave-browser#15224, brave/brave-browser#14218) w/o this approach.
and I think this approach is straightforward and can be maintained easily. WDYT? @bridiver

I think I agree with @iefremov here. In addition to what @iefremov said, changing the result based on the active window could easily return the wrong result if there are any async calls to it. It seems very risky to me based on that.

in fact I found instances where GetDefaultSearchProvider is called async which means the active window could change from what it was when the action was initiated

will close this PR and I'll try to resolve with another approach - #8626

bridiver · 2021-04-22T22:42:06Z

patches/chrome-browser-ui-views-omnibox-omnibox_view_views.h.patch

+     return popup_view_.get();
+   }
+
+  BRAVE_OMNIBOX_VIEW_VIEWS_H


#define OnOmniboxPaste NotUsed() {}; friend ... OnOmniboxPaste ?

@bridiver Right, we don't need to add new patch file for this :)
Fixed. thanks!

fix: brave/brave-browser#15224 fix: brave/brave-browser#10601 fix: brave/brave-browser#14218 Changed to use same TemplateURLService for normal & private profile.

As we use one TemplateURLService for normal/private profile and set proper provider for current active window's profile, we always can get proper selection navigation url.

On linux, BraveBrowserFrame is not used. Changed ActiveWindowSearchProviderManager owner to BrowserView. and updated test cases.

Omnibox placeholder text includes current search provider info. As we are using same TemplateURLService for normal/private window, Inactive browser window's omnibox placeholder could show different search provider. For example, when private window is active and DDG is on, other inactive window's omnibox placeholder shows DDG's placeholder if it's current tab is NTP. Of course, it's set properly when it's activated. but it could make user confusing. To fix this, inactive windows omnibox place holder will not be updated. and when activated anytime, omnibox can have proper placeholder text.

As we started to use private profile for tor window instead of guest window, we don't need to handle kDefaultSearchProviderDataPrefName differently for tor. When tor window is started, proper search provider is set regardless of `kDefaultSearchProviderDataPrefName`.

If private window is activated first, cached normal search provider is empty. In this case, we just use initial default search provider.

github-actions bot assigned simonhong Apr 14, 2021

simonhong force-pushed the fix_crash_with_search_provider_extension branch from 2c199a0 to 6d6ae20 Compare April 15, 2021 05:25

simonhong changed the title ~~WIP: Fixed crash with search provider extension~~ Fixed crash with search provider extension Apr 15, 2021

simonhong marked this pull request as ready for review April 15, 2021 14:21

simonhong requested a review from a team as a code owner April 15, 2021 14:21

simonhong requested review from darkdh, bsclifton and iefremov April 15, 2021 14:21

simonhong force-pushed the fix_crash_with_search_provider_extension branch 3 times, most recently from 3de7c8d to 66d4c62 Compare April 16, 2021 04:11

bsclifton reviewed Apr 16, 2021

View reviewed changes

browser/search_engines/search_engine_provider_service.h Show resolved Hide resolved

darkdh reviewed Apr 16, 2021

View reviewed changes

browser/search_engines/search_engine_provider_service.cc Outdated Show resolved Hide resolved

browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved

simonhong force-pushed the fix_crash_with_search_provider_extension branch from 66d4c62 to 6691c44 Compare April 17, 2021 01:20

simonhong requested review from darkdh and bsclifton April 17, 2021 01:22

simonhong force-pushed the fix_crash_with_search_provider_extension branch 3 times, most recently from c0dfc49 to a1519b6 Compare April 19, 2021 01:30

simonhong requested a review from bridiver as a code owner April 19, 2021 01:30

simonhong force-pushed the fix_crash_with_search_provider_extension branch 2 times, most recently from 377ae23 to 0db630e Compare April 20, 2021 06:42

iefremov reviewed Apr 20, 2021

View reviewed changes

bridiver reviewed Apr 22, 2021

View reviewed changes

simonhong added 4 commits April 23, 2021 08:34

WIP: Fixed crash with search provider extension

cf5de7e

fix: brave/brave-browser#15224 fix: brave/brave-browser#10601 fix: brave/brave-browser#14218 Changed to use same TemplateURLService for normal & private profile.

Use separated class for setting proper DSE to active window

91903f6

Deleted temporal AutocompleteClassifier usage for context menu

22c81c0

As we use one TemplateURLService for normal/private profile and set proper provider for current active window's profile, we always can get proper selection navigation url.

Cleanup code

5679f45

simonhong added 9 commits April 23, 2021 08:34

Fixed and updated test cases

adc89ef

Fixed search engine is not set properly on linux

6b464a8

On linux, BraveBrowserFrame is not used. Changed ActiveWindowSearchProviderManager owner to BrowserView. and updated test cases.

Check simply whether tor windows's search provider is ddg or not.

b7ad03d

Define BraveOmniboxViewViews in a separted files

76c2a82

Fixed no rule applying failure for views/omnibox/

3eda188

Fixed crash during the browser test

8dcee36

If private window is activated first, cached normal search provider is empty. In this case, we just use initial default search provider.

Deleted omibox_view_views.h patch

ca8f294

simonhong force-pushed the fix_crash_with_search_provider_extension branch from 0db630e to ca8f294 Compare April 22, 2021 23:51

simonhong requested a review from bridiver April 22, 2021 23:53

simonhong closed this May 3, 2021

simonhong deleted the fix_crash_with_search_provider_extension branch June 28, 2021 05:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed crash with search provider extension #8539

Fixed crash with search provider extension #8539

simonhong commented Apr 14, 2021 •

edited

Loading

bsclifton Apr 16, 2021 •

edited

Loading

simonhong commented Apr 20, 2021

iefremov Apr 20, 2021

simonhong Apr 20, 2021

simonhong Apr 21, 2021 •

edited

Loading

bridiver Apr 23, 2021

bridiver Apr 23, 2021

simonhong May 3, 2021

bridiver Apr 22, 2021

simonhong Apr 22, 2021

Fixed crash with search provider extension #8539

Fixed crash with search provider extension #8539

Conversation

simonhong commented Apr 14, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

simonhong commented Apr 20, 2021

iefremov Apr 20, 2021

Choose a reason for hiding this comment

simonhong Apr 20, 2021

Choose a reason for hiding this comment

simonhong Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

bridiver Apr 23, 2021

Choose a reason for hiding this comment

bridiver Apr 23, 2021

Choose a reason for hiding this comment

simonhong May 3, 2021

Choose a reason for hiding this comment

bridiver Apr 22, 2021

Choose a reason for hiding this comment

simonhong Apr 22, 2021

Choose a reason for hiding this comment

simonhong commented Apr 14, 2021 •

edited

Loading

bsclifton Apr 16, 2021 •

edited

Loading

simonhong Apr 21, 2021 •

edited

Loading