Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10786038:
Prevent loading libffmpegsumo.so to python (autotests) process. (Closed)
|
Created:
8 years, 5 months ago by glotov Modified:
8 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Nirnimesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrevent loading libffmpegsumo.so to python (autotests) process.
This is needed for ASAN environment because libffmpegsumo.so, being
built with ASAN support, fails to load to python process (which is not
ASAN-ed currently). At the same time it loads file to chrome or to
gtest processes (like content_unittests) because they do have ASAN
runtime.
FYI: Alternative approach (https://chromiumcodereview.appspot.com/10692190/)
was skipped because it disallowed ASAN to test libffmpegsumo.so.
BUG=chromium-os:32259
TESTS=unit
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148183
Patch Set 1 #Patch Set 2 : restrict to OS_CHROMEOS #
Total comments: 4
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : sync + name change #
Total comments: 2
Patch Set 6 : update #Patch Set 7 : sync + iteration #
Total comments: 4
Patch Set 8 : iteration #
Total comments: 6
Patch Set 9 : fix #
Messages
Total messages: 26 (0 generated)
Hi Andrew! I think I found much simpler solution. What do you think?
https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... content/test/content_test_suite_base.cc:29: #ifdef OS_CHROMEOS #if defined(OS_CHROMEOS) https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... content/test/content_test_suite_base.cc:33: const char kPythonPrefix[] = "/usr/bin/python"; would this catch processes started with /usr/bin/env python? overall this seems a bit brittle but I'm not a linux guy so I'm not sure what's the best way to figure out whether we've been invoked via python or not Is there a stronger signal of some sort (i.e., command line flag) that we can use as opposed to snooping /proc/self/cmdline?
https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... content/test/content_test_suite_base.cc:29: #ifdef OS_CHROMEOS On 2012/07/18 17:40:02, scherkus wrote: > #if defined(OS_CHROMEOS) Done. https://chromiumcodereview.appspot.com/10786038/diff/5002/content/test/conten... content/test/content_test_suite_base.cc:33: const char kPythonPrefix[] = "/usr/bin/python"; On 2012/07/18 17:40:02, scherkus wrote: > would this catch processes started with /usr/bin/env python? > > overall this seems a bit brittle but I'm not a linux guy so I'm not sure what's > the best way to figure out whether we've been invoked via python or not > > Is there a stronger signal of some sort (i.e., command line flag) that we can > use as opposed to snooping /proc/self/cmdline? Done.
lgtm w/ nits +nirnimesh as FYI since he also solved some FFmpeg + PyAuto bugs https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... content/test/content_test_suite_base.cc:8: #include <unistd.h> this doesn't exist on windows (compile failure) https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... content/test/content_test_suite_base.cc:12: #include "base/eintr_wrapper.h" this isn't used anymore
https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... content/test/content_test_suite_base.cc:8: #include <unistd.h> On 2012/07/19 20:38:53, scherkus wrote: > this doesn't exist on windows (compile failure) Done. https://chromiumcodereview.appspot.com/10786038/diff/20002/content/test/conte... content/test/content_test_suite_base.cc:12: #include "base/eintr_wrapper.h" On 2012/07/19 20:38:53, scherkus wrote: > this isn't used anymore Done.
https://chromiumcodereview.appspot.com/10786038/diff/23001/content/test/conte... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/23001/content/test/conte... content/test/content_test_suite_base.cc:39: return false; This statement is not true for non-chromeos. At least it doesn't fit go with the name of this function since pyauto on non-chromeos would reach here.
https://chromiumcodereview.appspot.com/10786038/diff/23001/content/test/conte... File content/test/content_test_suite_base.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/23001/content/test/conte... content/test/content_test_suite_base.cc:39: return false; On 2012/07/20 01:57:37, Nirnimesh wrote: > This statement is not true for non-chromeos. At least it doesn't fit go with the > name of this function since pyauto on non-chromeos would reach here. Done.
Code I commented on LGTM
http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... File content/test/content_test_suite_base.cc (right): http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... content/test/content_test_suite_base.cc:32: int num_read = readlink("/proc/self/exe", buf, sizeof(buf) - 1); this is a layering violation, since content shouldn't know about chromeos paths. since ContentTestSuiteBase is inherited by Chrome's test suite, move this code there, and you can pass a boolean to this Initialize method to say whether to suppress initializing the media library.
http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... File content/test/content_test_suite_base.cc (right): http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... content/test/content_test_suite_base.cc:32: int num_read = readlink("/proc/self/exe", buf, sizeof(buf) - 1); Adding a parameter to the Initialize() function will force changing its descendants, so I tried to avoid it by moving media library init to the Chrome's class. Running unittests to make sure it works... On 2012/07/20 21:46:16, John Abd-El-Malek wrote: > this is a layering violation, since content shouldn't know about chromeos paths. > > since ContentTestSuiteBase is inherited by Chrome's test suite, move this code > there, and you can pass a boolean to this Initialize method to say whether to > suppress initializing the media library.
I committed the change to make bots green. If the change does not look good, please leave comments, I'll make another CL. On 2012/07/20 23:47:29, glotov wrote: > http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... > File content/test/content_test_suite_base.cc (right): > > http://codereview.chromium.org/10786038/diff/21005/content/test/content_test_... > content/test/content_test_suite_base.cc:32: int num_read = > readlink("/proc/self/exe", buf, sizeof(buf) - 1); > Adding a parameter to the Initialize() function will force changing its > descendants, so I tried to avoid it by moving media library init to the Chrome's > class. Running unittests to make sure it works... > > On 2012/07/20 21:46:16, John Abd-El-Malek wrote: > > this is a layering violation, since content shouldn't know about chromeos > paths. > > > > since ContentTestSuiteBase is inherited by Chrome's test suite, move this code > > there, and you can pass a boolean to this Initialize method to say whether to > > suppress initializing the media library.
On 2012/07/21 14:52:42, glotov wrote: > I committed the change to make bots green. If the change does not look good, > please leave comments, I'll make another CL. This isn't how things work. You can't land changes without approval of owners. If the test is broken upstream (i.e. cros bots), you can disable them there until this is figured out. You moved the cros specific code from one file in content to another file in content, which doesn't address the issues I raised up. I'm reverting this change now.
Done, as discussed by chat. Please have a look now.
http://codereview.chromium.org/10786038/diff/25001/chrome/test/base/chrome_te... File chrome/test/base/chrome_test_suite.cc (right): http://codereview.chromium.org/10786038/diff/25001/chrome/test/base/chrome_te... chrome/test/base/chrome_test_suite.cc:176: content::ContentTestSuiteBase::DisableLoadingExternalLibraries(); now you're doing this for all chrome test suites, which I don't think you want to do. i.e. shouldn't this be done as a result of doing the cros specific code you had in the previous patch? http://codereview.chromium.org/10786038/diff/25001/content/public/test/conten... File content/public/test/content_test_suite_base.h (right): http://codereview.chromium.org/10786038/diff/25001/content/public/test/conten... content/public/test/content_test_suite_base.h:29: // process. In case of autotests (the process is actually a python) nit: don't mention autotests in src/content. A developer reading this code most likely won't know what that is.
https://chromiumcodereview.appspot.com/10786038/diff/25001/chrome/test/base/c... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/25001/chrome/test/base/c... chrome/test/base/chrome_test_suite.cc:176: content::ContentTestSuiteBase::DisableLoadingExternalLibraries(); On 2012/07/23 23:02:40, John Abd-El-Malek wrote: > now you're doing this for all chrome test suites, which I don't think you want > to do. i.e. shouldn't this be done as a result of doing the cros specific code > you had in the previous patch? Done. https://chromiumcodereview.appspot.com/10786038/diff/25001/content/public/tes... File content/public/test/content_test_suite_base.h (right): https://chromiumcodereview.appspot.com/10786038/diff/25001/content/public/tes... content/public/test/content_test_suite_base.h:29: // process. In case of autotests (the process is actually a python) On 2012/07/23 23:02:40, John Abd-El-Malek wrote: > nit: don't mention autotests in src/content. A developer reading this code most > likely won't know what that is. Done.
lgtm with nits http://codereview.chromium.org/10786038/diff/22009/chrome/test/base/chrome_te... File chrome/test/base/chrome_test_suite.cc (right): http://codereview.chromium.org/10786038/diff/22009/chrome/test/base/chrome_te... chrome/test/base/chrome_test_suite.cc:167: static bool IsCrosPythonProcess() { nit: put static method in the annonymous namespace beside the other static method, i.e. line 56. then the "static" should be redundant http://codereview.chromium.org/10786038/diff/22009/content/public/test/conten... File content/public/test/content_test_suite_base.h (right): http://codereview.chromium.org/10786038/diff/22009/content/public/test/conten... content/public/test/content_test_suite_base.h:29: void DisableLoadingExternalLibraries() { nit: since you're inlining this, should be void set_extenral_libraries_enabled(bool value) { external_libraries_enabled_ = value; }
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/glotov@chromium.org/10786038/11008
https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... chrome/test/base/chrome_test_suite.cc:167: static bool IsCrosPythonProcess() { I'd leave it here: locality is more important for readability IMO. On 2012/07/24 15:32:19, John Abd-El-Malek wrote: > nit: put static method in the annonymous namespace beside the other static > method, i.e. line 56. then the "static" should be redundant https://chromiumcodereview.appspot.com/10786038/diff/22009/content/public/tes... File content/public/test/content_test_suite_base.h (right): https://chromiumcodereview.appspot.com/10786038/diff/22009/content/public/tes... content/public/test/content_test_suite_base.h:29: void DisableLoadingExternalLibraries() { On 2012/07/24 15:32:19, John Abd-El-Malek wrote: > nit: since you're inlining this, should be > > void set_extenral_libraries_enabled(bool value) { > external_libraries_enabled_ = value; > } Done.
https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... chrome/test/base/chrome_test_suite.cc:167: static bool IsCrosPythonProcess() { On 2012/07/24 16:23:41, glotov wrote: > I'd leave it here: locality is more important for readability IMO. > > On 2012/07/24 15:32:19, John Abd-El-Malek wrote: > > nit: put static method in the annonymous namespace beside the other static > > method, i.e. line 56. then the "static" should be redundant > it's chrome style to put static functions that aren't members of a class at the top of the file, instead of interspersing them within the class definitions
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/glotov@chromium.org/10786038/25004
https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/10786038/diff/22009/chrome/test/base/c... chrome/test/base/chrome_test_suite.cc:167: static bool IsCrosPythonProcess() { On 2012/07/24 16:27:55, John Abd-El-Malek wrote: > On 2012/07/24 16:23:41, glotov wrote: > > I'd leave it here: locality is more important for readability IMO. > > > > On 2012/07/24 15:32:19, John Abd-El-Malek wrote: > > > nit: put static method in the annonymous namespace beside the other static > > > method, i.e. line 56. then the "static" should be redundant > > > > it's chrome style to put static functions that aren't members of a class at the > top of the file, instead of interspersing them within the class definitions Done.
Try job failure for 10786038-25004 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/glotov@chromium.org/10786038/25004
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 404: Not Found
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/glotov@chromium.org/10786038/2016
Change committed as 148183 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
