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

Issue 846417 link

Starred by 1 user
Status: Fixed
Owner:
█ toniki...@igalia.com
Last visit > 30 days ago
Closed: May 2018
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

browser_tests are flaking on Win7 bots

Project Member Reported by gab@chromium.org, May 24 2018 
e.g. https://ci.chromium.org/buildbot/chromium.win/Win7%20Tests%20%281%29/80200 (WebstoreInlineInstallerRedirectTest.NoRedirectDataWhenSafeBrowsingDisabled)
and https://ci.chromium.org/buildbot/chromium.win/Win7%20Tests%20%281%29/80201 (ExtensionWebstorePrivateApiTest.InstallLocalized)

Stack always looks like :

Received fatal exception EXCEPTION_ACCESS_VIOLATION
Backtrace:
	mojo::edk::NodeChannel::OnChannelMessage [0x0208A410+246]
	network::SimpleURLLoader::Create [0x03120323+635]
	extensions::internals::ContentHashFetcher::Start [0x02613346+260]
	extensions::ContentHash::FetchVerifiedContentsOnIOThread [0x02616A32+118]
	base::internal::FunctorTraits<void (__cdecl*)(base::RepeatingCallback<content::WebContents * __cdecl(void)> const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,std::basic_string<char,std::char_traits<char>,std::allocator<c [0x02615A7C+60]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(extensions::ContentHash::ExtensionKey const &,extensions::ContentHash::FetchParams const &,base::RepeatingCallback<bool __cdecl(void)> const &,base::OnceCallback<void __cdecl(scoped_refptr< [0x02615A38+36]
	base::debug::TaskAnnotator::RunTask [0x02F959D6+230]
	base::internal::IncomingTaskQueue::RunTask [0x03C08193+19]
	base::MessageLoop::RunTask [0x02FB15E6+438]
	base::MessageLoop::DeferOrRunPendingTask [0x02FB1913+83]
	base::MessageLoop::DoWork [0x02FB1A23+211]
	base::MessagePumpForIO::DoRunLoop [0x02FB3125+309]
	base::MessagePumpWin::Run [0x02FB263E+110]
	base::MessageLoop::Run [0x02FB133F+31]
	base::RunLoop::Run [0x02FCE6BE+46]
	base::Thread::Run [0x02FE4E7B+11]
	content::BrowserProcessSubThread::IOThreadRun [0x0223D10E+36]
	base::Thread::ThreadMain [0x02FE4FCD+333]
	base::PlatformThread::GetCurrentThreadPriority [0x02FE3D9B+299]
	BaseThreadInitThunk [0x76D1338A+18]
	RtlInitializeExceptionChain [0x77789F72+99]
	RtlInitializeExceptionChain [0x77789F45+54]

Suspecting https://chromium-review.googlesource.com/c/chromium/src/+/1056587 which touched ContentHash::FetchVerifiedContentsOnIOThread().
Project Member

Comment 1 by bugdroid1@chromium.org, May 24 2018 

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

commit 082ca997aba8426949fb918d8f38f552100d96b0
Author: Gabriel Charette <gab@chromium.org>
Date: Thu May 24 18:47:07 2018

Revert "Migrate ContentHashFetcher to SimpleURLLoader"

This reverts commit 27f8891c7cd9a86b2c92508cf6818726df4058d6.

Reason for revert: suspect for  crbug.com/846417 

Original change's description:
> Migrate ContentHashFetcher to SimpleURLLoader
> 
> This CL migrates ContentHashFetcher away from URLRequestFetcher to
> SimpleURLLoader.
> 
> In order to accomplish this, a few other related changes are also performed:
> 
> 1) The regular flow of the code is ContentVerifier -> ContentHash ->
>    ContentHashFetcher. In ContentVerifier, for instance, UI, IO and
>    "File tasks" threads take place.
>    Previously, the URLRequestFetcher logic residing in ContentHashFetcher
>    executed in the "file tasks" thread.
> 
>    As part of the migration from URLRequestFetcher to SimpleURLLoader
>    machinery, this CL also changes the ContentHashFetcher logic to
>    execute to the IO thread.
>    Note that it could be possible to keep the new SimpleURLLoader logic
>    in the "file tasks" thread. However, this would impose a way bigger change,
>    and require unittests to be considerably rewritten as well (see for
>    patchsets 7, 8 and 9.
>    The issue is that mojo objects are not thread safe. We could create a new
>    URLLoaderFactory for the extensions thread, but then we'd need to create a
>    new one when the network service crashes (Something this SharedURLLoaderFactory
>    one, already bound to the IOThread, magically handles), but that would be
>    significantly more complicated.
> 
> 2) content_verifier_hash_fetch_behavior_browsertest.cc was changed
>    to support running with the network service feature both enabled
>    and disabled. This is a pattern that is also present in various other
>    unittests.
> 
> In summary, the migration to use SimpleURLLoader performed here includes
> also a change in the thread that ContentHashFetcher runs on, but no
> functionality change is expected from this CL.
> 
> BUG=773295, 844926 
> 
> Change-Id: If570f69d01ff75ac59d8d043f8687621336dddcf
> Reviewed-on: https://chromium-review.googlesource.com/1056587
> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561480}

TBR=lazyboy@chromium.org,rdevlin.cronin@chromium.org,mmenke@chromium.org,tonikitoo@igalia.com

Change-Id: I31235cf26cfff34aac0ff1e4047575ed2881722b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 773295,  844926 ,  846417 
Reviewed-on: https://chromium-review.googlesource.com/1072110
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561572}
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/BUILD.gn
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/DEPS
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier.h
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier/content_hash.h

Comment 2 by gab@chromium.org, May 24 2018

Status: Fixed (was: Assigned)
Hasn't flaked since.
►

Sign in to add a comment