Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed crash with search extension #8626

Merged
merged 4 commits into from
May 14, 2021
Merged

fixed crash with search extension #8626

merged 4 commits into from
May 14, 2021

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Apr 23, 2021 •
edited
Loading

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

With this PR, brave uses search provider from extension by default on all platform like chromium does.

Crash comes from absense of exension search provider data in private profile's TemplateURLService because
we are using different TemplateURLService for private profile. When search provider extension is loaded/installed, browser doesn't notify about this adding to private profile's TUS.
To fix this, each SearchEngineProviderService sets extension's template url data to its service when normal profile's search provider comes from extension.

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=*ExtensionSearchProviderWithPrivateWindow*

  1. Launch browser
  2. Install ecosia extension - https://chrome.google.com/webstore/detail/ecosia-the-search-engine/eedlgdlajadkbbjoobobefphmfkcchfk/related
  3. Check Normal window's default search provider is ecosia
  4. Open private window and check ecosia is default search provider
  5. Turn ddg button on doesn't change search provider - still using ecosia
  6. Turn ddg button off doesn't change search provider - still using ecosia
  7. Goto ecosia extension details and toggle Allow in private repeatedly
  8. Check changing Allow in privatge option doesn't affect private window's search provider - still using ecosia
  9. Disable ecosia extension and check normal and private window uses default search provider
  10. Toggle DDG button on in private NTP and check search provider is changed to DDG

@simonhong simonhong self-assigned this Apr 23, 2021
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch 2 times, most recently from 3a7b61f to 7870a76 Compare April 30, 2021 04:46
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label Apr 30, 2021
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch 2 times, most recently from 35b5f46 to c3e213a Compare April 30, 2021 08:11
@simonhong simonhong marked this pull request as ready for review April 30, 2021 09:08
@simonhong simonhong changed the title WIP: fixed crash with search extension V2 fixed crash with search extension Apr 30, 2021
@simonhong simonhong added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Apr 30, 2021
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch 2 times, most recently from d207e41 to c66ba27 Compare April 30, 2021 09:15
@simonhong simonhong removed the CI/skip Do not run CI builds (except noplatform) label Apr 30, 2021
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from c66ba27 to 6b9aaa1 Compare April 30, 2021 09:20
@simonhong simonhong requested review from bridiver and bsclifton April 30, 2021 09:20
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from 6b9aaa1 to 322f3b2 Compare April 30, 2021 09:23
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label May 3, 2021
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from 322f3b2 to bf9af23 Compare May 3, 2021 00:42
@simonhong simonhong mentioned this pull request May 3, 2021
Closed
24 tasks
@simonhong simonhong requested a review from iefremov May 3, 2021 00:44
@simonhong simonhong removed the CI/skip Do not run CI builds (except noplatform) label May 4, 2021
@simonhong
Copy link
Member Author

Kindly ping :)

@iefremov
Copy link
Contributor

iefremov commented May 6, 2021

I'll defer to @goodov

@iefremov iefremov requested review from goodov and removed request for iefremov May 6, 2021 06:27
goodov
goodov reviewed May 6, 2021
View reviewed changes
browser/search_engines/private_window_search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.h Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.h Outdated Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.cc Outdated Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.h Outdated Show resolved Hide resolved
components/search_engines/pref_names.h Outdated Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from bf9af23 to a76d539 Compare May 7, 2021 02:40
@simonhong simonhong requested a review from a team as a code owner May 7, 2021 02:40
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from a76d539 to 7a0d11b Compare May 7, 2021 02:47
simonhong
simonhong commented May 7, 2021
View reviewed changes
Copy link
Member Author

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goodov Thanks for review. All addressed!

browser/search_engines/private_window_search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.h Outdated Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.h Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.h Outdated Show resolved Hide resolved
components/search_engines/pref_names.h Outdated Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.cc Show resolved Hide resolved
browser/search_engines/tor_window_search_engine_provider_service.cc Outdated Show resolved Hide resolved
browser/search_engines/search_engine_provider_service.cc Show resolved Hide resolved
@simonhong simonhong requested a review from goodov May 7, 2021 08:44
@goodov
Copy link
Member

goodov commented May 7, 2021 •
edited
Loading

@simonhong
got this DCHECK while testing https://gist.github.com/goodov/5437d6735dce60bc24b9ef2943719c8e
reproduced two times in private profile. after DuckDuck search via extension tried to search using Google by writing :g test in address bar.

@simonhong
Copy link
Member Author

@goodov Thanks for sharing DCHECK failure. Failure from OnFaviconUpdated is very new to me.
Will investigate this more. Thanks again! 👍

