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

Stop explicitly disabling SharedArrayBuffers #14766

Closed
fmarier opened this issue Mar 16, 2021 · 2 comments · Fixed by brave/brave-core#8544
Closed

Stop explicitly disabling SharedArrayBuffers #14766

fmarier opened this issue Mar 16, 2021 · 2 comments · Fixed by brave/brave-core#8544

Comments

@fmarier
Copy link
Member

fmarier commented Mar 16, 2021

SharedArrayBuffers were explicitly disabled in brave/brave-core#114 in order to match the Muon settings in the MVP.

Given that Google is actively addressing the Spectre threat, I think we can remove our customization and rely on their judgement when it comes to re-enabling it and the extra protections that would be needed for that to happen.

@fmarier fmarier added OS/Android Fixes related to Android browser functionality OS/Desktop labels Mar 16, 2021
@fmarier fmarier self-assigned this Mar 16, 2021
@fmarier
Copy link
Member Author

fmarier commented Apr 15, 2021

Testing this is really tricky.

I found an old test page with some code to check for SharedArraybuffers:

  if (typeof SharedArrayBuffer !== 'function') {
    alert('this browser does not have SharedArrayBuffer support enabled' + '\n\n' + msg);
    return
  }

However, the problem is that now the SharedArrayBuffer constructor is now always available.

So I instead wrote a test page which passes a SharedArrayBuffer via postMessage to a worker. The problem however is that it works just fine in release without my change. That's because the flag we are setting is overridden later by SetCustomizedRuntimeFeaturesFromCombinedArgs(). If I disable that override:

--- a/content/child/runtime_features.cc
+++ b/content/child/runtime_features.cc
@@ -537,9 +537,9 @@ void SetCustomizedRuntimeFeaturesFromCombinedArgs(
     WebRuntimeFeatures::EnableDecodeLossyWebPImagesToYUV(true);
   }
 
-  WebRuntimeFeatures::EnableSharedArrayBuffer(
-      base::FeatureList::IsEnabled(features::kSharedArrayBuffer) ||
-      base::FeatureList::IsEnabled(features::kWebAssemblyThreads));
+  WebRuntimeFeatures::EnableSharedArrayBuffer(false);
+//      base::FeatureList::IsEnabled(features::kSharedArrayBuffer) ||
+//      base::FeatureList::IsEnabled(features::kWebAssemblyThreads));
 
   // These checks are custom wrappers around base::FeatureList::IsEnabled
   // They're moved here to distinguish them from actual base checks

then I can confirm that our code is correctly disabling the feature.

Furthermore, my test page specifically avoids the isolation headers because the feature is also enabled for cross-origin isolated renderers regardless of the feature flag. Howevever, while it's a good test right now, it will stop working in C91.

The alternative, which will work in C91 or later, is to apply this hack:

--- a/third_party/blink/renderer/core/execution_context/execution_context.cc
+++ b/third_party/blink/renderer/core/execution_context/execution_context.cc
@@ -171,8 +171,9 @@ unsigned ExecutionContext::ContextLifecycleStateObserverCountForTesting()
 }
 
 bool ExecutionContext::SharedArrayBufferTransferAllowed() const {
-  if (RuntimeEnabledFeatures::SharedArrayBufferEnabled(this) ||
-      CrossOriginIsolatedCapability()) {
+  if (RuntimeEnabledFeatures::SharedArrayBufferEnabled(this)) {
+  //if (RuntimeEnabledFeatures::SharedArrayBufferEnabled(this) ||
+  //    CrossOriginIsolatedCapability()) {
     return true;
   }
 #if defined(OS_ANDROID)

so that we can continue to test the flag regardless of the isolation headers.

fmarier added a commit to brave/brave-core that referenced this issue Apr 15, 2021
@fmarier fmarier removed the OS/Android Fixes related to Android browser functionality label Apr 15, 2021
@fmarier
Copy link
Member Author

fmarier commented Apr 15, 2021

So the bottom line is that SharedArrayBuffers are already enabled in Brave and so my PR is essentially just a NOOP cleaning up dead code.

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

Successfully merging a pull request may close this issue.

1 participant