New issue
Advanced search Search tips

Issue 812600 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

webkit_unittests CSSFontFaceSourceTest.UnboundedGrowth fails when compiled with MSVC

Project Member Reported by h...@chromium.org, Feb 15 2018

Issue description

From https://ci.chromium.org/buildbot/chromium.clang/CrWinClang%28shared%29/1540

[ RUN      ] CSSFontFaceSourceTest.UnboundedGrowth
[588:4528:0214/153951.025:2538822:FATAL:HashTable.h(600)] Security DCHECK failed: container_modifications_ == container_->Modifications().
Backtrace:
	base::debug::StackTrace::StackTrace [0x7415BD47+55]
	base::debug::StackTrace::StackTrace [0x7415B89A+10]
	WTF::HashTableAddResult<WTF::HashTable<blink::FontCacheKey,WTF::KeyValuePair<blink::FontCacheKey,scoped_refptr<blink::SimpleFontData> >,WTF::KeyValuePairKeyExtractor,blink::FontCacheKeyHash,WTF::HashMapValueTraits<blink::FontCacheKeyTraits,WTF::HashTraits [0x6E5DA655+109]
	blink::CSSFontFaceSource::GetFontData [0x6E5DAFC4+312]
	blink::CSSFontFaceSourceTest_UnboundedGrowth_Test::TestBody [0x017C0BF1+401]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x012D7099+32]
	testing::Test::Run [0x012E3CCC+93]
	testing::TestCase::Run [0x012E3D99+133]
	testing::internal::UnitTestImpl::RunAllTests [0x012E41D2+466]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x012D7117+32]
	testing::UnitTest::Run [0x012E3FAF+136]
	base::TestSuite::Run [0x02388A16+102]
	?Run@?$Invoker@U?$BindState@P6AXXZ$$V@internal@base@@$$A6AXXZ@internal@base@@SAXPAVBindStateBase@23@@Z [0x0121BABB+43]
	base::internal::Invoker<base::internal::BindState<int (__cdecl*)(base::TestSuite *),base::internal::UnretainedWrapper<base::TestSuite> >,int __cdecl(void)>::Run [0x0121BA7E+14]
	base::LaunchUnitTests [0x023859D7+813]
	base::LaunchUnitTests [0x0238567B+86]
	main [0x0121BBE3+75]
	__scrt_common_main_seh [0x0259628A+248] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
	BaseThreadInitThunk [0x76E3338A+18]
	RtlInitializeExceptionChain [0x77939902+99]
	RtlInitializeExceptionChain [0x779398D5+54]


Other builders:
https://ci.chromium.org/buildbot/chromium.clang/CrWinClang%28dbg%29/1463
https://ci.chromium.org/buildbot/chromium.clang/CrWinClang64%28dbg%29/897
https://ci.chromium.org/buildbot/chromium.clang/CrWinClang64%28dll%29/841

It doesn't seem to repro on the release builders, maybe because those don't enable the dcheck that's getting hit?
 

Comment 3 by h...@chromium.org, Feb 15 2018

Verified locally that the test has been failing in this config since it landed.

Comment 4 by h...@chromium.org, Feb 15 2018

The code looks like this:

scoped_refptr<SimpleFontData> CSSFontFaceSource::GetFontData(
    const FontDescription& font_description,
    const FontSelectionCapabilities& font_selection_capabilities) {
  // If the font hasn't loaded or an error occurred, then we've got nothing.
  if (!IsValid())
    return nullptr;

  if (IsLocal()) {
    // We're local. Just return a SimpleFontData from the normal cache.
    return CreateFontData(font_description, font_selection_capabilities);
  }

  FontCacheKey key = font_description.CacheKey(FontFaceCreationParams());

  scoped_refptr<SimpleFontData>& font_data =
      font_data_table_.insert(key, nullptr).stored_value->value; // << ---- 
  if (!font_data)
    font_data = CreateFontData(font_description, font_selection_capabilities);

  font_cache_key_age.PrependOrMoveToFirst(key);
  PruneOldestIfNeeded();

  DCHECK_LE(font_data_table_.size(), kMaxCachedFontData);
  // No release, because fontData is a reference to a RefPtr that is held in the
  // font_data_table_.
  return font_data;
}

I think what's happening is that MSVC extends the lifetime of the object returned by font_data_table_.insert() until the end of the function. When that object, a WTF::HashTableAddResult, destructs after the PruneOldestIfNeeded(), it trips the dcheck.

Comment 5 by h...@chromium.org, Feb 15 2018

Cc: drott@chromium.org e...@chromium.org r...@chromium.org
A potential fix: https://chromium-review.googlesource.com/#/c/chromium/src/+/921942
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 15 2018

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

commit ce5486ae573d41c2d29e1e58308b99355595b0be
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Feb 15 18:43:23 2018

Fix CSSFontFaceSourceTest.UnboundedGrowth test in MSVC builds

The WTF::HashTableAddResult from calling font_data_table_.insert()
was being lifetime-extended in MSVC builds to the end of the function,
causing the HashTableAddResult's destructor to run after the hash table
had been changed, causing a dcheck.

Bug:  812600 
Change-Id: Ifa2b05ef92370c7ba043eea4258f4086274d1871
Reviewed-on: https://chromium-review.googlesource.com/921942
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537085}
[modify] https://crrev.com/ce5486ae573d41c2d29e1e58308b99355595b0be/third_party/WebKit/Source/core/css/CSSFontFaceSource.cpp

Comment 7 by r...@chromium.org, Feb 15 2018

Cc: dpranke@chromium.org
 Issue 799589  has been merged into this issue.

Comment 8 by h...@chromium.org, Feb 16 2018

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 1 2018

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

commit d93525cde8ce7151e584db7bd690a09c06b83715
Author: Reid Kleckner <rnk@google.com>
Date: Thu Mar 01 01:26:11 2018

Fix dangling reference into font data hashtable

It's unlikely that pruning the cache will resize the hash table,
but this makes a copy of the scoped_refptr just in case.

R=drott@chromium.org
BUG= 812600 

Change-Id: I5a6256b9079e9c7f36249cadbb679189f0248afd
Reviewed-on: https://chromium-review.googlesource.com/852531
Reviewed-by: Hans Wennborg <hans@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539970}
[modify] https://crrev.com/d93525cde8ce7151e584db7bd690a09c06b83715/third_party/WebKit/Source/core/css/CSSFontFaceSource.cpp

Sign in to add a comment