simonhong added 3 commits May 10, 2021 07:31
@simonhong
fixed search extension crash and bug V2
9f2d295 
fix: brave/brave-browser#15224
fix: brave/brave-browser#10601
fix: brave/brave-browser#14218

Crash comes from absense of exension search provider data in private profile's
TemplateURLService because we are using different TemplateURLService for private profile.
When search provider extension is loaded/installed, browser doesn't notify about
this adding to private profile's TUS.
To fix this, each SearchEngineProviderService sets extension's template url data to
its service when normal profile's search provider comes from extension.
@simonhong
Address comments
f41e01e
@simonhong
Extends components/search_engines/search_engines_pref_names.h for new…
26da69e 
… pref
@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from 131c9aa to d091ba7 Compare May 10, 2021 08:43
@simonhong
Copy link
Member Author

@simonhong
got this DCHECK while testing https://gist.github.com/goodov/5437d6735dce60bc24b9ef2943719c8e
reproduced two times in private profile. after DuckDuck search via extension tried to search using Google by writing :g test in address bar.

@goodov Fixed by d091ba7. PTAL again!

@goodov
Copy link
Member

goodov commented May 12, 2021

@simonhong
got this DCHECK while testing https://gist.github.com/goodov/5437d6735dce60bc24b9ef2943719c8e
reproduced two times in private profile. after DuckDuck search via extension tried to search using Google by writing :g test in address bar.

@goodov Fixed by d091ba7. PTAL again!

Still doesn't seem right. After disabling the Extension search engine via Settings and enabling extension manually I got this error: https://gist.github.com/goodov/58a34a9ed19350984a9454b6522cae4d

@simonhong simonhong force-pushed the fix_crash_with_search_provider_extension_v2 branch from d091ba7 to e4316b3 Compare May 13, 2021 10:58
@simonhong
Copy link
Member Author

simonhong commented May 13, 2021 •
edited
Loading

@simonhong
got this DCHECK while testing https://gist.github.com/goodov/5437d6735dce60bc24b9ef2943719c8e
reproduced two times in private profile. after DuckDuck search via extension tried to search using Google by writing :g test in address bar.

@goodov Fixed by d091ba7. PTAL again!

Still doesn't seem right. After disabling the Extension search engine via Settings and enabling extension manually I got this error: https://gist.github.com/goodov/58a34a9ed19350984a9454b6522cae4d

Fixed by https://github.com/brave/brave-core/pull/8626/files#diff-303c4f4e4e368bfb2681efc54ff35e1412d362740939fc50c7f93b9b92bcca2cR139. Changed to setting non-empty sync_guid. This PR will work :) Thanks for catching this.

And I'm trying another approach - This PR added new patches because we didn't make kDefaultSearchProviderDataPrefName as a extension controlled prefs. but I'm trying to set data to kDefaultSearchProviderDataPrefName as a extension controlled pref. If I can do that, we can fix this issue w/o patching.

@goodov
Copy link
Member

goodov commented May 13, 2021

latest version seems fine, no any visible issues and the logic works great!

And I'm trying another approach - This PR added new patches because we didn't make kDefaultSearchProviderDataPrefName as a extension controlled prefs. but I'm trying to set data to kDefaultSearchProviderDataPrefName as a extension controlled pref. If I can do that, we can fix this issue w/o patching.

so should we wait for solution without patching? :)

bridiver
bridiver approved these changes May 14, 2021
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved, but please follow-up as discussed to see if we can get rid of the patch

@simonhong simonhong mentioned this pull request May 14, 2021
Open
@simonhong
Copy link
Member Author

Merged because only unrelated brave ads/rewards browser tests were failed.

@simonhong simonhong merged commit 60136ab into master May 14, 2021
@simonhong simonhong deleted the fix_crash_with_search_provider_extension_v2 branch May 14, 2021 14:53
@simonhong simonhong added this to the 1.26.x - Nightly milestone May 14, 2021
This was referenced May 17, 2021
Closed
Closed
@bsclifton bsclifton mentioned this pull request May 25, 2021
Closed
@LaurenWags LaurenWags mentioned this pull request Jun 1, 2021
Closed
@btlechowski btlechowski mentioned this pull request Jun 3, 2021
Open
kylehickinson pushed a commit that referenced this pull request Jan 8, 2024
@soner-yuksel
Fix #8070: Support 'Clean Copy Link' from Sharesheet (#8626)
57095b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

@goodov goodov Awaiting requested review from goodov

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.26.x - Release
4 participants
@simonhong @iefremov @goodov @bridiver