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.

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

Fixes error thrown when speculation rules are disabled #13772

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

samartnik
Copy link
Contributor

@samartnik samartnik commented Jun 14, 2022

Resolves brave/brave-browser#23433

The error we hit is:
[ERROR:render_process_host_impl.cc(5119)] Terminating render process for bad Mojo message: Received bad user message: Speculation rules must be enabled to bind to blink.mojom.SpeculationHost in the browser.

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:

@samartnik samartnik added this to the 1.41.x - Nightly milestone Jun 14, 2022
@samartnik samartnik requested a review from a team as a code owner June 14, 2022 11:08
@samartnik samartnik self-assigned this Jun 14, 2022
// kSpeculationRulesPrefetchProxy and kPrerender2 are disabled in Brave per
// https://github.com/brave/brave-core/commit/fe8b2f327f258c241345c67ba7224bd34d34a6fc
// So we have nothing to do here. Overriding this function just to avoid
// ReportBadMessage.
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should fix brave/chromium_src/third_party/blink/common/origin_trials/origin_trials.cc instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goodov cloud you elaborate? What fix can be applied there?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure right now, but adding SpeculationRulesPrefetchProxy into the list of disabled features in origin_trials.cc won't fix the issue? (there're two lists)
It looks like the feature is partially disabled now. Because of that we see this incorrect request on the browser side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably add kSpeculationRulesPrefetchProxy there either way, but it doesn't fix error. We still hit it.

Copy link
Contributor Author

@samartnik samartnik Jun 14, 2022

Choose a reason for hiding this comment

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

Just to be more specific, this change doesn't help

--- a/chromium_src/third_party/blink/common/origin_trials/origin_trials.cc
+++ b/chromium_src/third_party/blink/common/origin_trials/origin_trials.cc
@@ -35,6 +35,7 @@ bool IsTrialDisabledInBrave(base::StringPiece trial_name) {
       "Prerender2",
       "PrivacySandboxAdsAPIs",
       "SignedExchangeSubresourcePrefetch",
+      "SpeculationRulesPrefetch",
       "SubresourceWebBundles",
       "TrustTokens",
   };
@@ -65,6 +66,7 @@ bool IsTrialDisabledInBrave(OriginTrialFeature feature) {
           OriginTrialFeature::kPrerender2,
           OriginTrialFeature::kPrivacySandboxAdsAPIs,
           OriginTrialFeature::kSignedExchangeSubresourcePrefetch,
+          OriginTrialFeature::kSpeculationRulesPrefetchProxy,
           OriginTrialFeature::kTrustTokens,
       };
   // clang-format on

Copy link
Member

Choose a reason for hiding this comment

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

ok. I'll try to figure out what's going on there with feature/trial disabling.

Copy link
Member

Choose a reason for hiding this comment

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

@samartnik a better fix is to remove this line: chromium/chromium@3c9efd5#diff-a800f352ae257f734b958312eedfc8fd15d26d50574ecc4c4e639b9a1d825df3R2197

I'm okay with patching it until we figure out what is the better way to do this without patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want just directly patch it instead of chromium_src changes?

Copy link
Member

Choose a reason for hiding this comment

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

I think I've got a patchless solution. Put this file into brave/chromium_src/content/child/runtime_features.cc:
https://gist.github.com/goodov/a62434bb5e940b2f9f6b26e711bdc638

Copy link
Member

Choose a reason for hiding this comment

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

@samartnik Actually there is even a better place which already does exactly this:

void BraveContentRendererClient::
SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() {
ChromeContentRendererClient::
SetRuntimeFeaturesDefaultsBeforeBlinkInitialization();
blink::WebRuntimeFeatures::EnableWebNfc(false);
// These features don't have dedicated WebRuntimeFeatures wrappers.
blink::WebRuntimeFeatures::EnableFeatureFromString("DigitalGoods", false);
if (!base::FeatureList::IsEnabled(blink::features::kFileSystemAccessAPI)) {
blink::WebRuntimeFeatures::EnableFeatureFromString("FileSystemAccess",
false);
blink::WebRuntimeFeatures::EnableFeatureFromString(
"FileSystemAccessAPIExperimental", false);
}
blink::WebRuntimeFeatures::EnableFeatureFromString("Serial", false);
}

Just add a single line with this exact feature right there and we should be fine. No need to add new source file.

@samartnik samartnik force-pushed the android_disable_speculation_rules branch from 5145259 to 0c63abc Compare June 14, 2022 13:18
@samartnik samartnik force-pushed the android_disable_speculation_rules branch from 0c63abc to a211abc Compare June 14, 2022 13:48
@samartnik samartnik merged commit 7803b5b into master Jun 14, 2022
@samartnik samartnik deleted the android_disable_speculation_rules branch June 14, 2022 15:13
emerick pushed a commit that referenced this pull request Jun 14, 2022
Fixes error thrown when speculation rules are disabled
@Uni-verse
Copy link
Contributor

Uni-verse commented Jun 15, 2022

Verified on Samsung GS 21 & Tab S7 using

Brave	1.41.63 Chromium: 103.0.5060.42 (Official Build) canary (32-bit) 
Revision	de0d840bf9439c31bd86bf74f065c31fdf9b208d-refs/branch-heads/5060@{#667}
OS	Android 12; Build/SP1A.210812.016

STR - brave/brave-browser#23433 (comment)

  • Confirmed search results are displayed when using search on google homepage.
  • Confirmed user is able to search using key phrase and suggested trending searches.
1 2
screenshot-1655253919376 screenshot-1655254185286

emerick pushed a commit that referenced this pull request Jun 15, 2022
Fixes error thrown when speculation rules are disabled
cdesouza-chromium pushed a commit that referenced this pull request Dec 11, 2023
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
cdesouza-chromium pushed a commit that referenced this pull request Dec 13, 2023
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
cdesouza-chromium pushed a commit that referenced this pull request Dec 13, 2023
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
cdesouza-chromium pushed a commit that referenced this pull request Dec 14, 2023
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
cdesouza-chromium pushed a commit that referenced this pull request Dec 22, 2023
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
cdesouza-chromium pushed a commit that referenced this pull request Jan 4, 2024
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
mkarolin added a commit that referenced this pull request Jan 12, 2024
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
mkarolin added a commit that referenced this pull request Jan 15, 2024
We originally disabled it for brave/brave-browser#23433 via
#13772, but the issue is no longer reproducible in
cr121, so we can just drop the call to disable the flag.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/d1cf902be6fc9ae8654fe5a6c466dfb51f782197

commit d1cf902be6fc9ae8654fe5a6c466dfb51f782197
Author: Jeremy Roman <jbroman@chromium.org>
Date:   Thu Nov 9 16:38:02 2023 +0000

    Remove RuntimeEnabledFeatures checks for SpeculationRules and SpeculationRulesPrefetchProxy.

    These have been shipped for almost a year.

    Bug: 1173646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page error when searching from google homepage
3 participants