Bug 63127 – DatabaseTask synchronization error

• Component: New Bugs
• Status: RESOLVED FIXED

[Dmitry Vyukov]

The problem is with
Source/WebCore/storage/DatabaseTask.cpp
void DatabaseTaskSynchronizer::waitForTaskCompletion()
{
    m_synchronousMutex.lock();
    if (!m_taskCompleted)
        m_synchronousCondition.wait(m_synchronousMutex);
    m_synchronousMutex.unlock();
}

Since POSIX condition variables are amenable to spurious wake-ups, waits must always be expressed as loops. Current implementation leads to premature returns from waitForTaskCompletion() and subsequent bad memory/stack corruptions. I observe crashes like:

Program terminated with signal 11, Segmentation fault.
#0  0x0000000002452d10 in WTF::RefPtr<WebCore::CSSRuleList>::RefPtr<WebCore::CSSRuleList> (this=0x0, o=<value optimized out>) at third_party/WebKit/Source/JavaScriptCore/wtf/RefPtr.h:94
94	        : m_ptr(o.leakRef())
(gdb) bt
#0  0x0000000002452d10 in WTF::RefPtr<WebCore::CSSRuleList>::RefPtr<WebCore::CSSRuleList> (this=0x0, o=<value optimized out>) at third_party/WebKit/Source/JavaScriptCore/wtf/RefPtr.h:94
#1  0x000000000283edd1 in append<WTF::PassRefPtr<WebCore::CSSRuleList> > (this=0x7fff75788480) at third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:992
#2  WebCore::CSSParser::createRuleList (this=0x7fff75788480) at third_party/WebKit/Source/WebCore/css/CSSParser.cpp:6316
#3  0x0000000003d3aebe in cssyyparse (parser=0x7fff75788480) at /usr/local/google/home/dvyukov/chrome/src/third_party/WebKit/Source/WebCore/css/CSSGrammar.y:430
#4  0x000000000282aa4d in WebCore::CSSParser::parseSheet (this=0x7fff75788480, sheet=0x969efe0, string=..., startLineNumber=0, ruleRangeMap=0x0) at third_party/WebKit/Source/WebCore/css/CSSParser.cpp:266
#5  0x00000000028b4c74 in WebCore::CSSStyleSheet::parseStringAtLine (this=0x969efe0, string=..., strict=true, startLineNumber=0) at third_party/WebKit/Source/WebCore/css/CSSStyleSheet.cpp:204
#6  0x0000000003ec23ff in WebCore::StyleElement::createSheet (this=0x7f765d52f868, e=0x7f765d52f7f0, startLineNumber=0, text=...) at third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:157
#7  0x0000000003ec28d8 in WebCore::StyleElement::process (this=0x7f765d52f868, e=0x7f765d52f7f0) at third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:133
#8  0x0000000003ec2ac8 in WebCore::StyleElement::insertedIntoDocument (this=0x7f765d52f868, document=0x6883310, element=0x7f765d52f7f0) at third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:67
#9  0x0000000003d13fd5 in WebCore::HTMLStyleElement::insertedIntoDocument (this=0x7f765d52f7f0) at third_party/WebKit/Source/WebCore/html/HTMLStyleElement.cpp:75
#10 0x00000000028e8fa0 in WebCore::notifyChildInserted (child=0xb5d52f7f0) at third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:1066
#11 0x00000000028eba98 in WebCore::ContainerNode::appendChild (this=0x6892900, newChild=<value optimized out>, ec=@0x7fff757889ac, shouldLazyAttach=true) at third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:645
#12 0x00000000029725de in WebCore::Node::appendChild (this=0x6892900, newChild=..., ec=@0x7fff757889ac, shouldLazyAttach=true) at third_party/WebKit/Source/WebCore/dom/Node.cpp:668
#13 0x00000000027d36a8 in WebCore::V8Node::appendChildCallback (args=...) at third_party/WebKit/Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:124
#14 0x0000000001bb55c3 in v8::internal::HandleApiCallHelper<false> (args=..., isolate=0x67da280) at v8/src/builtins.cc:1105
#15 0x0000000001bb5745 in v8::internal::Builtin_HandleApiCall (args=..., isolate=0x67da280) at v8/src/builtins.cc:1122

There are a lot of similar crashes in the crash database, but due to the nature of the bug it's difficult to match them.
In order to fix it, replace 'if' with 'while'.

Btw, the bug is found with GCC-ThreadSanitizer which forces such bugs to occur more frequently. Below are usage instructions for GCC-ThreadSanitizer if you are interested:
http://code.google.com/p/data-race-test/wiki/GccInstrumentation

[Dmitry Vyukov]

Created attachment 98151 [details]
Proposed patch

[Dmitry Lomov]

(In reply to comment #1)
> Created an attachment (id=98151) [details]
> Proposed patch

Patch looks good to me.

[David Levin]

Comment on attachment 98151 [details]
Proposed patch

Nice fix.

This needs a changelog before it can go in. Please read the info at http://www.webkit.org/coding/contributing.html#changelogs

Thanks!

[Dmitry Vyukov]

Created attachment 98322 [details]
Proposed patch

[Dmitry Vyukov]

PTAL

[David Levin]

Comment on attachment 98322 [details]
Proposed patch

Actually a statement about how it isn't testable (or how it is covered by existing tests) would have been better for the ChangeLog, but ok.

Thanks!

[WebKit Review Bot]

Comment on attachment 98322 [details]
Proposed patch

Clearing flags on attachment: 98322

Committed r89544: <http://trac.webkit.org/changeset/89544>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.