Skip to content

fix: corner smoothing feature gate crash #47759

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

Merged
merged 2 commits into from
Jul 16, 2025

Conversation

clavin
Copy link
Member

@clavin clavin commented Jul 15, 2025

Description of Change

There was a crash in third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc from parsing the -electron-corner-smoothing CSS property when the property was added to a stylesheet with no associated document. To resolve this, I changed the feature gating to use Blink's built-in runtime feature gating instead. As a result, I was able to cut out a majority of the patch that was just for plumbing the web preference. There is an insignificant API change for how the feature is disabled.

Checklist

Release Notes

Notes: Fixed a crash when adding the -electron-corner-smoothing CSS rule to a stylesheet with no associated document.

@clavin clavin requested a review from a team as a code owner July 15, 2025 18:57
@clavin clavin added semver/patch backwards-compatible bug fixes target/37-x-y PR should also be added to the "37-x-y" branch. target/38-x-y PR should also be added to the "38-x-y" branch. labels Jul 15, 2025
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 15, 2025
}
})
```

The CSS rule will still parse, but will have no visual effect.
Copy link
Member Author

Choose a reason for hiding this comment

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

This removal was intentional: the built-in runtime feature gating will prevent this rule from parsing, similar to how experimental CSS properties work.

Comment on lines +292 to +297
+ // Since the Electron implementation of DrawSmoothRoundRect uses one radius
+ // for both dimensions, we need to use the minimum of the two supplied.
+ auto min_radius = [](const gfx::SizeF& radius) -> float {
+ return std::min(radius.width(), radius.height());
+ };
+
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive-by fix for a bug I ran into while testing

--- a/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
+++ b/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
@@ -12325,5 +12325,25 @@ const CSSValue* InternalEmptyLineHeight::ParseSingleValue(
@@ -12325,5 +12325,31 @@ const CSSValue* InternalEmptyLineHeight::ParseSingleValue(
CSSValueID::kNone>(stream);
}

+const CSSValue* ElectronCornerSmoothing::ParseSingleValue(
+ CSSParserTokenStream& stream,
+ const CSSParserContext& context,
+ const CSSParserLocalContext&) const {
Copy link
Member

Choose a reason for hiding this comment

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

With the blink runtime flag disabled, is this codepath no longer entered? Just want to confirm whether that's the reason why feature checking is no longer needed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct 👍 For comparison, other experimental CSS properties rely on the JSON definition file to supply the required flag, instead of the parsing code here.

@absidue
Copy link

absidue commented Jul 15, 2025

Is there any public discussion that I could read through on why this non-standard Electron-only CSS property is enabled by default? It seems like an odd choice when File.path was removed because it was non-standard and the non-standard <webview> tag is disabled by default. Was it just a mistake that it is enabled by default or has Electron's policy on not adding non-standard stuff changed?

@clavin
Copy link
Member Author

clavin commented Jul 16, 2025

@absidue First off, thanks for the feedback! That is a great question to raise. I've thought about this a lot during this feature's development.

The design & rationale for this feature was captured in RFC 12 and its corresponding PR. The non-standard-ness was a point of discussion I proactively brought up in the RFC text and during the API Working Group meetings.

Your question about consistency with other non-standard APIs is fair to ask. Our thinking was based on precedent, ergonomic design, and risk assessment:

  • Electron does support specific non-standard CSS properties when they solve a platform need. The best example is -webkit-app-region1, commonly used for specifying custom draggable regions. While this property is implemented in Chromium2, its modern use is almost exclusively for Electron apps. Adding another targeted non-standard property (with appropriate standard-supported vendor prefixing) didn't feel like a departure from this practice.

  • From an ergonomic standpoint, we want features to have minimal friction (unless there is a significant concern with its safety, of course). If this property were disabled by default, developers wanting to use it would have to enable a flag before they can use it. By enabling it by default, developers who want it can simply use the CSS property, and those who don't can simply avoid it. Adding an extra configuration step (a "speedbump") for developers wasn't justified without a high-risk drawback.

  • You're right that File.path and <webview> were removed/disabled in part due to being non-standard. However, their primary issues were severe security and privacy risks: File.path could expose sensitive data (usernames and confidential project names); <webview> had significant security implications. While the corner smoothing property does introduce a fingerprinting vector (a valid privacy concern!), we judged its risk to be in a different, lower class than the others.

To directly answer your last question: Electron's policy on avoiding non-standard APIs hasn't changed. We still weigh the purpose and utility of a feature against its drawbacks—security, privacy, and non-standard-ness. In this case, we felt the feature was an appropriate addition to the platform.

All that said, we're open to revisiting this decision. You're one of the first people to reach out to me about this topic, so if you feel the trade-off isn't right and have a motivating explanation for why corner smoothing should be disabled by default, then please feel free to open a new discussion issue!

Footnotes

  1. The app-region CSS property was proposed to be included in WCO, which could have standardized it. I believe the app-region spelling is implemented and usable in Electron right now as part of the WCO experiment despite not being an accepted standard yet.

  2. Historically, this was implemented for the deprecated Chrome Apps platform before Chromium forked WebKit to make Blink. That is to say, this property has a rich and detailed history.

@VerteDinde VerteDinde merged commit 389927d into main Jul 16, 2025
98 of 102 checks passed
@VerteDinde VerteDinde deleted the clavin/fix-corner-smoothing-feature-gate branch July 16, 2025 15:39
@release-clerk
Copy link

release-clerk bot commented Jul 16, 2025

Release Notes Persisted

Fixed a crash when adding the -electron-corner-smoothing CSS rule to a stylesheet with no associated document.

@trop
Copy link
Contributor

trop bot commented Jul 16, 2025

I was unable to backport this PR to "37-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/37-x-y and removed target/37-x-y PR should also be added to the "37-x-y" branch. labels Jul 16, 2025
@trop
Copy link
Contributor

trop bot commented Jul 16, 2025

I have automatically backported this PR to "38-x-y", please check out #47785

@trop trop bot added in-flight/38-x-y and removed target/38-x-y PR should also be added to the "38-x-y" branch. labels Jul 16, 2025
georgexu99 pushed a commit that referenced this pull request Jul 16, 2025
* fix: corner smoothing feature gate crash

* Fix ElectronCornerSmoothing::CSSValueFromComputedStyleInternal
@trop
Copy link
Contributor

trop bot commented Jul 16, 2025

@clavin has manually backported this PR to "37-x-y", please check out #47792

VerteDinde pushed a commit that referenced this pull request Jul 16, 2025
fix: corner smoothing feature gate crash (#47759)
@trop trop bot added merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. and removed in-flight/37-x-y in-flight/38-x-y labels Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. new-pr 🌱 PR opened recently semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants