Bug 175010 – Use LazyNeverDestroyed instead of DEFINE_GLOBAL for QualifiedName

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fujii Hironori]

Use LazyNeverDestroyed instead of DEFINE_GLOBAL for QualifiedName

This is a sub-task of Bug 174979.

[Fujii Hironori]

Created attachment 316832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316832 [details]
Patch

Clearing flags on attachment: 316832

Committed r220089: <http://trac.webkit.org/changeset/220089>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33647818>

[Matt Lewis]

Created attachment 316877 [details]
Sample crash log

This patch:
https://trac.webkit.org/changeset/220089/webkit

caused multiple crashes in macOS Debug:
  http/tests/contentextensions/css-display-none-overflows-rule-data-1.html [ Crash ]
  http/tests/contentextensions/css-display-none-overflows-rule-data-2.html [ Crash ]
  http/tests/contentextensions/css-display-none-overflows-rule-data-3.html [ Crash ]
  http/tests/contentextensions/css-display-none-overflows-rule-data-4.html [ Crash ]
  http/tests/contentextensions/css-display-none-overflows-rule-data-5.html [ Crash ]
  http/tests/contentextensions/css-display-none-overflows-rule-data-6.html [ Crash ]

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/2297
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r220095%20(2297)/results.html

sample history:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcontentextensions%2Fcss-display-none-overflows-rule-data-1.html

[Matt Lewis]

Reverted r220089 for reason:

This caused multiple crashes on macOS Debug testers.

Committed r220106: <http://trac.webkit.org/changeset/220106>

[Matt Lewis]

This also caused an API Test to crash:
UNEXPECTEDLY EXITED ContentExtensionTest.ValidSelector
ASSERTION FAILED: m_isConstructed
/Volumes/Data/slave/elcapitan-debug/build/WebKitBuild/Debug/usr/local/include/wtf/NeverDestroyed.h(114) : PointerType WTF::LazyNeverDestroyed<const WebCore::QualifiedName>::storagePointer() const [T = const WebCore::QualifiedName]
1   0x10f2c4330 WTFCrash
2   0x1169cc476 WTF::LazyNeverDestroyed<WebCore::QualifiedName const>::storagePointer() const
3   0x1169cc425 WTF::LazyNeverDestroyed<WebCore::QualifiedName const>::operator WebCore::QualifiedName const&()
4   0x1169cc0e0 WebCore::anyQName()
5   0x1169caafd WebCore::CSSSelector::RareData::RareData(WTF::AtomicString&&)
6   0x1169cac5d WebCore::CSSSelector::RareData::RareData(WTF::AtomicString&&)
7   0x1169cb6ff WebCore::CSSSelector::RareData::create(WTF::AtomicString&&)
8   0x1169c7c86 WebCore::CSSSelector::createRareData()
9   0x1169c9c9a WebCore::CSSSelector::setAttribute(WebCore::QualifiedName const&, bool, WebCore::CSSSelector::AttributeMatchType)
10  0x1169dc1ed WebCore::CSSParserSelector::setAttribute(WebCore::QualifiedName const&, bool, WebCore::CSSSelector::AttributeMatchType)
11  0x1169d5884 WebCore::CSSSelectorParser::consumeAttribute(WebCore::CSSParserTokenRange&)
12  0x1169d2752 WebCore::CSSSelectorParser::consumeSimpleSelector(WebCore::CSSParserTokenRange&)
13  0x1169d03ac WebCore::CSSSelectorParser::consumeCompoundSelector(WebCore::CSSParserTokenRange&)
14  0x1169ce982 WebCore::CSSSelectorParser::consumeComplexSelector(WebCore::CSSParserTokenRange&)
15  0x1169ce321 WebCore::CSSSelectorParser::consumeComplexSelectorList(WebCore::CSSParserTokenRange&)
16  0x1169ce231 WebCore::CSSSelectorParser::parseSelector(WebCore::CSSParserTokenRange, WebCore::CSSParserContext const&, WebCore::StyleSheetContents*)
17  0x1169264fe WebCore::CSSParser::parseSelector(WTF::String const&, WebCore::CSSSelectorList&)
18  0x116751a0b WebCore::ContentExtensions::isValidCSSSelector(WTF::String const&)
19  0x10d55ec08 TestWebKitAPI::ContentExtensionTest_ValidSelector_Test::TestBody()
20  0x10d85208a testing::Test::Run()
21  0x10d85296d testing::internal::TestInfoImpl::Run()
22  0x10d85350d testing::TestCase::Run()
23  0x10d859492 testing::internal::UnitTestImpl::RunAllTests()
24  0x10d859109 testing::UnitTest::Run()
25  0x10d741d0e TestWebKitAPI::TestsController::run(int, char**)
26  0x10d824762 main
27  0x7fff8db0d5ad start
28  0x2

