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

Freetype crash regression test #18338

Merged
merged 2 commits into from
May 4, 2023
Merged

Freetype crash regression test #18338

merged 2 commits into from
May 4, 2023

Conversation

rillian
Copy link
Contributor

@rillian rillian commented May 2, 2023

Add a regression test for the release crash we saw on some Linux systems with Brave 1.50.121.

Note this requires a release build to reproduce, but does seem to trigger when such runs in ci, so this should work to catch a re-occurance.

Points for review:

  • Seems like this should be a webtest, but doesn't look like we run those(?) so I cloned a browsertest.
  • I wasn't able to minimize the triggering font, so this is a 884 KB test. If that's a problem I can write some tooling to isolate the trigger, but I've already spent quite a bit of time on it.
  • Test file is verbatim font file from the Fedora google-noto-sans-vf-fonts-20201206^1.git0c78c8329-7.fc37.noarch package, under the SIL Open Font License 1.1, included in the file.

Resolves brave/brave-browser#29957

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 wiki
    • npm run lint, npm run presubmit wiki, 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:

@rillian rillian self-assigned this May 2, 2023
@rillian rillian requested review from mkarolin and wknapik May 2, 2023 21:39
Comment on lines 42 to 47
content::WebContents* web_contents() {
return browser()->tab_strip_model()->GetActiveWebContents();
}

content::RenderFrameHost* primary_main_frame() {
return web_contents()->GetPrimaryMainFrame();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these needed (and related header #includes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed they are not. Thanks!
Fixed in 539277fa27134081c164f645564b730b346ea0da.

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

LGTM

@rillian rillian enabled auto-merge May 4, 2023 18:12
@rillian rillian disabled auto-merge May 4, 2023 18:16
The Brave 1.50.121 release crashed loading certain pages on
Linux. Add a regression test based on an isolated reproduction.
The issue seemed to be using the Google Noto Sans VF shipped
as a system font by Fedora, Arch, and a few other distributions
inside a `<select>` element.

This adds a test page, and loads it as part of a browser test.
The test page is minified from https://brave.com/ which also
showed the issue. The font in question was reduced in fontforge
to just lowercase latin glyphs, with all additional tables
removed so save space. The result was included directly in
the test page using a `data:` url.

Resolves brave/brave-browser#29957
Fix css errors which were preventing the page from using the
minimized font from the data: url. Unfortunately this revealed
that the minimized version doesn't actually reproduce the problem;
it was still falling back to the system font.

The should be a stand-alone reproduction, at the expense of an 844 kB
test file.
@rillian
Copy link
Contributor Author

rillian commented May 4, 2023

ci being weird. rebased, squashing fixups.

@rillian rillian enabled auto-merge May 4, 2023 18:20
@rillian rillian merged commit a441fb7 into master May 4, 2023
@rillian rillian deleted the freetype-regression branch May 4, 2023 21:46
@github-actions github-actions bot added this to the 1.53.x - Nightly milestone May 4, 2023
@rillian rillian restored the freetype-regression branch May 4, 2023 21:49
brave-builds added a commit that referenced this pull request May 4, 2023
brave-builds added a commit that referenced this pull request May 4, 2023
kjozwiak pushed a commit that referenced this pull request May 8, 2023
kjozwiak pushed a commit that referenced this pull request May 8, 2023
@bsclifton bsclifton deleted the freetype-regression branch November 10, 2023 06:14
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.

Add tests to detect the kind of issues we had on v1.50.121 on Linux
2 participants