Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
988063003:
Define the Settings interface for a CrashReportDatabase and provide a Mac implementation. (Closed)
|
Created:
5 years, 9 months ago by Robert Sesek Modified:
5 years, 9 months ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
DescriptionDefine the Settings interface for a CrashReportDatabase and provide a Mac implementation.
R=mark@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/1a635e3a799c9c93b39d1a9f507b3c9eebbd873b
Patch Set 1 #
Total comments: 31
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Drop plist, use binary #
Total comments: 48
Patch Set 4 : Address comments #
Total comments: 12
Patch Set 5 : #Patch Set 6 : Sync #Patch Set 7 : INITIALIZATION_STATE_DCHECK #
Total comments: 4
Patch Set 8 : For landing #
Messages
Total messages: 15 (1 generated)
I didn't integrate the last upload time into CrashReportDatabase, because I can't test that until there's a Windows implementation.
This is a partial review but I wanted to give a quick turnaround. https://codereview.chromium.org/988063003/diff/1/client/crash_report_database.h File client/crash_report_database.h (right): https://codereview.chromium.org/988063003/diff/1/client/crash_report_database... client/crash_report_database.h:146: //! \brief Returns the Settings object for this database. Ownership? https://codereview.chromium.org/988063003/diff/1/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode31 client/settings.h:31: class Settings { I thought this might be a nested class inside CrashReportDatabase, but that’s not a strong preference. https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode40 client/settings.h:40: virtual bool GetClientID(std::string* client_id) = 0; Shouldn’t this pass a UUID* back? https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode48 client/settings.h:48: //! error logged. As implemented for Mac, these can return false with no error logged if the key was missing from the dictionary. I do think that this is a mistake, though, I think key-missing should be handled by the settings implementation. See comments in the implementation file. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm File client/settings_mac.mm (right): https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:47: O_CREAT|O_EXCL|O_EXLOCK|O_WRONLY, Spaces around the |s, everywhere you use bitwise OR. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:70: bool SettingsMac::GetClientID(std::string* client_id) { These are going to be called from weird places that don’t have run loops. All of the public entry points that use Objective-C should be wrapped in @autoreleasepool. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:72: if (!value) If the plist couldn’t be read at any point, we should reinitialize it with a fresh client ID. We shouldn’t let a corrupt file ruin the database forever. Log a message and recover. If the plist could be read but is missing the client ID key or the value for it is not of the expected type, we should set a new client ID and get on with life. I also think that if what we read back isn’t formatted validly for a UUID, we should trash it and set a new one. Recovery operations should relock the file with a write lock, retry the read to make sure nobody else already fixed the file, and then do the recovery. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:75: if (![value isKindOfClass:[NSString class]]) { Why not ObjCCast? https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:95: return true; With this implementation, a fresh database will return false for GetUploadsEnabled() until someone calls SetUploadsEnabled(). I think it’d be more useful to just provide a default false value when the database is created (or if the key is of the wrong type, etc). The caller can’t know whether a new settings file was created and thus that it needs to push in its preferred default value, because this class is the only thing that knows that a new settings file was created. That leaves callers in the awkward spot of treating a failure return of this method as “just use your preferred default” but there’s no way to disambiguate it from a genuine error, and it makes the caller’s code more complex. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:150: NSPropertyListFormat format = NSPropertyListXMLFormat_v1_0; Not the faster binary format? https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:182: base::scoped_nsobject<id> value; You don’t need to declare this ’til much later, if even at all. You could just make the error case returns use base::scoped_nsobject<id>(), and the successful return use base::scoped_nsobject<id>([[plist objectForKey:key] retain]). https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:203: LOG(ERROR) << "open for set setting"; PLOG https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc#newc... client/settings_test.cc:23: Inner unnamed namespace, too, like all of the other tests. https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc#newc... client/settings_test.cc:42: private: DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/988063003/diff/1/client/crash_report_database.h File client/crash_report_database.h (right): https://codereview.chromium.org/988063003/diff/1/client/crash_report_database... client/crash_report_database.h:146: //! \brief Returns the Settings object for this database. On 2015/03/08 05:21:56, Mark Mentovai wrote: > Ownership? Done. https://codereview.chromium.org/988063003/diff/1/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode31 client/settings.h:31: class Settings { On 2015/03/08 05:21:56, Mark Mentovai wrote: > I thought this might be a nested class inside CrashReportDatabase, but that’s > not a strong preference. The Database interface is large enough as it is. A separate file seemed warranted. https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode40 client/settings.h:40: virtual bool GetClientID(std::string* client_id) = 0; On 2015/03/08 05:21:56, Mark Mentovai wrote: > Shouldn’t this pass a UUID* back? I think the server is going to deal only in strings, so I think it makes sense to store it as a string. https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode48 client/settings.h:48: //! error logged. On 2015/03/08 05:21:56, Mark Mentovai wrote: > As implemented for Mac, these can return false with no error logged if the key > was missing from the dictionary. I do think that this is a mistake, though, I > think key-missing should be handled by the settings implementation. See comments > in the implementation file. Done. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm File client/settings_mac.mm (right): https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:47: O_CREAT|O_EXCL|O_EXLOCK|O_WRONLY, On 2015/03/08 05:21:56, Mark Mentovai wrote: > Spaces around the |s, everywhere you use bitwise OR. Done. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:70: bool SettingsMac::GetClientID(std::string* client_id) { On 2015/03/08 05:21:56, Mark Mentovai wrote: > These are going to be called from weird places that don’t have run loops. All of > the public entry points that use Objective-C should be wrapped in > @autoreleasepool. Done. I didn't know the context of when these would be called, so I put it on the caller via the class comment. Removed and just provide a pool in each public method. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:72: if (!value) On 2015/03/08 05:21:56, Mark Mentovai wrote: > If the plist couldn’t be read at any point, we should reinitialize it with a > fresh client ID. We shouldn’t let a corrupt file ruin the database forever. Log > a message and recover. Done. > If the plist could be read but is missing the client ID key or the value for it > is not of the expected type, we should set a new client ID and get on with life. Done. > I also think that if what we read back isn’t formatted validly for a UUID, we > should trash it and set a new one. Do you feel strongly? I don't think this is necessary to impose, and it would also prohibit migrating the old client IDs if we wanted to do that. > Recovery operations should relock the file with a write lock, retry the read to > make sure nobody else already fixed the file, and then do the recovery. Done. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:75: if (![value isKindOfClass:[NSString class]]) { On 2015/03/08 05:21:56, Mark Mentovai wrote: > Why not ObjCCast? I didn't see any advantage to using that because I still want an error logged and the type is |id| so I'm not going to benefit from the result of the cast. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:95: return true; On 2015/03/08 05:21:56, Mark Mentovai wrote: > With this implementation, a fresh database will return false for > GetUploadsEnabled() until someone calls SetUploadsEnabled(). I think it’d be > more useful to just provide a default false value when the database is created > (or if the key is of the wrong type, etc). The caller can’t know whether a new > settings file was created and thus that it needs to push in its preferred > default value, because this class is the only thing that knows that a new > settings file was created. That leaves callers in the awkward spot of treating a > failure return of this method as “just use your preferred default” but there’s > no way to disambiguate it from a genuine error, and it makes the caller’s code > more complex. Done. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:150: NSPropertyListFormat format = NSPropertyListXMLFormat_v1_0; On 2015/03/08 05:21:56, Mark Mentovai wrote: > Not the faster binary format? Done. The Foundation guides don't actually provide guidance or a "do the sensible thing" value. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:182: base::scoped_nsobject<id> value; On 2015/03/08 05:21:56, Mark Mentovai wrote: > You don’t need to declare this ’til much later, if even at all. You could just > make the error case returns use base::scoped_nsobject<id>(), and the successful > return use base::scoped_nsobject<id>([[plist objectForKey:key] retain]). I dislike having to read and write such a verbose early return value when what I really want to say is `nil` but the scoper can't convert that like it can nullptr_t. Now that all callers have a NSAutoreleasePool in place, I'm just going to let these be returned autoreleased. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:203: LOG(ERROR) << "open for set setting"; On 2015/03/08 05:21:56, Mark Mentovai wrote: > PLOG Done. https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc#newc... client/settings_test.cc:23: On 2015/03/08 05:21:56, Mark Mentovai wrote: > Inner unnamed namespace, too, like all of the other tests. Done. https://codereview.chromium.org/988063003/diff/1/client/settings_test.cc#newc... client/settings_test.cc:42: private: On 2015/03/08 05:21:56, Mark Mentovai wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/988063003/diff/1/client/crash_report_database... File client/crash_report_database_mac.mm (right): https://codereview.chromium.org/988063003/diff/1/client/crash_report_database... client/crash_report_database_mac.mm:527: NSArray* paths = [[NSFileManager defaultManager] This talk of the autorelease pool in Settings prompted me to check the database too: An autorelease pool is needed here (a separate change for that would be OK). https://codereview.chromium.org/988063003/diff/1/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/1/client/settings.h#newcode40 client/settings.h:40: virtual bool GetClientID(std::string* client_id) = 0; Robert Sesek wrote: > On 2015/03/08 05:21:56, Mark Mentovai wrote: > > Shouldn’t this pass a UUID* back? > > I think the server is going to deal only in strings, so I think it makes sense > to store it as a string. You could make the same argument about report IDs in the database, and this is related pretty closely to the database so the inconsistency seems a little weird. https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm File client/settings_mac.mm (right): https://codereview.chromium.org/988063003/diff/1/client/settings_mac.mm#newco... client/settings_mac.mm:72: if (!value) Robert Sesek wrote: > Do you feel strongly? I don't think this is necessary to impose, and it would > also prohibit migrating the old client IDs if we wanted to do that. I discussed this with Anthony last week. We don’t need to do a migration. https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.h File client/settings_mac.h (right): https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.h#ne... client/settings_mac.h:56: id GetSetting(NSString* key, Class value_class); This can be templatized so that it returns the correct type instead of id. https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm File client/settings_mac.mm (right): https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm#n... client/settings_mac.mm:60: // The file wasn't opened successfully, try opening it without creating. “Wasn’t opened successfully” glosses over the fact that this was an O_CREAT | O_EXCL. How about “wasn’t created?” https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm#n... client/settings_mac.mm:71: return false; If the plist is good but doesn’t contain this key or doesn’t contain it as the right type, this shouldn’t fail, it should generate a new client ID. (If the plist was otherwise good, it should also preserve the rest of its contents.) A messed-up plist that’s missing this key but is otherwise validly formatted shouldn’t break crash reporting. This is still true even if you’re not doing format validation of the identifier as a UUID. https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm#n... client/settings_mac.mm:119: base::scoped_nsobject<NSFileHandle> file_handle( Rewind the fd to the beginning of the file first. When recovery happens, you’ll get a WritePlist() followed by a ReadPlist(), so it’ll be at EOF for the ReadPlist(). https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm#n... client/settings_mac.mm:210: fd.reset(); // Recovery will reopen the file. Kinda unnecessary to do the close-reopen-reread part here, since you already have the exclusive lock. Recovery would just be reinitialization. But if you are keeping this as-is with the close-reopen-reread, note that it’s identical to the block in GetSetting(), so this can be refactored so that the two share. You can have ReadOrRecoverPlist() do this by calling ReadPlist() and RecoverPlist(), or you can just rename RecoverPlist() and move it to there. (You could also do that and add a bool argument indicating what type of lock is held, and have it skip the close-reopen-reread if it’s exclusive.) https://codereview.chromium.org/988063003/diff/20001/client/settings_mac.mm#n... client/settings_mac.mm:234: LOG(INFO) << "Attempting to recover settings file " << file_path(); I don’t know if we need the LOG(INFO)s. Maybe just the “recovery complete” one to let you know everything’s OK. For the others, there’s no real error to report handling of, and some other process or thread would have dealt with the problem, so the log messages are just noise.
Patchset #3 (id:40001) has been deleted
Per offline discussion, switched from using a plist to a binary struct.
Probably for the better this way compared to the plist. https://codereview.chromium.org/988063003/diff/60001/client/client.gyp File client/client.gyp (right): https://codereview.chromium.org/988063003/diff/60001/client/client.gyp#newcode55 client/client.gyp:55: # Port to Win: https://code.google.com/p/crashpad/issues/detail?id=13 80. Same on line 86. https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... File client/crash_report_database_mac.mm (right): https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... client/crash_report_database_mac.mm:207: bool CrashReportDatabaseMac::Initialize() { An InitializationStateDcheck could help here. That’s OK for a distinct change. https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... client/crash_report_database_win.cc:572: return nullptr; Put a comment referencing bug 13 and call NOTREACHED() before returning. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:28: const uint16_t kSettingsVersion = 1; Why not make this a static const inside Settings::Data? https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:38: uint16_t version; Revise the struct order for better packing, to ensure that the 32-bit and 64-bit layouts are identical, and to ensure that unused parts of the struct are always initialized and don’t contain memory garbage. I would suggest: uint32_t version; uint32_t options; // uploads-enabled are a bit in here uint64_t last_upload_attempt_time; UUID client_id; Alternative 1: Keep bool uploads_enabled, but stick it in a union with options and initialize options. Alternative 2: Use a bit field that gives you uploads_enabled : 1 and reserved : 31. Otherwise, if you just have the bool by itself, you’ll need to memset the struct to fill the inaccessible bytes with zeroes in the constructor. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:41: time_t last_upload_attempt_time; time_t has a variable size in 32-bit and 64-bit environments. This should explicitly be uint64_t. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:51: bool Settings::Initialize() { InitializationStateDcheck here and in public entry points to guard against API misuse? https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:63: // The file wasn't created, try opening it for a write operation. Since you only do a read in most cases, say in the comment that you’re opening it for a write so that you have the right handle for RecoverSettings() if that becomes necessary. Also, trying to open it for a write has a nice property that if the file is unwritable (permission problem), Initialize() will fail. Whatever your intent was, say so in the comment, because the happy path appears to only actually require the handle be open for reading. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:125: ScopedFileHandle handle = OpenForWriting(); The bulk of this function is identical to SetUploadsEnabled() (and, for that matter, similar to OpenAndReadSettings()). You should refactor into a common “ScopedFileHandle OpenForWritingAndReadSettings(Data* out_data);” function. Perhaps better, modify OpenAndReadSettings() to have a bool argument controlling whether to open for reading or writing. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:147: ScopedFileHandle handle(HANDLE_EINTR(open(file_path(), O_RDWR | O_EXLOCK))); Add O_CREAT so that we can recover when the file gets removed accidentally just like all of the other things that we can now recover from. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:199: scoped_handle.reset(OpenForWriting().release()); If you open the handle yourself here, you need to try ReadSettings(), and if it succeeds, just return true. The exclusive write lock that you acquire here guarantees that nobody else can touch the file while you hold it, but someone else could have fixed the settings in between the time that this method’s caller decided that recovery was required and the time that you acquire the exclusive lock to actually do the recovery. You wouldn’t want to do recovery twice in that case. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:208: if (!InitializeSettings(handle)) You can make InitializeSettings() accept a Data* out-parameter to avoid having to call ReadSettings() immediately after WriteSettings(). https://codereview.chromium.org/988063003/diff/60001/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcode32 client/settings.h:32: //! This class cannot be instantiated directly, but rather an instance of it “cannot” makes it sound like it’s physically impossible, but that’s not the case (the constructor is public). Instead, say “must not”. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcode42 client/settings.h:42: //! to locate all crash reports from a specific Crashpad database. used on a server https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:102: ScopedFileHandle OpenForWriting(); OpenForReadingAndWriting(), and adjust the comment accordingly. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:106: // fails, returns false. Add the word “also” before the final “fails.” Also say that it logs more stuff if recovery fails. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:115: bool WriteSettings(FileHandle handle, const Data* data); const Data& instead of const Data* for the const in-parameter that is required. Looks like that works well at all call sites. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:119: // result of OpenForWriting(). The new settings data are stored in |out_data|. Also say that the caller must not be holding a handle if it passes in an invalid handle for this argument. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:36: void InitializeBadFile() { Also have something that removes settings_path() to test that set/get can recover from that case too. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:78: EXPECT_TRUE(settings()->SetUploadsEnabled(true)); Make sure that this sticks if you have another Settings object, like you did in the ClientID test. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:93: time_t expected = time(nullptr); Can be const. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:94: EXPECT_TRUE(settings()->SetLastUploadAttemptTime(expected)); Same. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:113: EXPECT_TRUE(settings()->GetClientID(&client_id)); Make sure that this stored the client ID and that it’s stable if you have a new Settings object that wasn’t involved in the recovery at all.
https://codereview.chromium.org/988063003/diff/60001/client/client.gyp File client/client.gyp (right): https://codereview.chromium.org/988063003/diff/60001/client/client.gyp#newcode55 client/client.gyp:55: # Port to Win: https://code.google.com/p/crashpad/issues/detail?id=13 On 2015/03/09 19:12:35, Mark Mentovai wrote: > 80. Same on line 86. Done. https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/crash_report_data... client/crash_report_database_win.cc:572: return nullptr; On 2015/03/09 19:12:35, Mark Mentovai wrote: > Put a comment referencing bug 13 and call NOTREACHED() before returning. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:28: const uint16_t kSettingsVersion = 1; On 2015/03/09 19:12:36, Mark Mentovai wrote: > Why not make this a static const inside Settings::Data? Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:38: uint16_t version; On 2015/03/09 19:12:36, Mark Mentovai wrote: > Revise the struct order for better packing, to ensure that the 32-bit and 64-bit > layouts are identical, and to ensure that unused parts of the struct are always > initialized and don’t contain memory garbage. I would suggest: > > uint32_t version; > uint32_t options; // uploads-enabled are a bit in here > uint64_t last_upload_attempt_time; > UUID client_id; > > Alternative 1: Keep bool uploads_enabled, but stick it in a union with options > and initialize options. > > Alternative 2: Use a bit field that gives you uploads_enabled : 1 and reserved : > 31. > > Otherwise, if you just have the bool by itself, you’ll need to memset the struct > to fill the inaccessible bytes with zeroes in the constructor. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:41: time_t last_upload_attempt_time; On 2015/03/09 19:12:35, Mark Mentovai wrote: > time_t has a variable size in 32-bit and 64-bit environments. This should > explicitly be uint64_t. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:51: bool Settings::Initialize() { On 2015/03/09 19:12:35, Mark Mentovai wrote: > InitializationStateDcheck here and in public entry points to guard against API > misuse? I think that clutters up Initialize more than it helps. Considering that construction is not allowed beyond that of the database, API misuse should not happen. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:63: // The file wasn't created, try opening it for a write operation. On 2015/03/09 19:12:36, Mark Mentovai wrote: > Since you only do a read in most cases, say in the comment that you’re opening > it for a write so that you have the right handle for RecoverSettings() if that > becomes necessary. Also, trying to open it for a write has a nice property that > if the file is unwritable (permission problem), Initialize() will fail. Whatever > your intent was, say so in the comment, because the happy path appears to only > actually require the handle be open for reading. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:125: ScopedFileHandle handle = OpenForWriting(); On 2015/03/09 19:12:35, Mark Mentovai wrote: > The bulk of this function is identical to SetUploadsEnabled() (and, for that > matter, similar to OpenAndReadSettings()). You should refactor into a common > “ScopedFileHandle OpenForWritingAndReadSettings(Data* out_data);” function. > Perhaps better, modify OpenAndReadSettings() to have a bool argument controlling > whether to open for reading or writing. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:147: ScopedFileHandle handle(HANDLE_EINTR(open(file_path(), O_RDWR | O_EXLOCK))); On 2015/03/09 19:12:36, Mark Mentovai wrote: > Add O_CREAT so that we can recover when the file gets removed accidentally just > like all of the other things that we can now recover from. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:199: scoped_handle.reset(OpenForWriting().release()); On 2015/03/09 19:12:35, Mark Mentovai wrote: > If you open the handle yourself here, you need to try ReadSettings(), and if it > succeeds, just return true. The exclusive write lock that you acquire here > guarantees that nobody else can touch the file while you hold it, but someone > else could have fixed the settings in between the time that this method’s caller > decided that recovery was required and the time that you acquire the exclusive > lock to actually do the recovery. You wouldn’t want to do recovery twice in that > case. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:208: if (!InitializeSettings(handle)) On 2015/03/09 19:12:35, Mark Mentovai wrote: > You can make InitializeSettings() accept a Data* out-parameter to avoid having > to call ReadSettings() immediately after WriteSettings(). It would be unnecessary to do that for the other caller. https://codereview.chromium.org/988063003/diff/60001/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcode32 client/settings.h:32: //! This class cannot be instantiated directly, but rather an instance of it On 2015/03/09 19:12:36, Mark Mentovai wrote: > “cannot” makes it sound like it’s physically impossible, but that’s not the case > (the constructor is public). Instead, say “must not”. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcode42 client/settings.h:42: //! to locate all crash reports from a specific Crashpad database. On 2015/03/09 19:12:36, Mark Mentovai wrote: > used on a server Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:102: ScopedFileHandle OpenForWriting(); On 2015/03/09 19:12:36, Mark Mentovai wrote: > OpenForReadingAndWriting(), and adjust the comment accordingly. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:106: // fails, returns false. On 2015/03/09 19:12:36, Mark Mentovai wrote: > Add the word “also” before the final “fails.” Also say that it logs more stuff > if recovery fails. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:115: bool WriteSettings(FileHandle handle, const Data* data); On 2015/03/09 19:12:36, Mark Mentovai wrote: > const Data& instead of const Data* for the const in-parameter that is required. > Looks like that works well at all call sites. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings.h#newcod... client/settings.h:119: // result of OpenForWriting(). The new settings data are stored in |out_data|. On 2015/03/09 19:12:36, Mark Mentovai wrote: > Also say that the caller must not be holding a handle if it passes in an invalid > handle for this argument. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:36: void InitializeBadFile() { On 2015/03/09 19:12:36, Mark Mentovai wrote: > Also have something that removes settings_path() to test that set/get can > recover from that case too. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:78: EXPECT_TRUE(settings()->SetUploadsEnabled(true)); On 2015/03/09 19:12:36, Mark Mentovai wrote: > Make sure that this sticks if you have another Settings object, like you did in > the ClientID test. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:93: time_t expected = time(nullptr); On 2015/03/09 19:12:36, Mark Mentovai wrote: > Can be const. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:94: EXPECT_TRUE(settings()->SetLastUploadAttemptTime(expected)); On 2015/03/09 19:12:36, Mark Mentovai wrote: > Same. Done. https://codereview.chromium.org/988063003/diff/60001/client/settings_test.cc#... client/settings_test.cc:113: EXPECT_TRUE(settings()->GetClientID(&client_id)); On 2015/03/09 19:12:36, Mark Mentovai wrote: > Make sure that this stored the client ID and that it’s stable if you have a new > Settings object that wasn’t involved in the recovery at all. Done.
This looks really robust now. These comments are all minor. https://codereview.chromium.org/988063003/diff/60001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:51: bool Settings::Initialize() { Robert Sesek wrote: > On 2015/03/09 19:12:35, Mark Mentovai wrote: > > InitializationStateDcheck here and in public entry points to guard against API > > misuse? > > I think that clutters up Initialize more than it helps. Considering that > construction is not allowed beyond that of the database, API misuse should not > happen. Well, it’s optional, but doing it doesn’t just guard that the public methods are called correctly. It also guards against accidental double-Initialize() and provides a pretty unambiguous signal of use-after-free problems when the memory for the object hasn’t yet been reclaimed. https://codereview.chromium.org/988063003/diff/80001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:26: struct ALIGNAS(4) Settings::Data { #include "base/compiler_specific.h" https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:30: options(), Explicit 0 for uint32_t. https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:35: kUploadsEnabled = 1 << 1, 1 << 0 https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:40: uint64_t last_upload_attempt_time; // time_t https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:65: // file has write permissions. process has permission to write the file. https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:120: *time = settings.last_upload_attempt_time; saturated_cast<time_t>()… Perhaps with a LOG(WARNING) if !base::IsValueInRangeForNumericType<time_t>() Actually, InRangeCast<time_t>(…, std::numeric_limits<time_t>::max()) will do both of these for you (going from unsigned to signed, saturation would only happen on the positive end). https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:130: settings.last_upload_attempt_time = time; satured_cast<uint64_t>(), again perhaps with a warning. Or InRangeCast<uint64_t>(…, 0) (going from signed to unsigned, saturation would only happen on the negative end provided that time_t is an integral type no wider than uint64_t). https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:204: LOG(INFO) << "Recovering settings file " << file_path(); Move this to after the next conditional, since this doesn’t actually recover anything if it finds that it’s already been recovered once it has the exclusive write lock. It falsely implies that this function, if successful, generated a new client ID. https://codereview.chromium.org/988063003/diff/80001/client/settings.cc#newco... client/settings.cc:211: // Test if the file has already been recovered. The critical thing here is that the test happens under the exclusive write lock. The comment should point this out. https://codereview.chromium.org/988063003/diff/80001/client/settings.h File client/settings.h (right): https://codereview.chromium.org/988063003/diff/80001/client/settings.h#newcod... client/settings.h:135: const char* file_path() { return file_path_.value().c_str(); } This method can be const. https://codereview.chromium.org/988063003/diff/80001/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/80001/client/settings_test.cc#... client/settings_test.cc:91: } Recheck that local_settings.GetUploadsEnabled() returns the same value just set on settings(). Ensures that the change was actually written somewhere that both can see, rather than Settings() just reading the data on Initialize() and then not consulting the disk again. https://codereview.chromium.org/988063003/diff/80001/client/settings_test.cc#... client/settings_test.cc:154: EXPECT_NE(client_id, new_client_id); Check that the other fields are at their expected defaults too (especially if you previously changed them to non-default).
PTAL https://codereview.chromium.org/988063003/diff/60001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:51: bool Settings::Initialize() { On 2015/03/09 22:00:57, Mark Mentovai wrote: > Robert Sesek wrote: > > On 2015/03/09 19:12:35, Mark Mentovai wrote: > > > InitializationStateDcheck here and in public entry points to guard against > API > > > misuse? > > > > I think that clutters up Initialize more than it helps. Considering that > > construction is not allowed beyond that of the database, API misuse should not > > happen. > > Well, it’s optional, but doing it doesn’t just guard that the public methods are > called correctly. It also guards against accidental double-Initialize() and > provides a pretty unambiguous signal of use-after-free problems when the memory > for the object hasn’t yet been reclaimed. Sure, but it means not being able to return the result of the other calls directly and results in code like: bool rv = InitializeSettings(handle.get()); if (rv) { INITIALIZATION_STATE_DCHECK_SET_VALID(initialized_); } else { INITIALIZATINO_STATE_DCHECK_SET_INVALID(initialized_); } return rv;
LGTM but the discussion continues… https://codereview.chromium.org/988063003/diff/60001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/60001/client/settings.cc#newco... client/settings.cc:51: bool Settings::Initialize() { Robert Sesek wrote: > Sure, but it means not being able to return the result of the other calls > directly and results in code like: > > bool rv = InitializeSettings(handle.get()); > if (rv) { > INITIALIZATION_STATE_DCHECK_SET_VALID(initialized_); > } else { > INITIALIZATINO_STATE_DCHECK_SET_INVALID(initialized_); > } > return rv; You don’t need to set invalid on failure, just leave it set to initializing. And, as you pointed out, nothing currently cares if someone forgot to call Initialize(), although this will produce log spam. An InitializationStateDcheck will actually guard that the interface is used as expected, in a non-log-spam-producing way.
LGTM https://codereview.chromium.org/988063003/diff/140001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/140001/client/settings.cc#newc... client/settings.cc:49: : file_path_(file_path) { , initialized_() https://codereview.chromium.org/988063003/diff/140001/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/140001/client/settings_test.cc... client/settings_test.cc:67: EXPECT_TRUE(local_settings.Initialize()); Oh, so it actually caught an API abuse, did it? Hmm, interesting…
https://codereview.chromium.org/988063003/diff/140001/client/settings.cc File client/settings.cc (right): https://codereview.chromium.org/988063003/diff/140001/client/settings.cc#newc... client/settings.cc:49: : file_path_(file_path) { On 2015/03/09 22:41:34, Mark Mentovai wrote: > , initialized_() Done. https://codereview.chromium.org/988063003/diff/140001/client/settings_test.cc File client/settings_test.cc (right): https://codereview.chromium.org/988063003/diff/140001/client/settings_test.cc... client/settings_test.cc:67: EXPECT_TRUE(local_settings.Initialize()); On 2015/03/09 22:41:34, Mark Mentovai wrote: > Oh, so it actually caught an API abuse, did it? Hmm, interesting… Is it? 🙉
LGTM
Message was sent while issue was closed.
Committed patchset #8 (id:160001) manually as 1a635e3a799c9c93b39d1a9f507b3c9eebbd873b (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
