Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774573002:
crashpad win: Ensure database dir exists on first run (Closed)
Descriptioncrashpad win: Ensure database dir exists on first run
Crashpad will only create one level of directory for its database
storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User
Data\Crashpad.
However, on first run when Crashpad is initialized, "User Data" will not
yet have been created. We don't want to move Crashpad initialization
later in startup, so we instead create the directory if it does not
exist.
Note that this is calling file_util::CreateDirectory(), rather than
CreateDirectoryW() which means the full parent tree will be created, not
just the last entry in the path.
R=thakis@chromium.org,wfh@chromium.org
TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run
BUG=591504
Committed: https://crrev.com/92905ef36a30be9be9041d8ee07ccd6e7ca45d64
Cr-Commit-Position: refs/heads/master@{#379676}
Patch Set 1 #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== crashpad win: Ensure database dir exists on first run R=wfh@chromium.org BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad R=wfh@chromium.org BUG=591504 ==========
scottmg@chromium.org changed reviewers: + thakis@chromium.org
Description was changed from ========== crashpad win: Ensure database dir exists on first run Crashpad R=wfh@chromium.org BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org BUG=591504 ==========
does this have test coverage through something?
not familiar with the previous code but I'm surprised the crashpad dir doesn't move if the user data dir is overridden by group policy or via --user-data-dir but I suppose this code doesn't change that expectation...
Description was changed from ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type-crashpad-handler exists on first run BUG=591504 ==========
On 2016/03/07 18:08:49, Nico wrote: > does this have test coverage through something? I don't know of any end-to-end automated installer testing, so only manual (updated TEST= line).
On 2016/03/07 18:12:03, Will Harris wrote: > not familiar with the previous code but I'm surprised the crashpad dir doesn't > move if the user data dir is overridden by group policy or via --user-data-dir > but I suppose this code doesn't change that expectation... This is https://crbug.com/565446. I'm not sure of the implications or history of why it's like that though.
Description was changed from ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type-crashpad-handler exists on first run BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run BUG=591504 ==========
lgtm
lgtm assuming that function isn't called from the UI thread (if it were, the scopedioallowed machinery should catch that), but it'd be good to have automated tests for this...
On 2016/03/07 19:02:37, Nico wrote: > lgtm assuming that function isn't called from the UI thread (if it were, the > scopedioallowed machinery should catch that), but it'd be good to have automated > tests for this... This happens a while before ThreadRestrictions is initialized. It's on the main thread, but it's not really the UI thread yet, just the WinMain() thread.
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774573002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774573002/1
Is there a way to not add disk i/o on the critical startup path?
Message was sent while issue was closed.
Description was changed from ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run BUG=591504 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run BUG=591504 ========== to ========== crashpad win: Ensure database dir exists on first run Crashpad will only create one level of directory for its database storage. On Windows, the target is %LOCALAPPDATA%\Google\Chrome\User Data\Crashpad. However, on first run when Crashpad is initialized, "User Data" will not yet have been created. We don't want to move Crashpad initialization later in startup, so we instead create the directory if it does not exist. Note that this is calling file_util::CreateDirectory(), rather than CreateDirectoryW() which means the full parent tree will be created, not just the last entry in the path. R=thakis@chromium.org,wfh@chromium.org TEST=remove local user data dir, install chrome, confirm that chrome --process-type=crashpad-handler exists on first run BUG=591504 Committed: https://crrev.com/92905ef36a30be9be9041d8ee07ccd6e7ca45d64 Cr-Commit-Position: refs/heads/master@{#379676} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/92905ef36a30be9be9041d8ee07ccd6e7ca45d64 Cr-Commit-Position: refs/heads/master@{#379676}
Message was sent while issue was closed.
On 2016/03/07 22:34:26, Nico wrote: > Is there a way to not add disk i/o on the critical startup path? ^ ping
Message was sent while issue was closed.
On 2016/03/07 22:34:26, Nico wrote: > Is there a way to not add disk i/o on the critical startup path? Sorry, I didn't see that comment. Not obviously, as we need to create the profile and user data dirs to store things in. Previously this happened to work because breakpad initialized later than crashpad does, so the dir was already made by other profile creation code. So theoretically there shouldn't be a noticeable startup regression, but famous last words, we'll see what the perf bots say. We could consider deferring Crashpad database creation until it's actually necessary (which is why it in particular wants the dir to exist). It's needed earlier than a crash occurring though, as it stores information such as the client id, whether uploading is enabled, etc. So it would add some complexity to have those API points need to block/auto-create the directory and database. I filed https://bugs.chromium.org/p/crashpad/issues/detail?id=99 to investigate that on the Crashpad side. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
