Suggest GitHub template when flutter tool crashes#45360
Suggest GitHub template when flutter tool crashes#45360jmagman merged 5 commits intoflutter:masterfrom
Conversation
jmagman
added
c: contributor-productivity
There was a problem hiding this comment.
I wanted this to be printError but when I changed it to that it always printed before some of the above status lines. I'm guessing stderr has more aggressive flush behavior, and none of the stdout flushes I tried seemed to help. Open to suggestions.
There was a problem hiding this comment.
Unfortunately it looks like dart:io stdout.flush() only flushes any buffering in Dart's memory. It doesn't actually result in a flush() call on the underlying fd to the OS.
There was a problem hiding this comment.
I'm okay with an angry-looking red status.
There was a problem hiding this comment.
I switched this back to an error at the top since now the output is much smaller and I'm less worried about it getting scrolled out of view.
There was a problem hiding this comment.
Keeping this vanilla and bland: something Bad is happening.
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
This is great. Just took a quick look so far
There was a problem hiding this comment.
Unfortunately it looks like dart:io stdout.flush() only flushes any buffering in Dart's memory. It doesn't actually result in a flush() call on the underlying fd to the OS.
There was a problem hiding this comment.
Hopefully the doctor won't crash, but I think it would be good to guard against it separately and continue with printing the github issue instructions.
There was a problem hiding this comment.
doctor exceptions are already caught in _doctorText.
There was a problem hiding this comment.
Consider whether moving some or all of the code under the else on 115 into a separate method would improve readability of _handleToolError.
There was a problem hiding this comment.
There was enough complicated try/catch and early returns that I just pulled out the wall-o-text part. Hopefully that makes it readable enough.
There was a problem hiding this comment.
I moved it into its own file.
@zanderso Are you cool with that being a future improvement, or do you want the URL shortening in this PR? |
If you can do it in this PR that would be great. If splitting it up makes life a lot easier, I would prefer that this not land until after the current release cycle so we don't have the big URLs on stable. |
@zanderso
|
There was a problem hiding this comment.
Long lines like this can be split up like:
final String gitHubTemplateURL = await gitHubTemplateCreator.toolCrashIssueTemplateGitHubURL(
command,
errorString,
_crashException(error),
stackTrace,
doctorText,
);There was a problem hiding this comment.
Future<String> toolCrashIssueTemplateGitHubURL(
String command,
String errorString,
String exception,
StackTrace stackTrace,
String doctorText
) async {
...
}There was a problem hiding this comment.
It looks like manifest has already been checked for null above on line 77.
There was a problem hiding this comment.
Best to wrap these file system calls in a try-catch.
There was a problem hiding this comment.
expect(
await creator.toolCrashSimilarIssuesGitHubURL('this is a 100% error'),
_kShortURL,
);There was a problem hiding this comment.
| final List<int> bodyBytes = utf8.encode('url=${Uri.encodeQueryComponent(fullURL)}'); | ||
| final HttpClientRequest request = await _client.postUrl(Uri.parse('https://git.io')); | ||
| request.headers.set(HttpHeaders.contentLengthHeader, bodyBytes.length.toString()); | ||
| request.add(bodyBytes); |
There was a problem hiding this comment.
I didn't realize this code existed until just now... surely this is potentially sending PII to GitHub? This seems like a problem.
|
My recollection is that the information is sanitized to contain only information about tool internals e.g. a sanitized exception message and a stack trace containing only flutter_tool frames, but @jmagman and @christopherfujino should verify. We should also revisit whether this has improved the quality of filed issues. Maybe @stuartmorgan can help us ask the triagers if issues filed with this template have good signal. |
|
For reference here is an example #54213 |
I can ask, but given that the audience for the stack information is primarily the tools team, not first-stage triagers, wouldn't having the tools team audit https://github.com/flutter/flutter/issues?q=is%3Aissue+tool_crash+in%3Atitle for value tell us more about how well the template works? |
|
It includes paths (which can include usernames), device IDs, potentially private plugin names, potentially sensitive arguments... |
|
For the record I've nothing against us filing bugs with this information, it's only the short link creation that's a problem because we don't prompt the user before uploading the information. |
|
See #53140 for discussion about removing the URL shortener. |
Feedback is the template is providing good signal (ability to effectively dup, avoiding round-trip requests for more information, etc.). So eliminating it completely would definitely not be ideal. |
6 participants
Description
When the Flutter tool crashes, suggest a GitHub template that contains the command, stacktrace, flutter doctor -v, and some metadata if the working directory is a Flutter project.
I discussed with @InMatrix and he suggested we not show the "Sending crash report to Google." with the report ID by default, because users will think they don't need to report their crash because something was automatically sent, so I moved that line to a trace log.
Also,
usesAndroidXwas crashing if the module section wasn't present, so I fixed that.The GitHub URL has been shortened with GitHub's URL shortener API.
Example
I faked out a crash.
And here's what it looks like visually to the user:
And here's the GitHub template when you click on that big link (try it!):
Title
[tool_crash] Exception: VersionCheckError: Command exited with code -9: git log -n 1 --pretty=format:%ad --date=iso
===========BODY START===========
Command
Steps to Reproduce
Logs
_Exception: Exception: VersionCheckError: Command exited with code -9: git log -n 1 --pretty=format:%ad --date=iso
Flutter Application Metadata
Version: 1.0.0+1
Material: true
Android X: false
Module: false
Plugin: false
Android package: null
iOS bundle identifier: null
Plugins
camera
===========BODY END===========
(The ===========BODY=========== ASCII art isn't in there, it's to try to make this issue-in-PR description easier to see)
Possible Future Work
Use URL shortener for that long link (cons: networking while crashing, though we do send that crash report).Done!Related Issues
Fixes #44126.
Fixes #45617.
Tests
Added a runner test, added new GitHub template tests, fixed up a few others based on the new input.
Added
usesAndroidXtests.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change