DCHECK in RequestCoordinator when saving 2 pages |
|||||||
Issue description
<b>Version: <Kenneth, what is the frequency?></b>
<b>OS: <please tell me it's not XP></b>
What steps will reproduce the problem?
(1) go to chrome://offline-internals
(2) type "https://www.google.com,https://xkcd.com" into the download box
(3) Observe crash.
What do you see instead?
[FATAL:request_coordinator.cc(365)] Check failed: !is_busy_.
Stack Trace:
RELADDR FUNCTION FILE:LINE
0008ade9 ~LogMessage /usr/local/google/home/dewittj/src/chrome/src/base/logging.cc:532
00a04439 offline_pages::RequestCoordinator::SendRequestToOffliner(offline_pages::SavePageRequest const&) /usr/local/google/home/dewittj/src/chrome/src/components/offline_pages/background/request_coordinator.cc:365
v------> base::Callback<void (offline_pages::SavePageRequest const&), (base::internal::CopyMode)1>::Run(offline_pages::SavePageRequest const&) const /usr/local/google/home/dewittj/src/chrome/src/base/callback.h:388
00a05be5 offline_pages::RequestPicker::GetRequestResultCallback(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/components/offline_pages/background/request_picker.cc:95
-----------------------------------------------------
r0 00000000 r1 0000788a r2 00000006 r3 b6fc2b7c
r4 b6fc2b84 r5 b6fc2b34 r6 0000000b r7 0000010c
r8 bedb4aa4 r9 bedb4500 sl bedb4a9c fp 8d4b1f00
ip 00000006 sp bedb4498 lr b6d31b61 pc b6d33f50
Stack Trace:
RELADDR FUNCTION FILE:LINE
00041f50 tgkill+12 /system/lib/libc.so
0003fb5d pthread_kill+32 /system/lib/libc.so
0001c30f raise+10 /system/lib/libc.so
000194c1 __libc_android_abort+34 /system/lib/libc.so
000174ac abort+4 /system/lib/libc.so
v------> base::debug::(anonymous namespace)::DebugBreak() /usr/local/google/home/dewittj/src/chrome/src/base/debug/debugger_posix.cc:219
0007757f base::debug::BreakDebugger() /usr/local/google/home/dewittj/src/chrome/src/base/debug/debugger_posix.cc:249
0008afe7 ~LogMessage /usr/local/google/home/dewittj/src/chrome/src/base/logging.cc:748
00a04439 offline_pages::RequestCoordinator::SendRequestToOffliner(offline_pages::SavePageRequest const&) /usr/local/google/home/dewittj/src/chrome/src/components/offline_pages/background/request_coordinator.cc:365
v------> base::Callback<void (offline_pages::SavePageRequest const&), (base::internal::CopyMode)1>::Run(offline_pages::SavePageRequest const&) const /usr/local/google/home/dewittj/src/chrome/src/base/callback.h:388
00a05be5 offline_pages::RequestPicker::GetRequestResultCallback(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/components/offline_pages/background/request_picker.cc:95
v------> void base::internal::InvokeHelper<true, void>::MakeItSo<void (offline_pages::RequestPicker::* const&)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), base::WeakPtr<offline_pages::RequestPicker> const&, offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&>(void (offline_pages::RequestPicker::* const&)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), base::WeakPtr<offline_pages::RequestPicker> const&, offline_pages::RequestQueue::GetRequestsResult&&, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/base/bind_internal.h:304
v------> void base::internal::Invoker<base::internal::BindState<void (offline_pages::RequestPicker::*)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), base::WeakPtr<offline_pages::RequestPicker> >, void (offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&)>::RunImpl<void (offline_pages::RequestPicker::* const&)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), std::__1::tuple<base::WeakPtr<offline_pages::RequestPicker> > const&, 0u>(void (offline_pages::RequestPicker::* const&)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), std::__1::tuple<base::WeakPtr<offline_pages::RequestPicker> > const&, base::IndexSequence<0u>, offline_pages::RequestQueue::GetRequestsResult&&, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/base/bind_internal.h:347
00a06085 base::internal::Invoker<base::internal::BindState<void (offline_pages::RequestPicker::*)(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), base::WeakPtr<offline_pages::RequestPicker> >, void (offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&)>::Run(base::internal::BindStateBase*, offline_pages::RequestQueue::GetRequestsResult&&, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/base/bind_internal.h:325
v------> base::Callback<void (offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1>::Run(offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) const /usr/local/google/home/dewittj/src/chrome/src/base/callback.h:388
00a0637d offline_pages::(anonymous namespace)::GetRequestsDone(base::Callback<void (offline_pages::RequestQueue::GetRequestsResult, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1> const&, bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/components/offline_pages/background/request_queue.cc:27
v------> base::Callback<void (bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1>::Run(bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) const /usr/local/google/home/dewittj/src/chrome/src/base/callback.h:388
00a07111 void base::internal::FunctorTraits<base::Callback<void (bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1>, void>::Invoke<base::Callback<void (bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1> const&, bool const&, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&>(base::Callback<void (bool, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&), (base::internal::CopyMode)1> const&, bool const&, std::__1::vector<offline_pages::SavePageRequest, std::__1::allocator<offline_pages::SavePageRequest> > const&) /usr/local/google/home/dewittj/src/chrome/src/base/bind_internal.h:263
v------> base::Callback<void (), (base::internal::CopyMode)1>::Run() const /usr/local/google/home/dewittj/src/chrome/src/base/callback.h:388
0007a061 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) /usr/local/google/home/dewittj/src/chrome/src/base/debug/task_annotator.cc:54
00091065 base::MessageLoop::RunTask(base::PendingTask const&) /usr/local/google/home/dewittj/src/chrome/src/base/message_loop/message_loop.cc:488
00091207 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) /usr/local/google/home/dewittj/src/chrome/src/base/message_loop/message_loop.cc:497
00091385 base::MessageLoop::DoWork() /usr/local/google/home/dewittj/src/chrome/src/base/message_loop/message_loop.cc:621
v------> DoRunLoopOnce(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, long long, long long, long long) /usr/local/google/home/dewittj/src/chrome/src/base/message_loop/message_pump_android.cc:44
00092531 Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce /usr/local/google/home/dewittj/src/chrome/src/out/and/gen/base/base_jni_headers/base/jni/SystemMessageHandler_jni.h:44
0001bf1d offset 0x38000 /data/data/com.google.android.apps.chrome/incremental-install-files/optimized-dexes/base.base_java.dex.dex
,
Sep 2 2016
,
Sep 3 2016
The possible duplicate is https://bugs.chromium.org/p/chromium/issues/detail?id=641000 but that landed on Aug 29th.
,
Sep 6 2016
This is not a duplicate of that bug. The issue is that we have an asynchronous chain called when you call savePageLater: 1) SavePageLater 2) AddRequestResultCallback, which indirectly calls StartProcessing synchronously 3) RequestPicked, which calls SendRequestToOffliner synchronously. The race condition happens because is_busy_ is set in 3, but only checked in 2. So if two requests happen quickly, 1 and 2 can happen for both, before 3 happens for either. Then 3 is called for each one and the second one DCHECKs.
,
Sep 6 2016
The easy fix is to just check is_busy_ and return early rather than DCHECK. This won't work if the request picker is stateful. Doug, what do you think?
,
Sep 6 2016
That sounds like a good change but there is a potential issue with not calling the scheduler callback if it is the first one to call StartProcessing and Resume or AddRequest is second call and overwrites its callback.
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81356643cfe6ffe145741fb4b8fef53a3db86a50 commit 81356643cfe6ffe145741fb4b8fef53a3db86a50 Author: dougarnett <dougarnett@chromium.org> Date: Wed Sep 07 16:32:51 2016 [Offline Pages] Close race condition of multiple StartProcessing callers. Introduces new bool to track the starting/picking phase so that we avoid concurrent pickers (and not overwrite the schedule_callback) in order to also avoid the DCHECK that guards concurrent SendRequestToOffliner calls. BUG= 643455 Review-Url: https://codereview.chromium.org/2317893002 Cr-Commit-Position: refs/heads/master@{#416957} [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/components/offline_pages/background/request_coordinator.cc [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/components/offline_pages/background/request_coordinator.h [modify] https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50/components/offline_pages/background/request_coordinator_unittest.cc
,
Sep 7 2016
What is impact on M54 (Downloads and AGSA API)? What behavior is observed in release builds where there is no DCHECK?
,
Sep 7 2016
For debug build, I expect AGSA may find themselves only able to send one request at a time. We need to verify on release build but from code inspection I expect a second request to not be accepted here: https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/prerendering_offliner.cc?rcl=0&l=112 and then the first request will be stopped subsequently here: https://cs.chromium.org/chromium/src/components/offline_pages/background/request_coordinator.cc?rcl=1473247219&l=456 and so processing will need to then wait until a GcmNetworkManager schedule window is triggered. So effect may be a delay of at least many minutes (but need to confirm that indeed will end up working). Btw, I have confirmed the landed patch works for Justin's repro case of comma separated list of URLs in internal page.
,
Sep 7 2016
Confirmed on an M54 release build that multiple pages on same offline-internals page load box do eventually load with no crashes (but loading is delayed - ie, immediate load does not happen).
,
Sep 7 2016
Dmitry, Justin, should we merge for M54? I gave my assessment of current situation with M54 in comments 9 & 10. I think the delay would be a real drawback for effectiveness of AGSA's use so I favor doing the merge.
,
Sep 8 2016
,
Sep 8 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 8 2016
Fixed in origin
,
Sep 8 2016
I am merging this now.
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7 commit f68a8dfdcc256445b040cb9cf0f447e1b8e39de7 Author: Justin DeWitt <dewittj@chromium.org> Date: Thu Sep 08 20:21:31 2016 [Offline Pages] Close race condition of multiple StartProcessing callers. Introduces new bool to track the starting/picking phase so that we avoid concurrent pickers (and not overwrite the schedule_callback) in order to also avoid the DCHECK that guards concurrent SendRequestToOffliner calls. BUG= 643455 Review-Url: https://codereview.chromium.org/2317893002 Cr-Commit-Position: refs/heads/master@{#416957} (cherry picked from commit 81356643cfe6ffe145741fb4b8fef53a3db86a50) R=dougarnett@chromium.org Review URL: https://codereview.chromium.org/2325563003 . Cr-Commit-Position: refs/branch-heads/2840@{#248} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator.cc [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator.h [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator_unittest.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7 commit f68a8dfdcc256445b040cb9cf0f447e1b8e39de7 Author: Justin DeWitt <dewittj@chromium.org> Date: Thu Sep 08 20:21:31 2016 [Offline Pages] Close race condition of multiple StartProcessing callers. Introduces new bool to track the starting/picking phase so that we avoid concurrent pickers (and not overwrite the schedule_callback) in order to also avoid the DCHECK that guards concurrent SendRequestToOffliner calls. BUG= 643455 Review-Url: https://codereview.chromium.org/2317893002 Cr-Commit-Position: refs/heads/master@{#416957} (cherry picked from commit 81356643cfe6ffe145741fb4b8fef53a3db86a50) R=dougarnett@chromium.org Review URL: https://codereview.chromium.org/2325563003 . Cr-Commit-Position: refs/branch-heads/2840@{#248} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator.cc [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator.h [modify] https://crrev.com/f68a8dfdcc256445b040cb9cf0f447e1b8e39de7/components/offline_pages/background/request_coordinator_unittest.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by dewittj@chromium.org
, Sep 2 2016