Chromium Code Reviews Chromium Code Reviews
Issue
69863006:
Address races in UserImageManagerImpl (Closed)
DescriptionAddress races in UserImageManagerImpl
UserImageManagerImpl uses background threads and delayed tasks to load
images, save images and schedule downloads. There are races in this
implementation so that an earlier operation can complete after a later
operation, clobbering its result. Delayed tasks using base::Unretained()
may cause crashes on shutdown if the unretained object no longer exists.
Some of the races were partially worked around with global state variables
that do not account for the fact that operations for multiple users may be
going on in parallel.
This CL is an almost complete rewrite of the UserImageManagerImpl that
addresses the races and thread safety issues. Pending tasks are
encapsulated by a Job class with at most one Job running per user at a
time.
Migration of old user images is simplified by this CL: There used to be a
delay between user login and migration, postponing the I/O-heavy operation
until after login is complete. However, the implementation was wrong and
migration of PNG images was actually carried out immediately. Only for
users who had selected one of the default images was the migration
delayed, which is unnecessary as it does not involve any file I/O or
transcoding. Since there have been no complaints about the performance
impact of migration during login, this CL removes the delay altogether.
The CL also updates the UserImageManagerImpl browser test, re-enabling the
previously flaky NonJPEGImageFromFile.
BUG=152957, 152959, 257009
TEST=Updated browser tests; manual tests with chromeos=1 and VM
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235645
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments addressed. #Patch Set 3 : Commit changes to UserManagerImpl that I accidentally missed. #Patch Set 4 : Fix WallpaperManager browser tests now that UserImageLoader contains a DCHECK(success). #Messages
Total messages: 15 (0 generated)
|
