Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1708763003:
Fix strict mode violation in TabPersistentStore.cleanupPersistentData() (Closed)
DescriptionFix strict mode violation in TabPersistentStore.cleanupPersistentData()
Moved File.list() calls to AsyncTask.
BUG=586233
Committed: https://crrev.com/0b3847db495092eaeedf4b37b73f7784a3c12414
Cr-Commit-Position: refs/heads/master@{#382338}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix test, address commetns #
Total comments: 16
Patch Set 3 : remove AtOrAboveId #
Total comments: 7
Patch Set 4 : comments & rebase #Depends on Patchset: Messages
Total messages: 32 (10 generated)
agrieve@chromium.org changed reviewers: + dfalcantara@chromium.org, wnwen@chromium.org
ðŸŽðŸŽ
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708763003/1
lgtm https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (right): https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:332: final File thumbnailDirectory = PathUtils.getThumbnailCacheDirectory(mContext); Actually if you could put this into the AsyncTask that'd be great. 👌 https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:355: }.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); THREAD_POOL_EXECUTOR may be better as we will not block other tasks that need to be on SERIAL_EXECUTOR. https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:963: new CleanupPersistentDataTask(-1).executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); s/-1/minForbiddenId ditto for THREAD_POOL_EXECUTOR, since we're filtering again on the UI thread to make sure it's okay, this can happen concurrently.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (right): https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:332: final File thumbnailDirectory = PathUtils.getThumbnailCacheDirectory(mContext); On 2016/02/17 21:24:23, Peter Wen wrote: > Actually if you could put this into the AsyncTask that'd be great. 👌 Done. https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:355: }.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); On 2016/02/17 21:24:23, Peter Wen wrote: > THREAD_POOL_EXECUTOR may be better as we will not block other tasks that need to > be on SERIAL_EXECUTOR. Done. https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:963: new CleanupPersistentDataTask(-1).executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); On 2016/02/17 21:24:23, Peter Wen wrote: > s/-1/minForbiddenId > > ditto for THREAD_POOL_EXECUTOR, since we're filtering again on the UI thread to > make sure it's okay, this can happen concurrently. Done.
https://codereview.chromium.org/1708763003/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1708763003/diff/20001/base/BUILD.gn#newcode2000 base/BUILD.gn:2000: "//third_party/android_tools:android_support_v13_java", Yeah, I've tried this before. I think newt@ said that base can't depend on the support library because of WebView or something...
https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/PathUtils.java (right): https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/PathUtils.java:37: private static Object sLazyInitLock = new Object(); private static final? Can't imagine ever recreating the lock. https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:199: // TODO(agrieve): May want to dedicate a thread(pool) to IO (or join native's File thread). nit: Until it's actually a dedicated thread, maybe assertOnBackgroundThread? https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:969: mTabContentManager.cleanupPersistentDataAtAndAboveId(minForbiddenId); Can't tell if it's cleaner to pull the common null check out. It makes it slightly more readable, though. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1007: private class CleanUpPersistentDataTask extends AsyncTask<Void, Void, String[]> { Can you rename this CleanUpTabStateDataTask, instead? Spent too much time trying to understand the difference between this and the other cleanUpPersistentData() method. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1024: if (!mDestroyed && fileNames != null) { nit: early exit instead to avoid indentation? https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1029: if (TabModelUtils.getTabById(model, data.first) == null) { There's a behavioral change here, right? Before, any TabState files >= the given ID would be deleted by cleanupPersistentDataAtAndAboveId, regardless of whether there was a Tab with that ID in the given TabModel. I believe this clause was (originally) put in to prevent cases where a new tab could be created with the same ID as a previously closed tab, which could potentially result in loading the old TabState for the new tab. I don't know if this is still possible now that we're storing the maximum ID ever used since some M4X milestone (in addition to using the original backwards-compatible calculation in TabPersistentStore#readSaveStateFile), but users upgrading to M50 from an old version of Clank could hit it. Do you think the benefit of the unified codepath outweigh that possibility? If so, this change should really be documented in the commit message.
https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1029: if (TabModelUtils.getTabById(model, data.first) == null) { On 2016/02/18 17:45:35, dfalcantara wrote: > There's a behavioral change here, right? Before, any TabState files >= the > given ID would be deleted by cleanupPersistentDataAtAndAboveId, regardless of > whether there was a Tab with that ID in the given TabModel. I believe this > clause was (originally) put in to prevent cases where a new tab could be created > with the same ID as a previously closed tab, which could potentially result in > loading the old TabState for the new tab. > > I don't know if this is still possible now that we're storing the maximum ID > ever used since some M4X milestone (in addition to using the original > backwards-compatible calculation in TabPersistentStore#readSaveStateFile), but > users upgrading to M50 from an old version of Clank could hit it. > > Do you think the benefit of the unified codepath outweigh that possibility? If > so, this change should really be documented in the commit message. Yikes! I stared and stared at this code and didn't see that it didn't call getTabById() in the old version :( It seems like even in the old code though, it would have been possible for the bad case to happen if a tab got loaded before the scheduled asynchronous delete happened? Perhaps we could just delete this code path and always call cleanUp(-1)? We've already read the state file by this time, so we know the list of tabs IDs that are reachable. If we want to protect against a new tab being created with a stale tab data file existing, we should probably add a call to deleteAsync whenever a new tab id is created? wdyt?
https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1029: if (TabModelUtils.getTabById(model, data.first) == null) { On 2016/02/19 01:43:26, agrieve (away Feb 19) wrote: > On 2016/02/18 17:45:35, dfalcantara wrote: > > There's a behavioral change here, right? Before, any TabState files >= the > > given ID would be deleted by cleanupPersistentDataAtAndAboveId, regardless of > > whether there was a Tab with that ID in the given TabModel. I believe this > > clause was (originally) put in to prevent cases where a new tab could be > created > > with the same ID as a previously closed tab, which could potentially result in > > loading the old TabState for the new tab. > > > > I don't know if this is still possible now that we're storing the maximum ID > > ever used since some M4X milestone (in addition to using the original > > backwards-compatible calculation in TabPersistentStore#readSaveStateFile), but > > users upgrading to M50 from an old version of Clank could hit it. > > > > Do you think the benefit of the unified codepath outweigh that possibility? > If > > so, this change should really be documented in the commit message. > > Yikes! I stared and stared at this code and didn't see that it didn't call > getTabById() in the old version :( > > It seems like even in the old code though, it would have been possible for the > bad case to happen if a tab got loaded before the scheduled asynchronous delete > happened? > > Perhaps we could just delete this code path and always call cleanUp(-1)? > > We've already read the state file by this time, so we know the list of tabs IDs > that are reachable. If we want to protect against a new tab being created with a > stale tab data file existing, we should probably add a call to deleteAsync > whenever a new tab id is created? wdyt? I think we'd be fine just with deleting TabState files for Tabs that aren't in a TabModel. I was worried that this would be problematic for multi-instance Clank, but each TabModelSelector uses a different directory for its tabs. I'm not sure if it'd be good, performance-wise, to keep firing (even async) tasks to delete TabState files immediately after the Tab closed, though we're probably mostly safe because we don't create tabs with the same ID nowadays (keeping a global max tab ID is a relatively new thing).
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708763003/40001
agrieve@chromium.org changed reviewers: + yfriedman@chromium.org
yfriedman for base/android stamp https://codereview.chromium.org/1708763003/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1708763003/diff/20001/base/BUILD.gn#newcode2000 base/BUILD.gn:2000: "//third_party/android_tools:android_support_v13_java", On 2016/02/18 17:09:13, dfalcantara wrote: > Yeah, I've tried this before. I think newt@ said that base can't depend on the > support library because of WebView or something... Removing from this patch, but might re-visit on its own. https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/PathUtils.java (right): https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/PathUtils.java:37: private static Object sLazyInitLock = new Object(); On 2016/02/18 17:45:35, dfalcantara wrote: > private static final? Can't imagine ever recreating the lock. Deleted in favour of using sDirPathFetchTask https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/1708763003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:199: // TODO(agrieve): May want to dedicate a thread(pool) to IO (or join native's File thread). On 2016/02/18 17:45:35, dfalcantara wrote: > nit: Until it's actually a dedicated thread, maybe assertOnBackgroundThread? Reverted for now. Makes sense though. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:969: mTabContentManager.cleanupPersistentDataAtAndAboveId(minForbiddenId); On 2016/02/18 17:45:35, dfalcantara wrote: > Can't tell if it's cleaner to pull the common null check out. It makes it > slightly more readable, though. Done. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1007: private class CleanUpPersistentDataTask extends AsyncTask<Void, Void, String[]> { On 2016/02/18 17:45:35, dfalcantara wrote: > Can you rename this CleanUpTabStateDataTask, instead? Spent too much time > trying to understand the difference between this and the other > cleanUpPersistentData() method. Done. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1024: if (!mDestroyed && fileNames != null) { On 2016/02/18 17:45:35, dfalcantara wrote: > nit: early exit instead to avoid indentation? Done. https://codereview.chromium.org/1708763003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1029: if (TabModelUtils.getTabById(model, data.first) == null) { On 2016/02/19 23:54:04, dfalcantara wrote: > On 2016/02/19 01:43:26, agrieve (away Feb 19) wrote: > > On 2016/02/18 17:45:35, dfalcantara wrote: > > > There's a behavioral change here, right? Before, any TabState files >= the > > > given ID would be deleted by cleanupPersistentDataAtAndAboveId, regardless > of > > > whether there was a Tab with that ID in the given TabModel. I believe this > > > clause was (originally) put in to prevent cases where a new tab could be > > created > > > with the same ID as a previously closed tab, which could potentially result > in > > > loading the old TabState for the new tab. > > > > > > I don't know if this is still possible now that we're storing the maximum ID > > > ever used since some M4X milestone (in addition to using the original > > > backwards-compatible calculation in TabPersistentStore#readSaveStateFile), > but > > > users upgrading to M50 from an old version of Clank could hit it. > > > > > > Do you think the benefit of the unified codepath outweigh that possibility? > > If > > > so, this change should really be documented in the commit message. > > > > Yikes! I stared and stared at this code and didn't see that it didn't call > > getTabById() in the old version :( > > > > It seems like even in the old code though, it would have been possible for the > > bad case to happen if a tab got loaded before the scheduled asynchronous > delete > > happened? > > > > Perhaps we could just delete this code path and always call cleanUp(-1)? > > > > We've already read the state file by this time, so we know the list of tabs > IDs > > that are reachable. If we want to protect against a new tab being created with > a > > stale tab data file existing, we should probably add a call to deleteAsync > > whenever a new tab id is created? wdyt? > > I think we'd be fine just with deleting TabState files for Tabs that aren't in a > TabModel. I was worried that this would be problematic for multi-instance > Clank, but each TabModelSelector uses a different directory for its tabs. > > I'm not sure if it'd be good, performance-wise, to keep firing (even async) > tasks to delete TabState files immediately after the Tab closed, though we're > probably mostly safe because we don't create tabs with the same ID nowadays > (keeping a global max tab ID is a relatively new thing). Done.
dfalcantara@chromium.org changed reviewers: + dtrainor@chromium.org
Dave: can you check my comment in TabContentManager? I think you might be the closest person to know how thumbnails work. https://chromiumcodereview.appspot.com/1708763003/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (right): https://chromiumcodereview.appspot.com/1708763003/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:325: String thumbnailDirectory = PathUtils.getThumbnailCacheDirectory(mContext); Huh. More speculation here, because I didn't write this code: You can have multiple TabModelSelectorImpls: one per ChromeTabbedActivity (you can have multiple on Samsung devices, side-by-side) and one per CustomTabActivity. Are thumbnails for all of these different TabModelSelectorImpls (which have their own directories for saving TabState files) stored in the same directory? If so, I'm not sure how this code even works properly... this looks only at one TabModelSelectorImpl. I don't think your patch changes this behavior, but as a sanity check, could you check what thumbnail directories are chosen for different instances of the ChromeTabbedActivities?
Well, I suppose Dave for all multi-instance wonkiness knowledge.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm but I think Dan's right. https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (right): https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:319: public void cleanupPersistentData(final TabModelSelector modelSelector) { should this also be cleanUpPersistentData? Seems like you're moving u -> U for the other file. Might as well do it here too :)? https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:325: String thumbnailDirectory = PathUtils.getThumbnailCacheDirectory(mContext); On 2016/02/23 22:11:45, dfalcantara wrote: > Huh. More speculation here, because I didn't write this code: > > You can have multiple TabModelSelectorImpls: one per ChromeTabbedActivity (you > can have multiple on Samsung devices, side-by-side) and one per > CustomTabActivity. Are thumbnails for all of these different > TabModelSelectorImpls (which have their own directories for saving TabState > files) stored in the same directory? If so, I'm not sure how this code even > works properly... this looks only at one TabModelSelectorImpl. > > I don't think your patch changes this behavior, but as a sanity check, could you > check what thumbnail directories are chosen for different instances of the > ChromeTabbedActivities? Yeah good point! It looks like this might delete thumbnails for those other models... https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:334: if (TabModelUtils.getTabById(modelSelector.getModel(false), id) == null We could have this check all tab models... but that might not work for any that aren't actually loaded into memory at this time. Maybe we need the background task to go to disk and get lists of all saved tab state files as well?
At long last I'm back at this. I've added a comment about the bug with multiple tab state models, but I think fixing it is a separate issue. https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (right): https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:319: public void cleanupPersistentData(final TabModelSelector modelSelector) { On 2016/02/24 17:19:35, David Trainor wrote: > should this also be cleanUpPersistentData? Seems like you're moving u -> U for > the other file. Might as well do it here too :)? Done. https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:325: String thumbnailDirectory = PathUtils.getThumbnailCacheDirectory(mContext); On 2016/02/24 17:19:35, David Trainor wrote: > On 2016/02/23 22:11:45, dfalcantara wrote: > > Huh. More speculation here, because I didn't write this code: > > > > You can have multiple TabModelSelectorImpls: one per ChromeTabbedActivity (you > > can have multiple on Samsung devices, side-by-side) and one per > > CustomTabActivity. Are thumbnails for all of these different > > TabModelSelectorImpls (which have their own directories for saving TabState > > files) stored in the same directory? If so, I'm not sure how this code even > > works properly... this looks only at one TabModelSelectorImpl. > > > > I don't think your patch changes this behavior, but as a sanity check, could > you > > check what thumbnail directories are chosen for different instances of the > > ChromeTabbedActivities? > > Yeah good point! It looks like this might delete thumbnails for those other > models... Added a comment https://codereview.chromium.org/1708763003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:334: if (TabModelUtils.getTabById(modelSelector.getModel(false), id) == null On 2016/02/24 17:19:35, David Trainor wrote: > We could have this check all tab models... but that might not work for any that > aren't actually loaded into memory at this time. Maybe we need the background > task to go to disk and get lists of all saved tab state files as well? Added comment
base/android lgtm but didn't look at the other stuff
On 2016/03/18 14:55:51, Yaron wrote: > base/android lgtm but didn't look at the other stuff ...and the nice thing about the delay here is that Glenn's SELinux fix is in so this shouldn't be called by renderer any more and shouldn't have the possibility of unexpected new issues there ;)
lgtm, thanks!
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/1708763003/#ps60001 (title: "comments & rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708763003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708763003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix strict mode violation in TabPersistentStore.cleanupPersistentData() Moved File.list() calls to AsyncTask. BUG=586233 ========== to ========== Fix strict mode violation in TabPersistentStore.cleanupPersistentData() Moved File.list() calls to AsyncTask. BUG=586233 Committed: https://crrev.com/0b3847db495092eaeedf4b37b73f7784a3c12414 Cr-Commit-Position: refs/heads/master@{#382338} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b3847db495092eaeedf4b37b73f7784a3c12414 Cr-Commit-Position: refs/heads/master@{#382338} |
