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

proposal: runtime: enable WER dumps for Windows under certain circumstances #49471

Open
zx2c4 opened this issue Nov 9, 2021 · 11 comments
Open

proposal: runtime: enable WER dumps for Windows under certain circumstances #49471

zx2c4 opened this issue Nov 9, 2021 · 11 comments
Labels
Projects
Milestone

Comments

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Nov 9, 2021

CL 307372 was committed, which contained a nice change to produce crashdumps under certain circumstances. However, several questions were proposed about when those crashdumps would be automatically uploaded to Microsoft in the context of using Insider Builds. In the absence of good answers to that, the otherwise okay CL was reverted (by me) with CL 362454. This proposal is essentially about reverting the revert, and having an extended discussion about under which circumstances these should be enabled and whether we need a new flag for it, as suggested by @bufflig.

Before this proposal moves to an "under consideration" step, we should get a clearer idea about what we're proposing. I'll defer to @zhizheng052 and @bhiggins on this, as they wrote the original CL.

CC @zhizheng052 @bhiggins @bufflig @ianlancetaylor @alexbrainman @aarzilli @aclements

@gopherbot gopherbot added this to the Proposal milestone Nov 9, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 9, 2021
@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 9, 2021

In the CL discussion, there was some question as to when "optional" diagnostics gets enabled. I was just installing a fresh Windows 11 board a few minutes ago and noticed that this was checked by default:

image

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Nov 9, 2021

It seems to me that if you opt to send you crash dumps to microsoft during install (by not opting out) and then opt into creating a crash dump by setting GOTRACEBACK=crash, then having your crash dump uploaded to microsoft is the expected behavior. Given that this only happens on insider builds, it isn't even that sketchy that the option is preselected.
Since this is already gated by two options I don't think adding a third opt-in (through a new flag) is going to make a difference. IMO it's either:

  • add a courtesy reminder that windows does this in the documentation of GOTRACEBACK (and maybe the release notes), or
  • do not implement GOTRACEBACK=crash on windows and document that this is the reason.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 9, 2021

Given that this only happens on insider builds, it isn't even that sketchy that the option is preselected.

This wasn't an insider build; I was installing the Windows 11 final release.

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Nov 9, 2021

Given that this only happens on insider builds, it isn't even that sketchy that the option is preselected.

This wasn't an insider build; I was installing the Windows 11 final release.

That sucks. Still, I don't think it changes the rest of my opinion.

@bufflig
Copy link
Contributor

@bufflig bufflig commented Nov 9, 2021

I assume there's something in the registry that enables the "sending to MS" behavior. I wonder if we could detect that and either:

a) Not enable WER or
b) Enable WER but warn the user or
c) Simply refuse to run when you have that combination (GOTRACEBACK=crash and uploading is enabled)?

Enabling GOTRACEBACK=crash even when you don't actually want the file on disk is not an unreasonable functionality, and was there before this change (it gives you some additional crash information). To get the added behavior that you upload crashdumps to a third party does not seem expected in those cases. At least a stern warning seems mandated, but I'd actually prefer to disable/revert the change for 1.18 and decide exactly how we want to proceed.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Nov 9, 2021

I'd actually prefer to disable/revert the change for 1.18 and decide exactly how we want to proceed.

This has already happened, FYI.

There's also the WER option to pop up a UI asking for consent. I suspect this won't work in services, though.

Also, I wonder if we could ditch this whole WER set of options and just create a minidump ourselves? https://docs.microsoft.com/en-us/windows/win32/api/minidumpapiset/nf-minidumpapiset-minidumpwritedump

@elagergren-spideroak
Copy link

@elagergren-spideroak elagergren-spideroak commented Nov 9, 2021

To clarify: this is only when GOTRACEBACK=crash is set, right?

@rsc rsc changed the title proposal: enable WER dumps for Windows under certain circumstances proposal: runtime: enable WER dumps for Windows under certain circumstances Dec 1, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Dec 1, 2021
@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Dec 1, 2021

From the top post:

Before this proposal moves to an "under consideration" step, we should get a clearer idea about what we're proposing.

For the record, I still don't quite know what we're proposing here.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 8, 2021

At least the proposal discussion will get more eyes on it.
What are the options? Is it possible to write a crash dump that doesn't get collected and sent off?

@rsc
Copy link
Contributor

@rsc rsc commented Jan 5, 2022

It sounds like we are still waiting to find out what exactly is being proposed here.
Does anyone know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants