Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824623002:
Last_n: Delete previously saved snapshot when navigating. (Closed)
|
Created:
3 years, 8 months ago by carlosk Modified:
3 years, 8 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, Dmitry Titov Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLast_n: Delete previously saved snapshot when navigating.
After the switch that made last_n use tab hidden events to trigger the creation
of snapshots a privacy bug was introduced. Saved pages were deleted along with
the creation of a new snapshot, what happened only on the next hide event. If
that never happened before the device would go offline, navigating back would
eventually present the last snapshotted page, potentially from many history
steps ago.
This change fixes this bug by storing information about the last successful
last_n snapshot (just like it's already done for downloads requests) and using
the offline_id of the previous saved page to delete it upon new navigations.
Unit tests were updated to comply with this behavior change and new integration
tests were added to enforce the privacy requirements IRT snapshot deletion.
BUG=711770
Review-Url: https://codereview.chromium.org/2824623002
Cr-Commit-Position: refs/heads/master@{#465459}
Committed: https://chromium.googlesource.com/chromium/src/+/0540bc797b12c1ee7d6f648b1502a8a61dc30b8c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Stop last_n for POST navigations; merge instrumentation tests from another CL. #
Total comments: 7
Patch Set 3 : Annotated method with @MainThread; comment updates #
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
carlosk@chromium.org changed reviewers: + dewittj@chromium.org
dewittj@: PTAL. dimich@: FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2824623002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2824623002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:216: if (last_n_latest_saved_snapshot_info_) { This is an interesting strategy, I wonder if there are any race conditions (with Chrome exiting for example) that might prevent us from deleting the page. Also, why have a whole new snapshot info rather than just annotating the existing snapshot info as complete?
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Last_n: Delete previously saved snapshot when navigating. After the switch that made last_n use tab hidden events to trigger the creation of snapshots a privacy bug was introduced. Saved pages were deleted along with the creation of a new snapshot, what happened only on the next hide event. If that never happened before the device would go offline, navigating back would eventually present the last snapshotted page, potentially from many history steps ago. This change fixes this bug by storing information about the last successful last_n snapshot (just like it's already done for downloads requests) and using the offline_id of the previous saved page to delete it upon new navigations. Tests were also updated to comply with this behavior change. BUG=711770 ========== to ========== Last_n: Delete previously saved snapshot when navigating. After the switch that made last_n use tab hidden events to trigger the creation of snapshots a privacy bug was introduced. Saved pages were deleted along with the creation of a new snapshot, what happened only on the next hide event. If that never happened before the device would go offline, navigating back would eventually present the last snapshotted page, potentially from many history steps ago. This change fixes this bug by storing information about the last successful last_n snapshot (just like it's already done for downloads requests) and using the offline_id of the previous saved page to delete it upon new navigations. Unit tests were updated to comply with this behavior change and new integration tests were added to enforce the privacy requirements IRT snapshot deletion. BUG=711770 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/17 21:17:42, dewittj wrote:
> (last_n_latest_saved_snapshot_info_) {
> This is an interesting strategy, I wonder if there are any race conditions
(with
> Chrome exiting for example) that might prevent us from deleting the page.
We talked offline and agreed that there is a race here but it is unlikely so OK
to leave for a future change.
> Also, why have a whole new snapshot info rather than just annotating the
> existing snapshot info as complete?
^^^
I just hadn't hit send yet. :) https://codereview.chromium.org/2824623002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2824623002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:216: if (last_n_latest_saved_snapshot_info_) { On 2017/04/17 21:17:42, dewittj wrote: > This is an interesting strategy, I wonder if there are any race conditions (with > Chrome exiting for example) that might prevent us from deleting the page. There is a very small window of time between the page committing and this observer method being called when if Chrome should be terminated within it then it would cause the snapshot to be left alive and potentially accessible. But upon future restarts the snapshot will eventually be deleted either when a new snapshot should be saved for that tab or when the tab should be closed. > Also, why have a whole new snapshot info rather than just annotating the > existing snapshot info as complete? This mimics the way we already keep the last successful snapshot info for downloads so it makes it consistent with how things already work inside this file. I am not creating a new instance but transferring from one smart pointer to another. I can't keep the "ongoing" pointer after snapshot completion because its existence is used as a flag for ongoing process of snapshot creation. https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:187: * requirement for last_n. I updated the privacy related comments here and below. Let me know if you think they are not yet clear. https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:282: private class OfflinePageDeletionMonitor { I switched to using this deletion monitor class that is more precise IRT where it is installed than the previous single method call that proved to be flaky.
lgtm https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:187: * requirement for last_n. On 2017/04/18 17:56:19, carlosk wrote: > I updated the privacy related comments here and below. Let me know if you think > they are not yet clear. "current" is not defined :) https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:282: private class OfflinePageDeletionMonitor { On 2017/04/18 17:56:19, carlosk wrote: > I switched to using this deletion monitor class that is more precise IRT where > it is installed than the previous single method call that proved to be flaky. Acknowledged. https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:290: public void installUIThreadObserver() { nit: annotate with @MainThread, rename to |installObserver|
Thanks. https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:187: * requirement for last_n. On 2017/04/18 18:09:16, dewittj wrote: > On 2017/04/18 17:56:19, carlosk wrote: > > I updated the privacy related comments here and below. Let me know if you > think > > they are not yet clear. > > "current" is not defined :) Rephrased yet one more time. I can't find a form that makes me really happy though. :) https://codereview.chromium.org/2824623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:290: public void installUIThreadObserver() { On 2017/04/18 18:09:16, dewittj wrote: > nit: annotate with @MainThread, rename to |installObserver| Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2824623002/#ps40001 (title: "Annotated method with @MainThread; comment updates")
The CQ bit was unchecked by carlosk@chromium.org
carlosk@chromium.org changed reviewers: + agrieve@chromium.org
agrieve@: PTAL at BUILD.gn.
On 2017/04/18 22:15:27, carlosk wrote: > agrieve@: PTAL at BUILD.gn. BUILD lgtm
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492561396230800,
"parent_rev": "4257abd6971db0275528a5db276806a24738bfda", "commit_rev":
"0540bc797b12c1ee7d6f648b1502a8a61dc30b8c"}
Message was sent while issue was closed.
Description was changed from ========== Last_n: Delete previously saved snapshot when navigating. After the switch that made last_n use tab hidden events to trigger the creation of snapshots a privacy bug was introduced. Saved pages were deleted along with the creation of a new snapshot, what happened only on the next hide event. If that never happened before the device would go offline, navigating back would eventually present the last snapshotted page, potentially from many history steps ago. This change fixes this bug by storing information about the last successful last_n snapshot (just like it's already done for downloads requests) and using the offline_id of the previous saved page to delete it upon new navigations. Unit tests were updated to comply with this behavior change and new integration tests were added to enforce the privacy requirements IRT snapshot deletion. BUG=711770 ========== to ========== Last_n: Delete previously saved snapshot when navigating. After the switch that made last_n use tab hidden events to trigger the creation of snapshots a privacy bug was introduced. Saved pages were deleted along with the creation of a new snapshot, what happened only on the next hide event. If that never happened before the device would go offline, navigating back would eventually present the last snapshotted page, potentially from many history steps ago. This change fixes this bug by storing information about the last successful last_n snapshot (just like it's already done for downloads requests) and using the offline_id of the previous saved page to delete it upon new navigations. Unit tests were updated to comply with this behavior change and new integration tests were added to enforce the privacy requirements IRT snapshot deletion. BUG=711770 Review-Url: https://codereview.chromium.org/2824623002 Cr-Commit-Position: refs/heads/master@{#465459} Committed: https://chromium.googlesource.com/chromium/src/+/0540bc797b12c1ee7d6f648b1502... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0540bc797b12c1ee7d6f648b1502...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2825973002/ by aboxhall@chromium.org. The reason for reverting is: This test has been crashing: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... .
Message was sent while issue was closed.
On 2017/04/19 05:40:01, aboxhall wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2825973002/ by mailto:aboxhall@chromium.org. > > The reason for reverting is: This test has been crashing: > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > . Findit's analysis suggest this the newly-added test is flaky according to https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Can someone please help confirm? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/04/19 18:36:08, lijeffrey wrote: > On 2017/04/19 05:40:01, aboxhall wrote: > > A revert of this CL (patchset #3 id:40001) has been created in > > https://codereview.chromium.org/2825973002/ by mailto:aboxhall@chromium.org. > > > > The reason for reverting is: This test has been crashing: > > > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > . > > Findit's analysis suggest this the newly-added test is flaky according to > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > Can someone please help confirm? > > Thanks, > Jeff on behalf of Findit team I being to think my tests are not the actual culprit of this flakyness. The logs shown what seems to me to be an unrelated DCHECK causing the crashes that I can't repro locally. There is an open issue on it: https://crbug.com/592961 (restricted view). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
