Skip to content

firefox: fix crash, enable debug symbols and clean up#146784

Merged
mweinelt merged 4 commits intoNixOS:masterfrom
oxalica:test/firefox-crash
Nov 21, 2021
Merged

firefox: fix crash, enable debug symbols and clean up#146784
mweinelt merged 4 commits intoNixOS:masterfrom
oxalica:test/firefox-crash

Conversation

@oxalica
Copy link
Contributor

@oxalica oxalica commented Nov 20, 2021 •
edited
Loading

Motivation for this change

  1. Fix YouTube tab crashes in recent Firefox (nixpkgs build only) #146382 by disabling jemalloc by default.
    It's caused by jemalloc violates clang's alignment assumption of malloc from LLVM 13. Check details in https://bugzilla.mozilla.org/show_bug.cgi?id=1741454

    Try this POC to confirm it's actually fixed by this PR.

  2. Enable separated debug symbols by default for easy debugging.

  3. Clean up unnecessary makeFlags and NIX_LDFLAGS since we're now using LLVM stdenv.

  4. Use --enable-lto=cross. I add this to make our configure flags closer to Arch's, and potentially make it more optimized.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@oxalica oxalica mentioned this pull request Nov 20, 2021
Closed
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 20, 2021
@Artturin
Copy link
Member

Artturin commented Nov 20, 2021

lets set , crashreporterSupport ? false to , crashreporterSupport ? !privacySupport

@oxalica
Copy link
Contributor Author

oxalica commented Nov 20, 2021 •
edited
Loading

lets set , crashreporterSupport ? false to , crashreporterSupport ? !privacySupport

crashreporter is not quite related to what's done in this PR, and I'd think it's not useful currently. Since generated crash report still doesn't have symbols even after this PR. They require building a custom symbol pack with a custom tool dump_syms and uploading it to mozilla symbol server with an account. See Mozilla docs. This process is usually done by distribution, like uploading in Arch's PKGBUILD. But of course Hydra cannot and should not do this.

If you want to package dump_syms and the symbol package, or have better approach, please create a separated issue or PR.

das-g
das-g reviewed Nov 20, 2021
View reviewed changes
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

Just some comments on comments:

pkgs/applications/networking/browsers/firefox/common.nix Outdated
Copy link
Member

@das-g das-g Nov 20, 2021

Choose a reason for hiding this comment

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

Do we disable "adding -g" on all platforms, so that linking is easier on 32bit platform? Or do we disable "adding -g" only on 32bit platform, for easier linking there? If the latter (which I assume from the code below), maybe phrase it as:

Suggested change
# We disable adding "-g" for easier linking on 32bit platform.
# On 32bit platform, we disable adding "-g" for easier linking.

so that it's clearer, what "on 32bit platform refers to.

Also, is "32bit" just one platform or a category of (potentially or actually) several platforms? If the latter, put it in plural:

Suggested change
# We disable adding "-g" for easier linking on 32bit platform.
# On 32bit platforms, we disable adding "-g" for easier linking.

Copy link
Contributor Author

@oxalica oxalica Nov 21, 2021

Choose a reason for hiding this comment

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

About 32bit platforms, I notice that we actually mark firefox broken when buildPlatform is 32bit, in,
https://github.com/NixOS/nixpkgs/blob/07534dee7f45a9945123e7bd1580b6c22bb4cdc5/pkgs/applications/networking/browsers/firefox/packages.nix#L22

Maybe we could remove these special cases in common.nix, but I'm not sure about testing 32bit builds. So I just keep them currently.

pkgs/applications/networking/browsers/firefox/common.nix Outdated
Copy link
Member

@das-g das-g Nov 20, 2021

Choose a reason for hiding this comment

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

For those familiar with LTO (which I'm not), does the comment "Cross-language LTO." add any information, or would they know that already from the code --enable-lto=cross?

Copy link
Contributor Author

@oxalica oxalica Nov 21, 2021

Choose a reason for hiding this comment

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

This option is parsed by mozconfig, I think it's not a common phrasing about LTO. At least I've never seen lto=cross anywhere else, and I learn its meaning after grepping firefox source tarball.

@collares collares mentioned this pull request Nov 20, 2021
Closed
13 tasks
@oxalica oxalica requested a review from das-g November 21, 2021 04:25
das-g
das-g approved these changes Nov 21, 2021
View reviewed changes
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

LGTM now. (As far as I can assess it, at least.)

Mic92
Mic92 approved these changes Nov 21, 2021
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on youtube with x86_64-linux/sway. No more crashes. Video preview works now.

@Mic92
Copy link
Member

Mic92 commented Nov 21, 2021 •
edited
Loading

One can test firefox-wayland like this:

nix-store -r /nix/store/xvsiw2v6nwahqwzs767xl0dj66xkifzv-firefox-94.0.1/ --option binary-caches 'https://cache.nixos.org/ https://mic92.cachix.org/' --option trusted-public-keys 'mic92.cachix.org-1:gi8IhgiT3CYZnJsaW7fxznzTkMUOn1RY4GmXdT/nXYQ='

It's the result of this build: https://gitlab.com/Mic92/dotfiles/-/jobs/1804561072#L62

mweinelt
mweinelt approved these changes Nov 21, 2021
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Thank you for debugging this issue and coming up with a solution. I've tested the resulting binary on x86_64-linux sway/pipewire and everything LGTM.

@mweinelt mweinelt merged commit 1debd0c into NixOS:master Nov 21, 2021
@oxalica oxalica deleted the test/firefox-crash branch November 21, 2021 17:07
@vcunat
Copy link
Member

vcunat commented Nov 21, 2021

I'm afraid this PR broke nixosTests.firefox, blocking the nixos-unstable channel. So far I only blindly tried it on the commits directly before and after merging.

@Artturin
Copy link
Member

Artturin commented Nov 21, 2021 •
edited
Loading

I'm afraid this PR broke nixosTests.firefox, blocking the nixos-unstable channel. So far I only blindly tried it on the commits directly before and after merging.

this didn't break it

Kernel panic - not syncing: Out of memory: compulsory panic_on_oom is enabled its running out of ram and it was fixed in #146804 (just ran both the firefox and -esr tests locally)

@mweinelt mweinelt mentioned this pull request Nov 22, 2021
Closed
@wkral
Copy link
Contributor

wkral commented Nov 23, 2021

The latest channel update for nixos-unstable as of a few hours ago 263ef4c has a fixed version of firefox (just verified myself) for anyone who was running into this issue. Thanks to the folks who worked on this.

@oxalica oxalica mentioned this pull request Dec 1, 2021
Merged
13 tasks
@collares collares mentioned this pull request May 11, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 approved these changes

@mweinelt mweinelt mweinelt approved these changes

@lovesegfault lovesegfault Awaiting requested review from lovesegfault

@edolstra edolstra Awaiting requested review from edolstra

@vcunat vcunat Awaiting requested review from vcunat

@nbp nbp Awaiting requested review from nbp

+2 more reviewers

@das-g das-g das-g approved these changes

@trofi trofi trofi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

YouTube tab crashes in recent Firefox (nixpkgs build only)

8 participants

@oxalica @Artturin @Mic92 @vcunat @wkral @das-g @mweinelt @trofi