Fixes crash when suggesting in Private window. #5221

mkarolin · 2020-04-10T18:04:02Z

When search suggestions are turned on and a user types into the omnibox
in a Private window a crash would occur. This started happening with the
upgrade to Chromium 81 because the check for BitmapFetchService bein
null has been removed in ChromeOmniboxClient::OnResultChanged (and other
places). This is because the code there isn't expected to be triggered
for an OTR profile.

Updated BraveAutocompleteProviderClient to override 2 additional
methods:

IsOffTheRecord
StartServiceWorker

These methods regulate when it is suitable to make suggestions and
should use the real profile (OTR or regular) instead of using regular
profile that we send to the base class even when the profile is OTR.

Submitter Checklist:

Test Plan:

See STR in brave/brave-browser#9119

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

Fixes brave/brave-browser#9119 When search suggestions are turned on and a user types into the omnibox in a Private window a crash would occur. This started happening with the upgrade to Chromium 81 because the check for BitmapFetchService bein null has been removed in ChromeOmniboxClient::OnResultChanged (and other places). This is because the code there isn't expected to be triggered for an OTR profile. Updated BraveAutocompleteProviderClient to override 2 additional methods: 1. IsOffTheRecord 2. StartServiceWorker These methods regulate when it is suitable to make suggestions and should use the real profile (OTR or regular) instead of using regular profile that we send to the base class even when the profile is OTR.

simonhong · 2020-04-13T01:39:21Z

browser/autocomplete/brave_autocomplete_provider_client.cc

+
+void BraveAutocompleteProviderClient::StartServiceWorker(
+    const GURL& destination_url) {
+  if (profile_->IsOffTheRecord())


I'm confusing with this method. Is it possible that ChromeAutocompleteProviderClient::profile_ is OTR?
It's always initialized by ChromeAutocompleteProviderClient(profile->GetOriginalProfile()).
So, I'm curious why this method should be overridden? (Maybe I'm missing something..?)

simonhong · 2020-04-13T01:55:56Z

browser/autocomplete/brave_autocomplete_provider_client.h

+  // We should not hide the true nature of the profile when these methods in
+  // ChromeAutocompleteProviderClient are called as they control what is
+  // suitable for suggestions in regular vs incognito profiles.
+  bool IsOffTheRecord() const override;


~~ChromeAutocompleteProvider::IsOffTheRecored() can return true?~~

simonhong · 2020-04-13T08:31:48Z

I think the root cause of this issue is below code.

BraveAutocompleteProviderClient::BraveAutocompleteProviderClient(
    Profile* profile)
    : ChromeAutocompleteProviderClient(profile->GetOriginalProfile()),
      profile_(profile) {
}