[Yusuke Suzuki]

(In reply to Matt Lewis from comment #7)
> This also caused an API Test to crash:
> UNEXPECTEDLY EXITED ContentExtensionTest.ValidSelector
> ASSERTION FAILED: m_isConstructed
> /Volumes/Data/slave/elcapitan-debug/build/WebKitBuild/Debug/usr/local/
> include/wtf/NeverDestroyed.h(114) : PointerType
> WTF::LazyNeverDestroyed<const WebCore::QualifiedName>::storagePointer()
> const [T = const WebCore::QualifiedName]
> 1   0x10f2c4330 WTFCrash
> 2   0x1169cc476 WTF::LazyNeverDestroyed<WebCore::QualifiedName
> const>::storagePointer() const
> 3   0x1169cc425 WTF::LazyNeverDestroyed<WebCore::QualifiedName
> const>::operator WebCore::QualifiedName const&()
> 4   0x1169cc0e0 WebCore::anyQName()
> 5   0x1169caafd WebCore::CSSSelector::RareData::RareData(WTF::AtomicString&&)
> 6   0x1169cac5d WebCore::CSSSelector::RareData::RareData(WTF::AtomicString&&)
> 7   0x1169cb6ff WebCore::CSSSelector::RareData::create(WTF::AtomicString&&)
> 8   0x1169c7c86 WebCore::CSSSelector::createRareData()
> 9   0x1169c9c9a WebCore::CSSSelector::setAttribute(WebCore::QualifiedName
> const&, bool, WebCore::CSSSelector::AttributeMatchType)
> 10  0x1169dc1ed
> WebCore::CSSParserSelector::setAttribute(WebCore::QualifiedName const&,
> bool, WebCore::CSSSelector::AttributeMatchType)
> 11  0x1169d5884
> WebCore::CSSSelectorParser::consumeAttribute(WebCore::CSSParserTokenRange&)
> 12  0x1169d2752
> WebCore::CSSSelectorParser::consumeSimpleSelector(WebCore::
> CSSParserTokenRange&)
> 13  0x1169d03ac
> WebCore::CSSSelectorParser::consumeCompoundSelector(WebCore::
> CSSParserTokenRange&)
> 14  0x1169ce982
> WebCore::CSSSelectorParser::consumeComplexSelector(WebCore::
> CSSParserTokenRange&)
> 15  0x1169ce321
> WebCore::CSSSelectorParser::consumeComplexSelectorList(WebCore::
> CSSParserTokenRange&)
> 16  0x1169ce231
> WebCore::CSSSelectorParser::parseSelector(WebCore::CSSParserTokenRange,
> WebCore::CSSParserContext const&, WebCore::StyleSheetContents*)
> 17  0x1169264fe WebCore::CSSParser::parseSelector(WTF::String const&,
> WebCore::CSSSelectorList&)
> 18  0x116751a0b WebCore::ContentExtensions::isValidCSSSelector(WTF::String
> const&)
> 19  0x10d55ec08
> TestWebKitAPI::ContentExtensionTest_ValidSelector_Test::TestBody()
> 20  0x10d85208a testing::Test::Run()
> 21  0x10d85296d testing::internal::TestInfoImpl::Run()
> 22  0x10d85350d testing::TestCase::Run()
> 23  0x10d859492 testing::internal::UnitTestImpl::RunAllTests()
> 24  0x10d859109 testing::UnitTest::Run()
> 25  0x10d741d0e TestWebKitAPI::TestsController::run(int, char**)
> 26  0x10d824762 main
> 27  0x7fff8db0d5ad start
> 28  0x2

Cool, I think this change figures out the existing bug! (We used this value without initializing it)

[Fujii Hironori]

Thank you for rolling back my patch, Matt.

Yes, they are bugs of WebKitTestRunner and WebKitTestAPI.
I'll update a new patch soon after testing on my mac.

[Fujii Hironori]

Created attachment 316933 [details]
Patch

[Alex Christensen]

Comment on attachment 316933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316933&action=review

> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:431
>      AtomicString::init();
> +    WebCore::QualifiedName::init();

This could be a bad binary compatibility issue to add a new requirement to call QualifiedName::init where we didn't have to before.

[WebKit Commit Bot]

Comment on attachment 316933 [details]
Patch

Clearing flags on attachment: 316933

Committed r220131: <http://trac.webkit.org/changeset/220131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

(In reply to Alex Christensen from comment #11)
> This could be a bad binary compatibility issue to add a new requirement to
> call QualifiedName::init where we didn't have to before.

IIUC, this is not a binary compatibility issue.
This is a uninitialized variables problem in release build,
and assertion failures in debug build.

IMHO, WebKit should have a API to initialize statics. But, it's too late.
WebKit need to have some init() calls at all entry points like Frame::Frame():
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Frame.cpp?rev=220159#L174