Skip to content

microsoft-edge: fix file picker and substituteInPlace #219170

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2023
Merged

microsoft-edge: fix file picker and substituteInPlace #219170

merged 1 commit into from
Mar 9, 2023

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented Mar 2, 2023 •
edited
Loading

Description of changes

This fixes Microsoft Edge crashing if someone tries to open or save a file via the file picker. For example, to debug the functionality, I went to:

https://drive.google.com -> New -> File upload

Also, I've removed the substituteInPlace for the xdg-settings file because it appears that they don't substitute anything. Maybe another argument for a --fail-on-no-subst flag (https://discourse.nixos.org/t/when-to-use-substituteinplace-functions-vs-a-patch/11073/11).

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/)
  • 23.05 Release Notes (or backporting 22.11 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
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/ms-edge-crash-when-trying-to-load-open-save-dialogs/25621/6

@rikhuijzer
Copy link
Contributor Author

I see you, @JonaEnz and @puffnfresh are also working on microsoft-edge. Mind taking a look at this PR?

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 2, 2023
@JonaEnz
Copy link
Contributor

JonaEnz commented Mar 2, 2023

The --replace "''${XDG... commands seem to break substituteInPlace silently. Adding a backslash like --replace "\''${XDG... solves this issue.
The same problem exists in the substituteInPlace command for xdg-mime.
I don't know what the substitutions do and for what system configurations they are relevant, they have been there since the initial commit, maybe they have always been broken.

@rikhuijzer
Copy link
Contributor Author

Thank you for your review @JonaEnz. I've implemented your suggestion (well spotted by the way) and confirmed that the substitutions now work. The substitutions are to functions such as binary_to_desktop_file() inside xdg-mime. I have no idea when those functions are used, so I don't know how to test the behavior. Therefore, I fixed the substitution in 9cfc523 in the hope that it is useful for some configurations.

At the same time, I also confirmed that the postFixup wrapper that I added is required. Without it, microsoft-edge will still crash on opening a file.

@rikhuijzer
microsoft-edge: fix file picker and subsituteInPlace
a48211c 
This fixes Microsoft Edge crashing if someone tries to open or save a file via the file picker. For example, to debug the functionality, I went to:
```
https://drive.google.com -> New -> File upload
```
This used to crash with the following error:
```
[...]

(microsoft-edge:22536): GLib-GIO-ERROR **: 11:11:46.555: No GSettings schemas are installed on the system
[0302/111146.559740:ERROR:elf_dynamic_array_reader.h(64)] tag not found
[0302/111146.559913:ERROR:process_memory_range.cc(75)] read out of range
[0302/111146.568118:ERROR:watson_metadata.cc(170)] unexpected header
Trace/breakpoint trap (core dumped)
```
This PR fixes this error by explicitly making `gsettings-schemas` from
`gtk` available to `microsoft-edge` via `XDG_DATA_DIRS`.

Furthermore, this PR fixes some broken `subsituteInPlace` commands.
Thanks to Jona Enzinger (`@JonaEnz`) who noticed that `--replace
"''${XDG...` should be written as `--replace "\''${XDG...`.
@rikhuijzer rikhuijzer changed the title microsoft-edge: fix file picker microsoft-edge: fix file picker and substituteInPlace Mar 3, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1900

@SuperSandro2000
Copy link
Member

The --replace "''${XDG... commands seem to break substituteInPlace silently.

It is not broken, it just needs to be escaped properly.

SuperSandro2000
SuperSandro2000 reviewed Mar 4, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Whats with this dangling symlink?

 ▶ ls -lah result/bin/microsoft-edge-stable
lrwxrwxrwx root root 36 B Thu Jan  1 01:00:01 1970  result/bin/microsoft-edge-stable ⇒ /opt/microsoft/msedge/microsoft-edge

@rikhuijzer
Copy link
Contributor Author

The --replace "''${XDG... commands seem to break substituteInPlace silently.

It is not broken, it just needs to be escaped properly.

Yes. Didn't we fix that?

@rikhuijzer
Copy link
Contributor Author

Whats with this dangling symlink?

 ▶ ls -lah result/bin/microsoft-edge-stable
lrwxrwxrwx root root 36 B Thu Jan  1 01:00:01 1970  result/bin/microsoft-edge-stable ⇒ /opt/microsoft/msedge/microsoft-edge

I have no idea. I considered it out of scope for this PR.

@figsoda figsoda added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 4, 2023
@GabrielDougherty
Copy link
Member

Successfully uploaded and downloaded an image to Bing image search with this PR using the file picker.

image

Result of nixpkgs-review pr 219170 run on x86_64-linux 1

2 packages failed to build:
  • microsoft-edge-beta
  • microsoft-edge-dev
1 package built:
  • microsoft-edge

@SuperSandro2000 SuperSandro2000 merged commit 6b4b347 into NixOS:master Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GabrielDougherty GabrielDougherty GabrielDougherty approved these changes

@JonaEnz JonaEnz JonaEnz approved these changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rikhuijzer @nixos-discourse @JonaEnz @SuperSandro2000 @GabrielDougherty @figsoda