content_browsertests should not attempt to load unused pak files on Android |
||||||
Issue descriptionLoaded here: https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle_android.cc?type=cs&sq=package:chromium&q=resource_bundle_an&g=0&l=85 From this callstack: """ 128 RELADDR FUNCTION FILE:LINE 129 00000000050f4577 ui::(anonymous namespace)::LoadFromApkOrFile(char const*, base::FilePath const*, int*, base::MemoryMappedFile::Region*) ??:0:0 130 00000000050f43c3 ui::ResourceBundle::LoadCommonResources() ??:0:0 131 00000000050f2b47 ui::ResourceBundle::InitSharedInstanceWithLocale(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, ui::ResourceBundle::Delegate*, ui ::ResourceBundle::LoadResources) ??:0:0 132 00000000044668f3 content::TestContentClient::TestContentClient() ??:0:0 133 000000000445463f content::RenderViewTest::CreateContentClient() ??:0:0 134 0000000004453dc7 content::RenderViewTest::SetUp() ??:0:0 135 0000000002c7ebe7 testing::Test::Run() ??:0:0 136 0000000002c7f247 testing::TestInfo::Run() ??:0:0 137 0000000002c7f59f testing::TestCase::Run() ??:0:0 138 0000000002c84023 testing::internal::UnitTestImpl::RunAllTests() ??:0:0 139 0000000002c83dc7 testing::UnitTest::Run() ??:0:0 140 000000000446e6af base::TestSuite::Run() ??:0:0 141 000000000443dc33 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) ??:0:0 142 000000000445ad7b content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) ??:0:0 143 000000000443dbf7 main """ By default, the APK does not include the pak file. The error is gracefully handled and the test continues as normal. However, it's possible for other APKs to install the pak file in the common resource directory. This will cause DCHECKs later on since this pak file has duplicate resources with content_shell.pak. This also applies to other binaries. At the very least, components_browsertests. See Issue 906587 for more context.
,
Dec 4
+agrieve, who seems to be the expert on resource loading, as per https://codereview.chromium.org/1181953002
,
Dec 4
+ avi, sky -- as I'm missing historical context. The code for ResourceBundle::LoadCommonResources attempts to load "common" resources, which are in chrome_100_percent.pak. This seems pretty reasonable, and it would be tricky to change the code to have ResourceBundle::LoadCommonResources not do this, or to not call ResourceBundle::LoadCommonResources. What I don't understand is why 1) content_shell.pak repackages some of these same resources 2) We then don't bother emitting chrome_100_percent.pak as a data dependency for some binaries, even though those binaries still go through the ResourceBundle::LoadCommonResources code path. Either we should have a set of common resources that we always emit. This means we don't need to change the logic ResourceBundle::LoadCommonResources. Or we could not have a set of common resources, in which we *should* change the logic in ResourceBundle::LoadCommonResources to sometimes be a no-op. I'm not sure what to use in the conditional though, since that translation unit is used both binaries that load common resources, and those that don't.
,
Dec 4
I have no context here. I tried to make loading a missing pak file an error and that broke the world. That's the extent of my knowledge here :(
,
Dec 5
We should only call ui::ResourceBundle::InitSharedInstanceWithLocale(..., ui::ResourceBundle::LOAD_COMMON_RESOURCES) if these common resources are present. This CL: https://chromium-review.googlesource.com/c/chromium/src/+/628171/36/content/test/test_content_client.cc#b53 Added the call to InitSharedInstanceWithLocale() that is causing problems on Android. I think that it should be safe to remove, since the test content client should have the relevant resources packed into content_shell.pak (?) + beccahughes -- do you remember why the call to InitSharedInstanceWithLocale() was added?
,
Dec 5
,
Dec 5
As far as I can remember this was so that ui::ResourceBundle::GetSharedInstance could be used in Blink to access the pak file.
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75d116ca56e6212e4ec6afc752957cd8916977ab commit 75d116ca56e6212e4ec6afc752957cd8916977ab Author: erikchen <erikchen@chromium.org> Date: Wed Dec 05 18:14:02 2018 TestContentClient should not try to load common resources. All resources used by content shell are in content_shell.pak. Attempting to load common resources will either fail to find the .pak file, or succeed and hit a DCHECK as there are duplicated resources between content_shell.pak and common resources. I've confirmed that content_browsertests fails without this CL, passes with this CL with the following repro steps: https://bugs.chromium.org/p/chromium/issues/detail?id=906587#c52 Ditto for compontents_browsertests. Change-Id: Ica7234898c6dc2c220340e942f827b331dfd14b0 Bug: 911764 , 906587 Reviewed-on: https://chromium-review.googlesource.com/c/1363441 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#614026} [modify] https://crrev.com/75d116ca56e6212e4ec6afc752957cd8916977ab/content/test/test_content_client.cc
,
Dec 5
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by erikc...@chromium.org
, Dec 4