We made all ChromeAutocompleteProviderClient uses original profile.
I think using original profile was valid fix for handling web search in context menu(#757).
However, this made side-effect.
I think all other places should instantiate it with use passed profile (not original profile)
For example, we should not use same code for below.

std::unique_ptr<AutocompleteProviderClient>
ChromeOmniboxClient::CreateAutocompleteProviderClient() {
  return std::make_unique<ChromeAutocompleteProviderClient>(profile_);
}

We overrides it with BraveAutocompleteProviderClient. So, SearchProvider gets wrong condition.
I think we should use different kinds of sub classes for ChromeAutocompleteProviderClient. WDYT?

For ChromeOmniboxClient, we don't need to use original profile explicitely. For AutocompleteProviderClassifier, we need to initialize base class with original profile For more detail, please see BraveAutocompleteProviderClientForClassifier.

simonhong · 2020-04-13T10:17:33Z

@mkarolin I added one commit based on my above comment.
If you think this is not a right way, feel free to delete.

mkarolin · 2020-04-13T16:05:08Z

@simonhong, yes that makes sense to me. I assumed it was the way it was for #757 and didn't try to dig into it, but if only the classifier needs the original profile instead of the OTR, then it's definitely a better solution.

mkarolin · 2020-04-13T22:14:33Z

CI: known unrelated failure on Android (dist). Unrelated browser test failure on Mac.

bridiver · 2020-04-15T16:51:22Z

browser/autocomplete/brave_autocomplete_provider_client.cc

 #include "chrome/common/webui_url_constants.h"

 BraveAutocompleteProviderClient::BraveAutocompleteProviderClient(
-    Profile* profile)
-    : ChromeAutocompleteProviderClient(profile->GetOriginalProfile()),


I'm fairly certain we're leaking private autocomplete into the regular profile here in the old code and here in the updated code https://github.com/brave/brave-core/pull/5221/files#diff-2325211a427ac6f007ad338411e100aaR14 cc @diracdeltas @simonhong

bridiver

approving to fix the crash, but this needs a follow-up to fix the private -> public leak

Fixes crash when suggesting in Private window.

kjozwiak · 2020-04-16T17:52:09Z

Verification PASSED on macOS 10.15.3 x64 Catalina using the following build:

Brave | 1.9.26 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
-- | --
Revision | 32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS | macOS Version 10.15.3 (Build 19D76)

Prerequisites:

Ensure that Google is the default search engine and Autocomplete searches and URLs is enabled

Cases:

ensured that PB doesn't crash when typing into the URL bar
ensured Tor windows aren't crashing when typing into the URL bar
ensured that Guest windows aren't crashing when typing into the URL bar

rebron · 2020-04-16T18:32:02Z

Looks good to me too.

Brave	1.9.26 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
Revision	32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS	macOS Version 10.15.5 (Build 19F53f)

Following same steps as above.

Fixes crash when suggesting in Private window.

mkarolin added this to the 1.9.x - Nightly milestone Apr 10, 2020

mkarolin self-assigned this Apr 10, 2020

mkarolin requested review from simonhong and bbondy April 10, 2020 18:04

simonhong reviewed Apr 13, 2020

View reviewed changes

mkarolin requested a review from bridiver as a code owner April 13, 2020 10:16

bridiver reviewed Apr 15, 2020

View reviewed changes

bridiver approved these changes Apr 15, 2020

View reviewed changes

mkarolin mentioned this pull request Apr 15, 2020

[Desktop] [Security] BraveAutocompleteProviderClientForClassifier is leaking OTR profile data brave/brave-browser#9239

Closed

mkarolin merged commit 6a9eb17 into master Apr 15, 2020

mkarolin deleted the maxk-issue-9119 branch April 15, 2020 18:06

mkarolin added a commit that referenced this pull request Apr 15, 2020

Merge pull request #5221 from brave/maxk-issue-9119

9b8edc2

Fixes crash when suggesting in Private window.

This was referenced Apr 15, 2020

Fixes crash when suggesting in Private window (1.8.x). #5256

Merged

Fixes crash when suggesting in Private window (1.7.x). #5257

Merged

bsclifton pushed a commit that referenced this pull request Apr 21, 2020

Merge pull request #5221 from brave/maxk-issue-9119

dff0b2e

Fixes crash when suggesting in Private window.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes crash when suggesting in Private window. #5221

Fixes crash when suggesting in Private window. #5221

mkarolin commented Apr 10, 2020

simonhong Apr 13, 2020 •

edited

Loading

simonhong Apr 13, 2020 •

edited

Loading

simonhong commented Apr 13, 2020 •

edited

Loading

simonhong commented Apr 13, 2020

mkarolin commented Apr 13, 2020 •

edited

Loading

mkarolin commented Apr 13, 2020

bridiver Apr 15, 2020 •

edited

Loading

bridiver left a comment

kjozwiak commented Apr 16, 2020

rebron commented Apr 16, 2020

Fixes crash when suggesting in Private window. #5221

Fixes crash when suggesting in Private window. #5221

Conversation

mkarolin commented Apr 10, 2020

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

simonhong Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

simonhong commented Apr 13, 2020 • edited Loading

simonhong commented Apr 13, 2020

mkarolin commented Apr 13, 2020 • edited Loading

mkarolin commented Apr 13, 2020

bridiver Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

kjozwiak commented Apr 16, 2020

rebron commented Apr 16, 2020

simonhong Apr 13, 2020 •

edited

Loading

simonhong Apr 13, 2020 •

edited

Loading

simonhong commented Apr 13, 2020 •

edited

Loading

mkarolin commented Apr 13, 2020 •

edited

Loading

bridiver Apr 15, 2020 •

edited

Loading