Fix valgrind failures in PhishingTermFeatureExtractor and remove supressions for said failures.

Fix valgrind failures in PhishingTermFeatureExtractor and remove supressions for said failures.


BUG=92267
TEST=Ran PhishingTermFeatureExtractor unittest under valgrind.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96143

[gcasto (DO NOT USE), 2011-08-10 03:09:10]

Actually, hold off on this review.

On Tue, Aug 9, 2011 at 7:36 PM, <gcasto@chromium.org> wrote:

> Reviewers: Brian Ryner, noelutz,
>
> Description:
> Fix valgrind failures in PhishingTermFeatureExtractor and remove
> supressions for
> said failures.
>
>
> BUG=92267
> TEST=Ran PhishingTermFeatureExtractor unittest under valgrind.
>
>
> Please review this at
http://codereview.chromium.**org/7604022/<http://codereview.chromium.org/7604...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/renderer/safe_browsing/**phishing_term_feature_**
> extractor_unittest.cc
>  M tools/valgrind/memcheck/**suppressions.txt
>
>
> Index: chrome/renderer/safe_browsing/**phishing_term_feature_**
> extractor_unittest.cc
> diff --git
a/chrome/renderer/safe_**browsing/phishing_term_**feature_extractor_unittest.cc
> b/chrome/renderer/safe_**browsing/phishing_term_**
> feature_extractor_unittest.cc
> index 1b18bf175f657e8a8919f6b75054bd**32365fcd2a..**
> 20f77156153959670af1584f787dc2**4ba6f0bf31 100644
> --- a/chrome/renderer/safe_**browsing/phishing_term_**
> feature_extractor_unittest.cc
> +++ b/chrome/renderer/safe_**browsing/phishing_term_**
> feature_extractor_unittest.cc
> @@ -240,6 +240,11 @@ TEST_F(**PhishingTermFeatureExtractorTe**st,
> Continuation) {
>
>   features.Clear();
>   EXPECT_FALSE(ExtractFeatures(&**page_text, &features));
> +  // We need to kill the extractor now, because if we don't then it will
> be
> +  // deleted after page_text has gone out of scope which causes memory
> +  // corruption as the extractor has internal data structures that depend
> +  // on page_text.
> +  extractor_.reset(NULL);
>  }
>
>  }  // namespace safe_browsing
> Index: tools/valgrind/memcheck/**suppressions.txt
> diff --git a/tools/valgrind/memcheck/**suppressions.txt
> b/tools/valgrind/memcheck/**suppressions.txt
> index 23b446fef58a232f857632ce8c5a9e**88b3f0297b..**
> 6a0bdf619595eb4b5cd0cab1574bf9**4cf7e49493 100644
> --- a/tools/valgrind/memcheck/**suppressions.txt
> +++ b/tools/valgrind/memcheck/**suppressions.txt
> @@ -5243,58 +5243,6 @@
>    fun:_**ZN14RunnableMethodIN3net16Host**ResolverImpl3JobEMS2_**
> FvRKN4base9TimeTicksEjE6Tuple2**IS4_jEE13ReleaseCalleeEv
>    fun:_**ZN14RunnableMethodIN3net16Host**ResolverImpl3JobEMS2_**
> FvRKN4base9TimeTicksEjE6Tuple2**IS4_jEED0Ev
>  }
> -{
> -   bug_92267_a
> -   Memcheck:Addr2
> -   fun:_ZNK9__gnu_**cxx4hashIN4base13StringPiece16**EEclERKS2_
> -   fun:_ZNK9__gnu_**cxx9hashtableISt4pairIKN4base1**
> 3StringPiece16ESt14_List_**iteratorIS1_IS3_bEEES3_NS_**
> 4hashIS3_EESt10_Select1stIS8_**ESt8equal_toIS3_ESaIS7_EE14_M_**
> bkt_num_keyERS4_j
> -   fun:_ZNK9__gnu_**cxx9hashtableISt4pairIKN4base1**
> 3StringPiece16ESt14_List_**iteratorIS1_IS3_bEEES3_NS_**
> 4hashIS3_EESt10_Select1stIS8_**ESt8equal_toIS3_ESaIS7_EE14_M_**
> bkt_num_keyERS4_
> -   fun:_ZN9__gnu_**cxx9hashtableISt4pairIKN4base1**
> 3StringPiece16ESt14_List_**iteratorIS1_IS3_bEEES3_NS_**
> 4hashIS3_EESt10_Select1stIS8_**ESt8equal_toIS3_ESaIS7_**EE5eraseERS4_
> -   fun:_ZN9__gnu_cxx8hash_**mapIN4base13StringPiece16ESt14**
> _List_iteratorISt4pairIS2_**bEENS_4hashIS2_EESt8equal_**
> toIS2_ESaIS6_EE5eraseERKS2_
> -   fun:_ZN4base12MRUCacheBaseINS_**13StringPiece16EbNS_**
> 19MRUCacheNullDeletorIbEENS_**15MRUCacheHashMapEE5EraseESt14**
> _List_iteratorISt4pairIS1_bEE
> -   fun:_ZN4base12MRUCacheBaseINS_**13StringPiece16EbNS_**
> 19MRUCacheNullDeletorIbEENS_**15MRUCacheHashMapEED2Ev
> -   fun:_**ZN4base15HashingMRUCacheINS_**13StringPiece16EbED1Ev
> -   fun:_ZN13safe_**browsing28PhishingTermFeatureE**xtractorD1Ev
> -   fun:_ZN10scoped_ptrIN13safe_**browsing28PhishingTermFeatureE**
> xtractorEED1Ev
> -   fun:_ZN13safe_**browsing32PhishingTermFeatureE**xtractorTestD2Ev
> -   fun:_ZN13safe_**browsing50PhishingTermFeatureE**
> xtractorTest_Continuation_**TestD0Ev
> -   fun:_**ZN7testing4Test11DeleteSelf_Ev
> -   fun:_**ZN7testing8internal38HandleSeh**ExceptionsInMethodIfSupportedI*
> *NS_4TestEvEET0_PT_MS4_FS3_**vEPKc
> -   fun:_**ZN7testing8internal35HandleExc**eptionsInMethodIfSupportedINS_*
> *4TestEvEET0_PT_MS4_FS3_vEPKc
> -   fun:_ZN7testing8TestInfo3RunEv
> -   fun:_ZN7testing8TestCase3RunEv
> -   fun:_**ZN7testing8internal12UnitTestI**mpl11RunAllTestsEv
> -   fun:_**ZN7testing8internal38HandleSeh**ExceptionsInMethodIfSupportedI*
> *NS0_12UnitTestImplEbEET0_PT_**MS4_FS3_vEPKc
> -   fun:_**ZN7testing8internal35HandleExc**eptionsInMethodIfSupportedINS0*
> *_12UnitTestImplEbEET0_PT_MS4_**FS3_vEPKc
> -   fun:_ZN7testing8UnitTest3RunEv
> -   fun:_ZN4base9TestSuite3RunEv
> -}
> -{
> -   bug_92267_b
> -   Memcheck:Addr2
> -   fun:_ZN4base9c16memcmpEPKtS1_j
> -   fun:_ZN4base20string16_char_**traits7compareEPKtS2_j
> -   fun:_**ZN4base13StringPiece1610wordme**mcmpEPKtS2_j
> -   fun:_ZN4baseeqERKNS_**13StringPiece16ES2_
> -   fun:_ZNKSt8equal_**toIN4base13StringPiece16EEclER**KS1_S4_
> -   fun:_ZN9__gnu_**cxx9hashtableISt4pairIKN4base1**
> 3StringPiece16ESt14_List_**iteratorIS1_IS3_bEEES3_NS_**
> 4hashIS3_EESt10_Select1stIS8_**ESt8equal_toIS3_ESaIS7_**EE5eraseERS4_
> -   fun:_ZN9__gnu_cxx8hash_**mapIN4base13StringPiece16ESt14**
> _List_iteratorISt4pairIS2_**bEENS_4hashIS2_EESt8equal_**
> toIS2_ESaIS6_EE5eraseERKS2_
> -   fun:_ZN4base12MRUCacheBaseINS_**13StringPiece16EbNS_**
> 19MRUCacheNullDeletorIbEENS_**15MRUCacheHashMapEE5EraseESt14**
> _List_iteratorISt4pairIS1_bEE
> -   fun:_ZN4base12MRUCacheBaseINS_**13StringPiece16EbNS_**
> 19MRUCacheNullDeletorIbEENS_**15MRUCacheHashMapEED2Ev
> -   fun:_**ZN4base15HashingMRUCacheINS_**13StringPiece16EbED1Ev
> -   fun:_ZN13safe_**browsing28PhishingTermFeatureE**xtractorD1Ev
> -   fun:_ZN10scoped_ptrIN13safe_**browsing28PhishingTermFeatureE**
> xtractorEED1Ev
> -   fun:_ZN13safe_**browsing32PhishingTermFeatureE**xtractorTestD2Ev
> -   fun:_ZN13safe_**browsing50PhishingTermFeatureE**
> xtractorTest_Continuation_**TestD0Ev
> -   fun:_**ZN7testing4Test11DeleteSelf_Ev
> -   fun:_**ZN7testing8internal38HandleSeh**ExceptionsInMethodIfSupportedI*
> *NS_4TestEvEET0_PT_MS4_FS3_**vEPKc
> -   fun:_**ZN7testing8internal35HandleExc**eptionsInMethodIfSupportedINS_*
> *4TestEvEET0_PT_MS4_FS3_vEPKc
> -   fun:_ZN7testing8TestInfo3RunEv
> -   fun:_ZN7testing8TestCase3RunEv
> -   fun:_**ZN7testing8internal12UnitTestI**mpl11RunAllTestsEv
> -   fun:_**ZN7testing8internal38HandleSeh**ExceptionsInMethodIfSupportedI*
> *NS0_12UnitTestImplEbEET0_PT_**MS4_FS3_vEPKc
> -   fun:_**ZN7testing8internal35HandleExc**eptionsInMethodIfSupportedINS0*
> *_12UnitTestImplEbEET0_PT_MS4_**FS3_vEPKc
> -}
>
>  #-----------------------------**------------------------------**
> ------------
>  # These only occur on our Google workstations
>
>
>

[Brian Ryner, 2011-08-10 03:49:16]

http://codereview.chromium.org/7604022/diff/1/chrome/renderer/safe_browsing/p...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/7604022/diff/1/chrome/renderer/safe_browsing/p...
chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc:247:
extractor_.reset(NULL);
Doesn't this problem also affect the ExtractFeatures test above?

[Garrett Casto, 2011-08-10 04:56:29]

Alright, fixed the actual problem. Please take a look.

http://codereview.chromium.org/7604022/diff/1/chrome/renderer/safe_browsing/p...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/7604022/diff/1/chrome/renderer/safe_browsing/p...
chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc:247:
extractor_.reset(NULL);
On 2011/08/10 03:49:17, Brian Ryner wrote:
> Doesn't this problem also affect the ExtractFeatures test above?

No. At first I thought that this was because the last thing that it extracts is
an empty page, but really it's because we only have this issue if we timeout.
I've fixed the actual issue now.

[Brian Ryner, 2011-08-10 05:04:49]

LGTM

good catch