Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2070993002:
List all crashes in chrome://crashes, including those not uploaded (Closed)
DescriptionList all crashes in chrome://crashes, including those not uploaded
This will only affect Crashpad platforms (currently Mac and Windows).
This is only a basic first step towards the linked bug, but might even
be slightly useful already for users who are willing to manually attach
a dump to a filed bug.
Screenshot: http://imgur.com/C58HDi5.
R=mark@chromium.org, caitkp@chromium.org
BUG=620762
Committed: https://crrev.com/6d5a71f13c8003163c0cb232de99e159ab8b8a0b
Cr-Commit-Position: refs/heads/master@{#400892}
Patch Set 1 #Patch Set 2 : add OWNERS entry for crash_strings.grdp #
Total comments: 5
Patch Set 3 : pass state through, and support pending #Patch Set 4 : unittest #
Total comments: 14
Patch Set 5 : fixes #Patch Set 6 : fixes #
Total comments: 4
Patch Set 7 : . #
Total comments: 2
Patch Set 8 : . #
Total comments: 8
Dependent Patchsets: Messages
Total messages: 40 (11 generated)
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded Looks like this: http://imgur.com/JGDMpHE BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Looks like this: http://imgur.com/JGDMpHE. R=mark@chromium.org, blundell@chromium.org BUG=620762 ==========
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Looks like this: http://imgur.com/JGDMpHE. R=mark@chromium.org, blundell@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Looks like this: http://imgur.com/JGDMpHE. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ==========
scottmg@chromium.org changed reviewers: + caitkp@chromium.org, mark@chromium.org
Mark for review, Cait for OWNERS addition.
https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:281: g_database->GetCompletedReports(&completed_reports); Since we want to show not-uploaded reports, should we call GetPendingReports() and add those to the list too? Perhaps bubble something up to the UI saying that an upload attempt will still be made for those? https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:288: UploadedReport uploaded_report; UploadedReport is a misnomer now. Given the above comment, maybe it should grow a field that distinguishes between pending, completed-uploaded, and completed-not-uploaded. Then you could rely on that elsewhere instead of further overloading the remote_id field.
https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:281: g_database->GetCompletedReports(&completed_reports); On 2016/06/16 22:02:04, Mark Mentovai wrote: > Since we want to show not-uploaded reports, should we call GetPendingReports() > and add those to the list too? Perhaps bubble something up to the UI saying that > an upload attempt will still be made for those? Yeah, that seems reasonable, but gets more complicated when the state would be expected to change. We'd sort of want to have the page auto refresh as the state changes or something? Maybe it's OK if the user has to manually refresh for that case. https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:288: UploadedReport uploaded_report; On 2016/06/16 22:02:04, Mark Mentovai wrote: > UploadedReport is a misnomer now. > > Given the above comment, maybe it should grow a field that distinguishes between > pending, completed-uploaded, and completed-not-uploaded. Then you could rely on > that elsewhere instead of further overloading the remote_id field. Yeah :(, was being a bit lazy. components/upload_list is misnamed too, so it's going to be a big whack of a CL. I'll see how it goes.
On 2016/06/16 22:28:08, scottmg wrote: > https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... > File components/crash/content/app/crashpad.cc (right): > > https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... > components/crash/content/app/crashpad.cc:281: > g_database->GetCompletedReports(&completed_reports); > On 2016/06/16 22:02:04, Mark Mentovai wrote: > > Since we want to show not-uploaded reports, should we call GetPendingReports() > > and add those to the list too? Perhaps bubble something up to the UI saying > that > > an upload attempt will still be made for those? > > Yeah, that seems reasonable, but gets more complicated when the state would be > expected to change. We'd sort of want to have the page auto refresh as the > state changes or something? Maybe it's OK if the user has to manually refresh > for that case. > > https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... > components/crash/content/app/crashpad.cc:288: UploadedReport uploaded_report; > On 2016/06/16 22:02:04, Mark Mentovai wrote: > > UploadedReport is a misnomer now. > > > > Given the above comment, maybe it should grow a field that distinguishes > between > > pending, completed-uploaded, and completed-not-uploaded. Then you could rely > on > > that elsewhere instead of further overloading the remote_id field. > > Yeah :(, was being a bit lazy. components/upload_list is misnamed too, so it's > going to be a big whack of a CL. I'll see how it goes. Oops, my mistake. UploadList is actually a thing used in other places. I guess UploadList::UploadInfo could reasonably have a state field.
https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:281: g_database->GetCompletedReports(&completed_reports); scottmg wrote: > Yeah, that seems reasonable, but gets more complicated when the state would be > expected to change. We'd sort of want to have the page auto refresh as the > state changes or something? Maybe it's OK if the user has to manually refresh > for that case. At some point, it might be nice to notify on state changes, but we’re probably not at that point yet. It starts to become more interesting once we have a working “upload now” button, so that the progress shows up on about:crashes in real time.
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Looks like this: http://imgur.com/JGDMpHE. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot of pending, uploaded, not uploaded: http://imgur.com/jwbd7WC. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ==========
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot of pending, uploaded, not uploaded: http://imgur.com/jwbd7WC. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot of pending, not uploaded, uploaded: http://imgur.com/jwbd7WC. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ==========
Renamed some things, and now passes state through to UI.
https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:51: default: // Fall through. These are all “enum class” and shouldn’t have other values. If they do wind up with other values, we probably want the compiler to tell us when we’ve forgotten to cover one of them, so we should get rid of this “default” here. https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:82: report.local_id, base::Time(), The time fields are confused. The first time field is supposed to be upload time. The second time field is supposed to be capture time. “creation time” sounds more like the latter than the former. The upload time should be taken from report.last_upload_attempt_time (but only for uploaded reports, unless FromTimeT(0) is correct for non-uploaded reports). https://codereview.chromium.org/2070993002/diff/50001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash/conten... components/crash/content/app/crashpad.cc:287: status = g_database->GetPendingReports(&pending_reports); This is fine right now, but we may want to add an API to Crashpad to get both “completed” and “pending” more atomically when possible, since here, we’ll take the lock and read the metadata two times in succession. https://codereview.chromium.org/2070993002/diff/50001/components/crash/core/b... File components/crash/core/browser/crashes_ui_util.cc (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash/core/b... components/crash/core/browser/crashes_ui_util.cc:50: default: // Fall through. I think we don’t want this “default” either. https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... components/crash_strings.grdp:19: <message name="IDS_CRASH_CRASH_PENDING_UPLOAD" desc="Format for crash entry occurrence on chrome://crashes that was has not yet been uploaded to the server"> Might not be “pending upload”. Might just be pending the decision that it won’t be uploaded at all. Calling this PENDING_UPLOAD seems to imply that if it’s in this state, the user has opted in (or not opted out), but that’s not true. https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... components/crash_strings.grdp:20: Pending crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> “Pending” isn’t really self-explanatory to users. Can we say something better that expresses “maybe we’ll try to upload this, but also maybe not?” https://codereview.chromium.org/2070993002/diff/50001/components/upload_list/... File components/upload_list/upload_list.h (right): https://codereview.chromium.org/2070993002/diff/50001/components/upload_list/... components/upload_list/upload_list.h:50: std::string upload_id; Comment that these fields are only valid in the Uploaded state now.
http://imgur.com/jwbd7WC: Cool! For the not-uploaded ones, the title looks weird. “Crash ID (local_id)” doesn’t look right. Maybe we should flip the IDs to show “Crash ID local_id” for not-uploaded ones, and “Crash ID local_id (remote_id)” for the uploaded ones.
On 2016/06/20 18:02:13, Mark Mentovai wrote: > http://imgur.com/jwbd7WC: > > Cool! > > For the not-uploaded ones, the title looks weird. “Crash ID (local_id)” doesn’t > look right. > > Maybe we should flip the IDs to show “Crash ID local_id” for not-uploaded ones, > and “Crash ID local_id (remote_id)” for the uploaded ones. Done. I also noticed while testing this that it's almost impossible to get a pending report on Windows (I had to change the code to get one) because we don't retry after network failure. Coincidentally someone else ran into the same bug elsewhere over the weekend: https://bugs.chromium.org/p/crashpad/issues/detail?id=123. I assume that the Mac's behaviour is what's intended?
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot of pending, not uploaded, uploaded: http://imgur.com/jwbd7WC. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot: http://imgur.com/C58HDi5. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ==========
Thanks, updated screenshot http://imgur.com/C58HDi5. https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:51: default: // Fall through. On 2016/06/20 17:55:21, Mark Mentovai wrote: > These are all “enum class” and shouldn’t have other values. If they do wind up > with other values, we probably want the compiler to tell us when we’ve forgotten > to cover one of them, so we should get rid of this “default” here. cl doesn't seem to believe that theory ("warning C4715: '`anonymous namespace'::ReportUploadStateToUploadInfoState': not all control paths return a value"). But I switched them to remove the default: and add a NOTREACHED(). https://codereview.chromium.org/2070993002/diff/50001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:82: report.local_id, base::Time(), On 2016/06/20 17:55:21, Mark Mentovai wrote: > The time fields are confused. The first time field is supposed to be upload > time. The second time field is supposed to be capture time. “creation time” > sounds more like the latter than the former. The upload time should be taken > from report.last_upload_attempt_time (but only for uploaded reports, unless > FromTimeT(0) is correct for non-uploaded reports). Yeah, I was wondering about that too. It's a bit convoluted because the UI only displays one time, and we copy things around too much, but I made it somewhat better. https://codereview.chromium.org/2070993002/diff/50001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash/conten... components/crash/content/app/crashpad.cc:287: status = g_database->GetPendingReports(&pending_reports); On 2016/06/20 17:55:21, Mark Mentovai wrote: > This is fine right now, but we may want to add an API to Crashpad to get both > “completed” and “pending” more atomically when possible, since here, we’ll take > the lock and read the metadata two times in succession. Yup, good point. https://codereview.chromium.org/2070993002/diff/50001/components/crash/core/b... File components/crash/core/browser/crashes_ui_util.cc (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash/core/b... components/crash/core/browser/crashes_ui_util.cc:50: default: // Fall through. On 2016/06/20 17:55:21, Mark Mentovai wrote: > I think we don’t want this “default” either. Done (same). https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... components/crash_strings.grdp:19: <message name="IDS_CRASH_CRASH_PENDING_UPLOAD" desc="Format for crash entry occurrence on chrome://crashes that was has not yet been uploaded to the server"> On 2016/06/20 17:55:21, Mark Mentovai wrote: > Might not be “pending upload”. Might just be pending the decision that it won’t > be uploaded at all. Calling this PENDING_UPLOAD seems to imply that if it’s in > this state, the user has opted in (or not opted out), but that’s not true. Done. https://codereview.chromium.org/2070993002/diff/50001/components/crash_string... components/crash_strings.grdp:20: Pending crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> On 2016/06/20 17:55:21, Mark Mentovai wrote: > “Pending” isn’t really self-explanatory to users. Can we say something better > that expresses “maybe we’ll try to upload this, but also maybe not?” Hmm, yeah, that's a bit sticky. How about this? https://codereview.chromium.org/2070993002/diff/50001/components/upload_list/... File components/upload_list/upload_list.h (right): https://codereview.chromium.org/2070993002/diff/50001/components/upload_list/... components/upload_list/upload_list.h:50: std::string upload_id; On 2016/06/20 17:55:21, Mark Mentovai wrote: > Comment that these fields are only valid in the Uploaded state now. Done.
On 2016/06/20 21:08:20, scottmg (slow 20june) wrote: > I assume that the Mac's behaviour is what's intended? No, we never turned on retry anywhere. We should.
LGTM https://codereview.chromium.org/2070993002/diff/90001/chrome/browser/crash_up... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/90001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:44: Lose the blank line. https://codereview.chromium.org/2070993002/diff/90001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/90001/components/crash_string... components/crash_strings.grdp:23: Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> (not yet uploaded or discarded) This is pretty close, but “discarded” isn’t quite right—we’ll still hang on to it locally.
https://codereview.chromium.org/2070993002/diff/90001/chrome/browser/crash_up... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/90001/chrome/browser/crash_up... chrome/browser/crash_upload_list_crashpad.cc:44: On 2016/06/20 21:14:56, Mark Mentovai wrote: > Lose the blank line. Done. https://codereview.chromium.org/2070993002/diff/90001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/90001/components/crash_string... components/crash_strings.grdp:23: Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> (not yet uploaded or discarded) On 2016/06/20 21:14:56, Mark Mentovai wrote: > This is pretty close, but “discarded” isn’t quite right—we’ll still hang on to > it locally. "ignored"? "saved" might sort of work, but could be a bit confusing too.
components/OWNERS lgtm
scottmg@chromium.org changed reviewers: + grt@chromium.org, thestig@chromium.org
Oops, I grew some new owners requirements: +grt: chrome/app/chrome_exe_main_win.cc +thestig: chrome/browser/crash_upload_list_crashpad.cc
I guess “ignored.” LGTM https://codereview.chromium.org/2070993002/diff/110001/components/crash_strin... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/110001/components/crash_strin... components/crash_strings.grdp:22: <message name="IDS_CRASH_CRASH_PENDING" desc="Format for crash entry occurrence on chrome://crashes that has been captured, but not yet uploaded or discarded"> Fix it here too.
Thanks https://codereview.chromium.org/2070993002/diff/110001/components/crash_strin... File components/crash_strings.grdp (right): https://codereview.chromium.org/2070993002/diff/110001/components/crash_strin... components/crash_strings.grdp:22: <message name="IDS_CRASH_CRASH_PENDING" desc="Format for crash entry occurrence on chrome://crashes that has been captured, but not yet uploaded or discarded"> On 2016/06/20 21:25:54, Mark Mentovai wrote: > Fix it here too. Done.
BTW, shall we move c/b/crash_upload* to its own directory with OWNERS one of these days? https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:55: return UploadList::UploadInfo::State::Uploaded; Shouldn't our compilers know all the enum values are covered in the switch() and this is not needed?
https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:55: return UploadList::UploadInfo::State::Uploaded; On 2016/06/20 21:49:30, Lei Zhang wrote: > Shouldn't our compilers know all the enum values are covered in the switch() and > this is not needed? No such luck. https://codereview.chromium.org/2070993002/#msg16 has the warning from cl.exe [[[ I don't think it's illegal to store a 7 in an enum class that only has 0-2 defined, is it? clang doesn't seem to mind either, so I'm not sure that cl is really wrong. [scottmg:~/work/x]$ cat e.cc enum class Foo { A, B, C }; Foo bar = Foo(44); [scottmg:~/work/x]$ clang++ -std=c++11 -Wall -Werror -c e.cc ]]]
On 2016/06/20 21:49:30, Lei Zhang wrote: > BTW, shall we move c/b/crash_upload* to its own directory with OWNERS one of > these days? Yeah, I can move it, or I could just add file-specific entries in c/b/OWNERS? > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > File chrome/browser/crash_upload_list_crashpad.cc (right): > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > chrome/browser/crash_upload_list_crashpad.cc:55: return > UploadList::UploadInfo::State::Uploaded; > Shouldn't our compilers know all the enum values are covered in the switch() and > this is not needed?
On 2016/06/20 22:00:50, scottmg wrote: > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > File chrome/browser/crash_upload_list_crashpad.cc (right): > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > chrome/browser/crash_upload_list_crashpad.cc:55: return > UploadList::UploadInfo::State::Uploaded; > On 2016/06/20 21:49:30, Lei Zhang wrote: > > Shouldn't our compilers know all the enum values are covered in the switch() > and > > this is not needed? > > No such luck. https://codereview.chromium.org/2070993002/#msg16 has the warning > from cl.exe > > [[[ I don't think it's illegal to store a 7 in an enum class that only has 0-2 > defined, is it? clang doesn't seem to mind either, so I'm not sure that cl is > really wrong. > > [scottmg:~/work/x]$ cat e.cc > enum class Foo { A, B, C }; > Foo bar = Foo(44); > [scottmg:~/work/x]$ clang++ -std=c++11 -Wall -Werror -c e.cc > ]]] Hmm. Grumble stupid enums grumble lgtm.
On 2016/06/20 22:01:32, scottmg wrote: > On 2016/06/20 21:49:30, Lei Zhang wrote: > > BTW, shall we move c/b/crash_upload* to its own directory with OWNERS one of > > these days? > > Yeah, I can move it, or I could just add file-specific entries in c/b/OWNERS? It's a little bit more work, but it would be good to reduce the clutter in c/b.
https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:19: const crash_reporter::Report** reports, const betwixt the two stars? The caller of this shouldn't be able to modify the array, should they? https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:31: if (get_crash_reports) { I don't follow this. Apologies if this is a an uninformed question, but does this run the lambda twice, once here and once below? If yes, why not ensure that it only happens once? If no, then how is this actually working? Is this condition merely checking whether or not the lambda has a value, and not actually run it at all?
https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:19: const crash_reporter::Report** reports, On 2016/06/20 23:26:08, grt (slow) wrote: > const betwixt the two stars? The caller of this shouldn't be able to modify the > array, should they? My const placement cdecl skillz are crud, but doesn't `X* const* x` mean I can't change the value of x? reports is the address of a pointer (so should mutable) *reports is the array we're retrieving. We don't want to change the value of that pointer from the callee's perspective, but the pointer is copied by value here (so has to be mutable) (*reports)[0..N-1] are the reports we're referencing (and those definitely shouldn't be changed... that's the leading existing const I think, right?) Hrm. Sorry if I'm lost and sound-like-it here. :) https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:31: if (get_crash_reports) { On 2016/06/20 23:26:08, grt (slow) wrote: > I don't follow this. Apologies if this is a an uninformed question, but does > this run the lambda twice, once here and once below? If yes, why not ensure that > it only happens once? If no, then how is this actually working? Is this > condition merely checking whether or not the lambda has a value, and not > actually run it at all? Just once; the sneaky part is the () after the closing } in the definition of the lambda, so the if is checking the value of the pointer returned by running the lambda right after it's defined. It might be a bit too clever. I think the idea was that it avoids requerying repeatedly in the case that the function is not found assuming it was written instead as: static Pointer p = nullptr; if (!p) { p = GetProcAddress(...) }
chrome/app lgtm https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... File chrome/browser/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:19: const crash_reporter::Report** reports, On 2016/06/21 00:06:30, scottmg wrote: > On 2016/06/20 23:26:08, grt (slow) wrote: > > const betwixt the two stars? The caller of this shouldn't be able to modify > the > > array, should they? > > My const placement cdecl skillz are crud, but doesn't `X* const* x` mean I can't > change the value of x? > > reports is the address of a pointer (so should mutable) > > *reports is the array we're retrieving. We don't want to change the value of > that pointer from the callee's perspective, but the pointer is copied by value > here (so has to be mutable) > > (*reports)[0..N-1] are the reports we're referencing (and those definitely > shouldn't be changed... that's the leading existing const I think, right?) > > Hrm. Sorry if I'm lost and sound-like-it here. :) > Ooops, you're right. I was thinking that the other file was working with a vector of pointers to reports rather than a vector of reports themselves. Apologies for the noise. This is what I get for trying to do a code review on a phone while cooking dinner. :-( https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... chrome/browser/crash_upload_list_crashpad.cc:31: if (get_crash_reports) { On 2016/06/21 00:06:29, scottmg wrote: > On 2016/06/20 23:26:08, grt (slow) wrote: > > I don't follow this. Apologies if this is a an uninformed question, but does > > this run the lambda twice, once here and once below? If yes, why not ensure > that > > it only happens once? If no, then how is this actually working? Is this > > condition merely checking whether or not the lambda has a value, and not > > actually run it at all? > > Just once; the sneaky part is the () after the closing } in the definition of > the lambda, so the if is checking the value of the pointer returned by running > the lambda right after it's defined. > > It might be a bit too clever. I think the idea was that it avoids requerying > repeatedly in the case that the function is not found assuming it was written > instead as: > > static Pointer p = nullptr; > if (!p) { > p = GetProcAddress(...) > } Ah, very clever indeed. I shouldn't review on phone. All of the scrolling around makes it hard to see the forest from the trees.
Thanks. On 2016/06/21 01:32:50, grt (no reviews Jun 23-Jul 18) wrote: > chrome/app lgtm > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > File chrome/browser/crash_upload_list_crashpad.cc (right): > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > chrome/browser/crash_upload_list_crashpad.cc:19: const crash_reporter::Report** > reports, > On 2016/06/21 00:06:30, scottmg wrote: > > On 2016/06/20 23:26:08, grt (slow) wrote: > > > const betwixt the two stars? The caller of this shouldn't be able to modify > > the > > > array, should they? > > > > My const placement cdecl skillz are crud, but doesn't `X* const* x` mean I > can't > > change the value of x? > > > > reports is the address of a pointer (so should mutable) > > > > *reports is the array we're retrieving. We don't want to change the value of > > that pointer from the callee's perspective, but the pointer is copied by value > > here (so has to be mutable) > > > > (*reports)[0..N-1] are the reports we're referencing (and those definitely > > shouldn't be changed... that's the leading existing const I think, right?) > > > > Hrm. Sorry if I'm lost and sound-like-it here. :) > > > > Ooops, you're right. I was thinking that the other file was working with a > vector of pointers to reports rather than a vector of reports themselves. > Apologies for the noise. This is what I get for trying to do a code review on a > phone while cooking dinner. :-( No problem, I don't often consider putting const other than in the "normal" place. I should think about that more often. > > https://codereview.chromium.org/2070993002/diff/130001/chrome/browser/crash_u... > chrome/browser/crash_upload_list_crashpad.cc:31: if (get_crash_reports) { > On 2016/06/21 00:06:29, scottmg wrote: > > On 2016/06/20 23:26:08, grt (slow) wrote: > > > I don't follow this. Apologies if this is a an uninformed question, but does > > > this run the lambda twice, once here and once below? If yes, why not ensure > > that > > > it only happens once? If no, then how is this actually working? Is this > > > condition merely checking whether or not the lambda has a value, and not > > > actually run it at all? > > > > Just once; the sneaky part is the () after the closing } in the definition of > > the lambda, so the if is checking the value of the pointer returned by running > > the lambda right after it's defined. > > > > It might be a bit too clever. I think the idea was that it avoids requerying > > repeatedly in the case that the function is not found assuming it was written > > instead as: > > > > static Pointer p = nullptr; > > if (!p) { > > p = GetProcAddress(...) > > } > > Ah, very clever indeed. I shouldn't review on phone. All of the scrolling around > makes it hard to see the forest from the trees.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/2070993002/#ps130001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070993002/130001
Message was sent while issue was closed.
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot: http://imgur.com/C58HDi5. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot: http://imgur.com/C58HDi5. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot: http://imgur.com/C58HDi5. R=mark@chromium.org, caitkp@chromium.org BUG=620762 ========== to ========== List all crashes in chrome://crashes, including those not uploaded This will only affect Crashpad platforms (currently Mac and Windows). This is only a basic first step towards the linked bug, but might even be slightly useful already for users who are willing to manually attach a dump to a filed bug. Screenshot: http://imgur.com/C58HDi5. R=mark@chromium.org, caitkp@chromium.org BUG=620762 Committed: https://crrev.com/6d5a71f13c8003163c0cb232de99e159ab8b8a0b Cr-Commit-Position: refs/heads/master@{#400892} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6d5a71f13c8003163c0cb232de99e159ab8b8a0b Cr-Commit-Position: refs/heads/master@{#400892} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
