New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 864592 link

Starred by 1 user
Status: Fixed
Owner:
█ harringtond@chromium.org
Last visit > 30 days ago
Closed: Sep 6
Cc:
dewittj@chromium.org
█ dim...@chromium.org
█ romax@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

All 3 stores from offline pages code should never crash due to null DB connection

Project Member Reported by carlosk@chromium.org, Jul 17 
We had a recent crash bug (private issue 860228) caused by the SQL DB Connection pointer being null due to problems initializing the DB, which was part of the store's Execute method contract. We should update this code so to change this from a convention to there being no way to not handle it properly.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02d0365c8cb04c4b32c46015f97bb1953a381b1b

commit 02d0365c8cb04c4b32c46015f97bb1953a381b1b
Author: Dan Harrington <harringtond@chromium.org>
Date: Tue Jul 17 17:47:39 2018

prefetch: Execute() will no longer call run func with null db

It's very easy to forget checking for !db, which explains crbug.com/860228.
Now Execute does the check for us. The caller must designate a value
to be passed to the result callback in the case of db failure.

BUG:  864592 

Change-Id: Ie4e1ac0bcd60e9e0c8c30413af10d44ff0175b47
Reviewed-on: https://chromium-review.googlesource.com/1140277
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575709}
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/add_unique_urls_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/download_archives_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/download_cleanup_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/download_completed_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/finalize_dismissed_url_suggestion_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/generate_page_bundle_reconcile_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/generate_page_bundle_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/get_operation_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/import_archives_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/import_cleanup_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/import_completed_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/mark_operation_done_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/metrics_finalization_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/page_bundle_update_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/sent_get_operation_cleanup_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/stale_entry_finalizer_task.cc
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/store/prefetch_store.h
[modify] https://crrev.com/02d0365c8cb04c4b32c46015f97bb1953a381b1b/components/offline_pages/core/prefetch/store/prefetch_store_test_util.cc

Comment 3 by carlosk@chromium.org, Jul 17 

Re #1: Thanks fgorski@ for reminding us of that. It will be updated along with the code changes.

Comment 4 by harringtond@chromium.org, Jul 17 

Sending CL to update that README now.

Also, it looks like the request queue store doesn't use the same 'Execute' interface, so we only have to update prefetch and offline metadata stores.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d05eb406bf4f72752c919a5b41f33d49ad6f80a4

commit d05eb406bf4f72752c919a5b41f33d49ad6f80a4
Author: Dan Harrington <harringtond@chromium.org>
Date: Fri Jul 20 15:48:18 2018

Update prefetch store document for recent changes to Execute

Bug:  864592 
Change-Id: I494881191b3d315bc6a0ac1753463e311c5e0978
Reviewed-on: https://chromium-review.googlesource.com/1140906
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576877}
[modify] https://crrev.com/d05eb406bf4f72752c919a5b41f33d49ad6f80a4/components/offline_pages/core/prefetch/store/README.md
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35677dc820db52347a41ef5819f1528d1699143f

commit 35677dc820db52347a41ef5819f1528d1699143f
Author: Dan Harrington <harringtond@chromium.org>
Date: Fri Jul 20 16:22:37 2018

Execute() will no longer call run func with null db

It's very easy to forget checking for !db, which explains crbug.com/860228.
Now Execute does the check for us. The caller must designate a value
to be passed to the result callback in the case of db failure.

BUG:  864592 

Change-Id: Ie2c2cde1e06a81a7a1495e25578e39ab5b4e2b90
Reviewed-on: https://chromium-review.googlesource.com/1139026
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Yafei Duan <romax@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576889}
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/add_page_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/add_page_to_download_manager_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/cleanup_thumbnails_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/clear_digest_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/clear_storage_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/complete_offline_page_upgrade_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/delete_page_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/get_pages_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/get_pages_task.h
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/get_thumbnail_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/has_thumbnail_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/mark_page_accessed_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/persistent_page_consistency_check_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/start_offline_page_upgrade_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/startup_maintenance_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/store_thumbnail_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/model/update_file_path_task.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/offline_page_metadata_store.h
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/offline_page_metadata_store_test_util.cc
[modify] https://crrev.com/35677dc820db52347a41ef5819f1528d1699143f/components/offline_pages/core/offline_page_metadata_store_unittest.cc

Comment 7 by harringtond@chromium.org, Sep 6

Status: Fixed (was: Assigned)
►

Sign in to add a comment