Oilpan: Support the HTML parser thread in oilpan

Oilpan: Support the HTML parser thread in oilpan

After r170314, the HTML parser thread allocates on-heap objects such as CSSValues. So this CL supports the HTML parser thread in oilpan. Also this CL marks the CSSValue hierarchy with ThreadAffinity=AnyThread because CSSValue can be allocated by both main thread and parser threads.

BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170564

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170741

[haraken, 2014-03-31 01:26:11]

PTAL.

I need to land https://codereview.chromium.org/198673002/ first.

[Mads Ager (chromium), 2014-03-31 05:52:51]

LGTM

It seems likely that the parser thread is not doing long-running synchronous
tasks but we should make sure that there are enough safe points. Having the
pending gc runner and the message loop interruptor is probably enough.

[tkent, 2014-03-31 06:17:30]

I'm not sure if we should support GC heap in the parser thread yet.
Is a MediaQueryParser object referred by the main thread and kept alive?  Can it
outlive the parser thread?

[Mads Ager (chromium), 2014-03-31 06:40:31]

This code was carefully reviewed with threading in mind:

https://codereview.chromium.org/201813002

So, yes, this is intentional. We need to support this. :)

[wibling-chromium, 2014-03-31 07:52:16]

lgtm

https://codereview.chromium.org/216203006/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLParserThread.cpp:82:
taskSynchronizer->taskCompleted();
Don't you want to clear m_pendingGCRunner and m_messageLoopInterruptor here
given they were allocated in setypHTMLParserThread?

[haraken, 2014-03-31 09:26:21]

https://codereview.chromium.org/216203006/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLParserThread.cpp:82:
taskSynchronizer->taskCompleted();
On 2014/03/31 07:52:17, wibling-chromium wrote:
> Don't you want to clear m_pendingGCRunner and m_messageLoopInterruptor here
> given they were allocated in setypHTMLParserThread?

Done.

[haraken, 2014-03-31 09:27:05]

On 2014/03/31 06:40:31, Mads Ager (chromium) wrote:
> This code was carefully reviewed with threading in mind:
> 
> https://codereview.chromium.org/201813002
> 
> So, yes, this is intentional. We need to support this. :)

tkent-san: Are you OK with supporting the HTML parser thread in oilpan?

[tkent, 2014-03-31 09:41:24]

On 2014/03/31 09:27:05, haraken wrote:
> On 2014/03/31 06:40:31, Mads Ager (chromium) wrote:
> > This code was carefully reviewed with threading in mind:
> > 
> > https://codereview.chromium.org/201813002
> > 
> > So, yes, this is intentional. We need to support this. :)
> 
> tkent-san: Are you OK with supporting the HTML parser thread in oilpan?

What's the answer to my questions?  I have no time to review 201813002.

[haraken, 2014-03-31 09:45:53]

On 2014/03/31 09:41:24, tkent wrote:
> On 2014/03/31 09:27:05, haraken wrote:
> > On 2014/03/31 06:40:31, Mads Ager (chromium) wrote:
> > > This code was carefully reviewed with threading in mind:
> > > 
> > > https://codereview.chromium.org/201813002
> > > 
> > > So, yes, this is intentional. We need to support this. :)
> > 
> > tkent-san: Are you OK with supporting the HTML parser thread in oilpan?
> 
> What's the answer to my questions?  I have no time to review 201813002.

Sorry, here are answers:

- Is a MediaQueryParser object referred by the main thread and kept alive?

No. MediaQueryParser is always stack-allocated, and thus MediaQueryParser
objects allocated in the parser thread are only touched by the parser thread.

- Can it outlive the parser thread?

No, because MediaQueryParser is stack-allocated.

[tkent, 2014-03-31 09:55:28]

On 2014/03/31 09:45:53, haraken wrote:
> No. MediaQueryParser is always stack-allocated, and thus MediaQueryParser
> objects allocated in the parser thread are only touched by the parser thread.
> 
> - Can it outlive the parser thread?
> 
> No, because MediaQueryParser is stack-allocated.

Then, the CL description is incorrect.  What are heap-allocated in the parser
thread, and can they outlive the parser thread?

[Mads Ager (chromium), 2014-03-31 09:59:54]

On 2014/03/31 09:55:28, tkent wrote:
> On 2014/03/31 09:45:53, haraken wrote:
> > No. MediaQueryParser is always stack-allocated, and thus MediaQueryParser
> > objects allocated in the parser thread are only touched by the parser
thread.
> > 
> > - Can it outlive the parser thread?
> > 
> > No, because MediaQueryParser is stack-allocated.
> 
> Then, the CL description is incorrect.  What are heap-allocated in the parser
> thread, and can they outlive the parser thread?

Honestly, I think these questions are for Yoav and I would be fine with asking
Yoav those questions on his code review. Currently, the parser thread
intentionally allocate these objects and this supports that for Oilpan. We can
rip out that support again if a discussion on the original changelist causes
this to change. However, I don't think this codereview is the right place for
these discussions.

[tkent, 2014-03-31 16:02:49]

+yoav

I saw crash logs and https://codereview.chromium.org/201813002/.  It seems
objects allocated in the parser thread won't be passed to other threads. 
However, CSSValue objects are created in the parser thread.  It will break our
ThreadAffinity assumption.

[tkent, 2014-04-01 01:37:34]

haraken, did you confirm this CL fixed all of crashes on Debug Oilpan? Would you
apply this and related CLs locally and check if CSSValue is allocated in the
parser thread please?

[haraken, 2014-04-01 02:19:15]

On 2014/04/01 01:37:34, tkent wrote:
> haraken, did you confirm this CL fixed all of crashes on Debug Oilpan? Would
you
> apply this and related CLs locally and check if CSSValue is allocated in the
> parser thread please?

Good point. http/tests/local/link-stylesheet-load-order.html crashes because the
HTML parser thread allocates CSSPrimitiveValue.

crash log for renderer (pid 9642):
STDOUT: #CRASHED - renderer (pid 9642)
STDERR: ASSERTION FAILED: ThreadState::isMainThread()
STDERR: ../../third_party/WebKit/Source/heap/ThreadState.h(565) : static
WebCore::ThreadState *WebCore::ThreadStateFor<1>::state()
STDERR: 1   0x1f6636a
STDERR: 2   0x1f66d61
STDERR: 3   0x1f65df5
STDERR: 4   0x26094dd
STDERR: 5   0x26622f2
STDERR: 6   0x29a5567
STDERR: 7   0x29a4fe7
STDERR: 8   0x29a5757
STDERR: 9   0x29a46ba
STDERR: 10  0x29a4619
STDERR: 11  0x2657beb
STDERR: 12  0x2457330
STDERR: 13  0x2459189
STDERR: 14  0x245817f
STDERR: 15  0x2457cfa
STDERR: 16  0x2456a2d
STDERR: 17  0x24f0df0
STDERR: 18  0x24f09b5
STDERR: 19  0x24f0817
STDERR: 20  0x24f08fd
STDERR: 21  0x244a77e
STDERR: 22  0x244a6a7
STDERR: 23  0x1940d53
STDERR: 24  0x24566dc
STDERR: 25  0x472aa42
STDERR: 26  0x472a9ac
STDERR: 27  0x472a955
STDERR: 28  0x43a0fe
STDERR: 29  0x75fff8
STDERR: 30  0x76031b
STDERR: 31  0x760545
STDERR: Received signal 11 SEGV_MAPERR 0000fbadbeef
STDERR: #0 0x00000084220e base::debug::StackTrace::StackTrace()
STDERR: #1 0x000000841d47 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR: #2 0x7f23fd957cb0 <unknown>
STDERR: #3 0x000001f66379 WebCore::ThreadStateFor<>::state()
STDERR: #4 0x000001f66d61 WebCore::Heap::allocate<>()
STDERR: #5 0x000001f65df5 WebCore::GarbageCollected<>::operator new()
STDERR: #6 0x0000026094dd WebCore::CSSPrimitiveValue::create()
STDERR: #7 0x0000026622f2 WebCore::MediaQueryExp::createIfValid()
STDERR: #8 0x0000029a5567 WebCore::MediaQueryData::addExpression()
STDERR: #9 0x0000029a4fe7 WebCore::MediaQueryParser::readFeatureEnd()
STDERR: #10 0x0000029a5757 WebCore::MediaQueryParser::processToken()
STDERR: #11 0x0000029a46ba WebCore::MediaQueryParser::parseImpl()
STDERR: #12 0x0000029a4619 WebCore::MediaQueryParser::parse()
STDERR: #13 0x000002657beb WebCore::MediaQuerySet::createOffMainThread()
STDERR: #14 0x000002457330 WebCore::mediaAttributeMatches()
STDERR: #15 0x000002459189
WebCore::TokenPreloadScanner::StartTagScanner::processAttribute<>()
STDERR: #16 0x00000245817f
WebCore::TokenPreloadScanner::StartTagScanner::processAttributes()
STDERR: #17 0x000002457cfa WebCore::TokenPreloadScanner::scanCommon<>()
STDERR: #18 0x000002456a2d WebCore::TokenPreloadScanner::scan()
STDERR: #19 0x0000024f0df0 WebCore::BackgroundHTMLParser::pumpTokenizer()
STDERR: #20 0x0000024f09b5 WebCore::BackgroundHTMLParser::appendDecodedBytes()
STDERR: #21 0x0000024f0817 WebCore::BackgroundHTMLParser::updateDocument()
STDERR: #22 0x0000024f08fd
WebCore::BackgroundHTMLParser::appendRawBytesFromMainThread()
STDERR: #23 0x00000244a77e WTF::FunctionWrapper<>::operator()()
STDERR: #24 0x00000244a6a7 WTF::BoundFunctionImpl<>::operator()()
STDERR: #25 0x000001940d53 WTF::Function<>::operator()()
STDERR: #26 0x0000024566dc WebCore::Task::run()
STDERR: #27 0x00000472aa42 base::internal::RunnableAdapter<>::Run()
STDERR: #28 0x00000472a9ac base::internal::InvokeHelper<>::MakeItSo()
STDERR: #29 0x00000472a955 base::internal::Invoker<>::Run()
STDERR: #30 0x00000043a0fe base::Callback<>::Run()
STDERR: #31 0x00000075fff8 base::MessageLoop::RunTask()
STDERR: #32 0x00000076031b base::MessageLoop::DeferOrRunPendingTask()
STDERR: #33 0x000000760545 base::MessageLoop::DoWork()
STDERR: #34 0x000000773308 base::MessagePumpDefault::Run()
STDERR: #35 0x00000075f937 base::MessageLoop::RunHandler()
STDERR: #36 0x0000007a10a2 base::RunLoop::Run()
STDERR: #37 0x00000075f161 base::MessageLoop::Run()
STDERR: #38 0x000005573709 base::Thread::Run()
STDERR: #39 0x000005573921 base::Thread::ThreadMain()
STDERR: #40 0x0000007d680f base::(anonymous namespace)::ThreadFunc()
STDERR: #41 0x7f23fd94fe9a start_thread


As far as I understand, after r170314, it is expected that the HTML parser
thread allocates CSSValues, so a right fix here would be to add the
ThreadingAffinityTrait to the CSSValue. Although it might regress performance of
CSSValue allocations, it seems unavoidable to me.

What do you think?

[tkent, 2014-04-01 02:28:23]

> As far as I understand, after r170314, it is expected that the HTML parser
> thread allocates CSSValues, so a right fix here would be to add the
> ThreadingAffinityTrait to the CSSValue. Although it might regress performance
of
> CSSValue allocations, it seems unavoidable to me.

I agree that we need to
 - add oilpan support to the HTML parser thread and 
 - change the ThreadAffinity of CSSValue to AnyThread.

[haraken, 2014-04-01 02:44:08]

On 2014/04/01 02:28:23, tkent wrote:
> > As far as I understand, after r170314, it is expected that the HTML parser
> > thread allocates CSSValues, so a right fix here would be to add the
> > ThreadingAffinityTrait to the CSSValue. Although it might regress
performance
> of
> > CSSValue allocations, it seems unavoidable to me.
> 
> I agree that we need to
>  - add oilpan support to the HTML parser thread and 
>  - change the ThreadAffinity of CSSValue to AnyThread.

I verified that the patch set 3 fixes all test crashes after r170314.

[eseidel, 2014-04-01 02:55:21]

Media queries on the parser thread are a very new (last week!) addition.  If
they're the only problem blocking oilpan on the parser thread, we should
re-consider how to do MQ on the parser thread since that code is still settling.
 Yoav is your man for that.

[haraken, 2014-04-01 03:30:50]

On 2014/04/01 02:55:21, eseidel wrote:
> Media queries on the parser thread are a very new (last week!) addition.  If
> they're the only problem blocking oilpan on the parser thread, we should
> re-consider how to do MQ on the parser thread since that code is still
settling.
>  Yoav is your man for that.

I think Media queries on the parser thread is fine.

As we move more things to oilpan, we'll need to support the parser thread in
oilpan at some point. Also there is no disadvantage in supporting the parser
thread in oilpan. So this would be a good opportunity to implement the support.

One thing we need to investigate is the performance overhead of allocating
CSSValues. Given that oilpan cannot assume that CSSValues are used only by the
main thread, oilpan needs to look up thread-local storage when allocating
CSSValues and Persistent<CSSValue>. Thread-local storage in Mac and Win is slow.
However, I think we can worry about that later.

[eseidel, 2014-04-01 03:39:18]

Long-term CSSValue will be replaced by something simpler.  Our current CSSParser
uses the same internal datastructures as are exposed to the DOM.  There is no
need for that to be the case and doing so constricts our ability to make our
parser fast.  If/when we re-write the CSSParser to match the new CSS3 syntax
spec (as yoav has started to do for MQ support) then we may use simpler "token"
type more broadly throghout the CSS code instead of going straight to these
CSSOM wrapper types.

In any case, none of this affects the short-term goals of oilpan.  Just noting
there is a bunch of cruft in this code we'd like to change.

[Mads Ager (chromium), 2014-04-01 05:33:14]

Still, LGTM.

Nice catch on the CSSValue allocations Kent! Luckily we have the asserts to
catch this. Let's get this change landed so we can get back into business on the
oilpan build?

Sounds like a good move to go away from these CSSValue types for most things.
:-)

https://codereview.chromium.org/216203006/diff/50001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/heap/ThreadState....
Source/heap/ThreadState.h:83: template<typename T, bool derivesNodeOrCSSValue =
WTF::IsSubclass<T, Node>::value> struct DefaultThreadingTrait;
Change the name as well? derivesNode.

[tkent, 2014-04-01 05:36:50]

lgtm

[eseidel, 2014-04-01 05:37:59]

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:55:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
s_sharedThread));
not lgtm.  This will cause the parser thread to start immediately when we load
the binary.  No need to do that.

The parser thread isnt' started until ensureThread is called, which is called as
part of postTask.

I'm not sure it matters hugely, but we do have a lazy load design here which
this change is defeating.

[eseidel, 2014-04-01 05:42:11]

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:58: void
HTMLParserThread::setupHTMLParserThread()
Seems we should call this method or post this task from inside ensureThread()
when the thread is lazily created.

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:71:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, &taskSynchronizer));
Why not do this from inside the HTMLParserThread destructor?

I believe we currently depend on WebThread's destructor to join the thread /
wait for termination, but I could be wrong.

Here are hte callsites in case you haven't seen:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[eseidel, 2014-04-01 05:42:49]

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:63: OwnPtr<PendingGCRunner>
m_pendingGCRunner;
Are other threads going to need these?  Should these be built into an object
which wraps a WebThread and Blink threads us instead?

[eseidel, 2014-04-01 05:44:53]

To confirm my suspicion, yes WebThread's destructor does join:

https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webt...

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

[haraken, 2014-04-01 06:31:26]

Thanks for reviewing! Addressed your comments. PTAL.

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:55:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
s_sharedThread));
On 2014/04/01 05:38:00, eseidel wrote:
> not lgtm.  This will cause the parser thread to start immediately when we load
> the binary.  No need to do that.
> 
> The parser thread isnt' started until ensureThread is called, which is called
as
> part of postTask.
> 
> I'm not sure it matters hugely, but we do have a lazy load design here which
> this change is defeating.

Thanks for pointing it out. Done. I moved the postTask into platformThread().

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:58: void
HTMLParserThread::setupHTMLParserThread()
On 2014/04/01 05:42:12, eseidel wrote:
> Seems we should call this method or post this task from inside ensureThread()
> when the thread is lazily created.

Done.

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:71:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, &taskSynchronizer));
On 2014/04/01 05:42:12, eseidel wrote:
> Why not do this from inside the HTMLParserThread destructor?
> 
> I believe we currently depend on WebThread's destructor to join the thread /
> wait for termination, but I could be wrong.
> 
> Here are hte callsites in case you haven't seen:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

Just help me understand: what's the advantage of moving this logic from
shutdown() to the destructor?

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:63: OwnPtr<PendingGCRunner>
m_pendingGCRunner;
On 2014/04/01 05:42:50, eseidel wrote:
> Are other threads going to need these?  Should these be built into an object
> which wraps a WebThread and Blink threads us instead?

Yeah, this is a good point. We've implemented a similar oilpan support for the
database thread (which is a WebThread).

As you mentioned, it might be better to create a thin wrapper for a WebThread
and use the wrapper in these threads. Would it be OK to address the issue in a
follow-up CL?

https://codereview.chromium.org/216203006/diff/50001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/heap/ThreadState....
Source/heap/ThreadState.h:83: template<typename T, bool derivesNodeOrCSSValue =
WTF::IsSubclass<T, Node>::value> struct DefaultThreadingTrait;
On 2014/04/01 05:33:15, Mads Ager (chromium) wrote:
> Change the name as well? derivesNode.

Done.

[oystein (OOO til 10th of July), 2014-04-01 06:40:10]

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:71:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, &taskSynchronizer));
Maybe add a check here to see if the thread has actually been started
previously? If not I suspect we'll redundantly start it at this point.

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:36: #include
"public/platform/WebThread.h"
nit: Already included below.

[haraken, 2014-04-01 06:47:54]

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:71:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, &taskSynchronizer));
On 2014/04/01 06:40:10, Oystein wrote:
> Maybe add a check here to see if the thread has actually been started
> previously? If not I suspect we'll redundantly start it at this point.

Done.

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/216203006/diff/50001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:36: #include
"public/platform/WebThread.h"
On 2014/04/01 06:40:10, Oystein wrote:
> nit: Already included below.

Done.

[eseidel, 2014-04-01 06:56:58]

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:53: delete s_sharedThread;
I have lead you astray.  This can't work.  s_sharedThread is an
HTMLParserThread?  How can this work?

[eseidel, 2014-04-01 06:57:51]

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:52:
taskSynchronizer.waitForTaskCompletion();
Do you need to wait for task completion, or will the WebThread automatically
fush its message queue before joining?

[eseidel, 2014-04-01 06:58:32]

I agree that leaving a gc'd thread wrapper for a later change is a good idea.

[eseidel, 2014-04-01 07:09:50]

i'm headed to sleep.  don't let my previous not l-g hold this patch up.  I'll
look again in the morning, you have plenty of good reviewers awake while I'm
not. :)

[haraken, 2014-04-01 07:36:00]

Thanks for reviewing, Eric. PTAL.

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:52:
taskSynchronizer.waitForTaskCompletion();
On 2014/04/01 06:57:52, eseidel wrote:
> Do you need to wait for task completion, or will the WebThread automatically
> fush its message queue before joining?

I need to wait for the task completion to make sure that we destruct the thread
instance _after_ cleaning up oilpan things. Since we need to clean up oilpan
things in the parser thread, we need to synchronization between the parser
thread and the main thread.

https://codereview.chromium.org/216203006/diff/70001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:53: delete s_sharedThread;
On 2014/04/01 06:56:59, eseidel wrote:
> I have lead you astray.  This can't work.  s_sharedThread is an
> HTMLParserThread?  How can this work?

Ah, good catch. I moved the logic back to shutdown().

[eseidel, 2014-04-01 08:03:32]

lgtm

I think this is OK for landing.  I think we're still conflating some concepts of
the class vs. the singleton.  Ideally the class should be stand-alone, and not
know anything about the fact that there is only one instance of it.

We can do the splitting of the GarbageCollectedThread out into a super class or
into just a helper object which thred classes hold and wraps WebThread, in
either case that can take care of all this setup and teardown in a way such that
the rest of the HTML parser bits don't need to know much about it.

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:70: TaskSynchronizer
taskSynchronizer;
These 3 lines may end up in the GarbageCollectedThread destrutor or whatever we
end up writing.

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:96:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
s_sharedThread));
Why s_sharedThread?  Why not just "this"?

[wibling-chromium, 2014-04-01 09:25:06]

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:68: ASSERT(m_thread);
Can you assert m_thread in a static member function?

[haraken, 2014-04-01 10:59:17]

Now all dependent CLs are landed, so I'm landing this CL.

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:68: ASSERT(m_thread);
On 2014/04/01 09:25:07, wibling-chromium wrote:
> Can you assert m_thread in a static member function?

Removed. This was a left-over from a previous CL.

https://codereview.chromium.org/216203006/diff/90001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.cpp:96:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
s_sharedThread));
On 2014/04/01 08:03:33, eseidel wrote:
> Why s_sharedThread?  Why not just "this"?

Done.

[haraken, 2014-04-01 10:59:22]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-01 10:59:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/100001

[wibling-chromium, 2014-04-01 11:13:01]

lgtm

[commit-bot: I haz the power, 2014-04-01 11:37:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:37:55]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[haraken, 2014-04-01 11:42:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:42:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/100001

[commit-bot: I haz the power, 2014-04-01 11:49:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:49:02]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[haraken, 2014-04-01 11:50:49]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:50:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/120001

[commit-bot: I haz the power, 2014-04-01 12:19:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 12:19:13]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[tkent, 2014-04-01 12:39:03]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-04-01 12:39:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/120001

[commit-bot: I haz the power, 2014-04-01 13:32:11]

Change committed as 170564

[tkent, 2014-04-02 01:44:04]

https://codereview.chromium.org/216203006/diff/120001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/120001/Source/core/html/parser...
Source/core/html/parser/HTMLParserThread.cpp:82: m_pendingGCRunner = nullptr;
This can be use-after-free. Calling taskCompleted can delete |this| immediately
in HTMLParserThread::shutdown().

[haraken, 2014-04-02 02:02:19]

https://codereview.chromium.org/216203006/diff/120001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/120001/Source/core/html/parser...
Source/core/html/parser/HTMLParserThread.cpp:82: m_pendingGCRunner = nullptr;
On 2014/04/02 01:44:04, tkent wrote:
> This can be use-after-free. Calling taskCompleted can delete |this|
immediately
> in HTMLParserThread::shutdown().
> 

Nice catch. The solution would be just to move line 81 down to below line 83,
right?

[haraken, 2014-04-02 02:18:22]

Let me summarize the current situation: Unfortunately I had to revert all
changes about the HTML parser thread because it broke a lot of things.

(1) r170536 broke webkit_tests in Android bots
(http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Android%2...).
Since r170536 just moved TaskSynchronizer from modules/ to platform/, I don't
understand why it caused crash/hang in webkit_tests.

(2) r170564 caused hangs in almost all content_unittests in all bots
(http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%2...).
I don't yet know what's going on, but I can reproduce the issue locally and am
investigating.

[haraken, 2014-04-02 05:16:26]

I minimized the CL to reproduce the crash in content_unittests. Now I'm trying
to somehow get a stack trace of the crash, but do you have any idea of what's
going on there?

Any comments are appreciated.

https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLParserThread.cpp:70:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, taskSynchronizer));

- This line causes a crash in almost all content_unittests. I'm not succeeding
in getting stack trace of the crash.

- The crash does not happen in layout tests.

- If I comment out this line, the crash in content_unittests disappears.

[tkent, 2014-04-02 05:33:34]

https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLParserThread.cpp:70:
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
s_sharedThread, taskSynchronizer));
On 2014/04/02 05:16:27, haraken wrote:
> 
> - This line causes a crash in almost all content_unittests. I'm not succeeding
> in getting stack trace of the crash.
> 
> - The crash does not happen in layout tests.
> 
> - If I comment out this line, the crash in content_unittests disappears.

See WebKit.cpp.  We don't setup Oilpan heap of the main thread if it's in unit
tests. cleanupHTMLParserThread will be deadlocked in such case.

[haraken, 2014-04-02 05:57:22]

On 2014/04/02 05:33:34, tkent wrote:
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> File Source/core/html/parser/HTMLParserThread.cpp (right):
> 
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> Source/core/html/parser/HTMLParserThread.cpp:70:
> s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
> s_sharedThread, taskSynchronizer));
> On 2014/04/02 05:16:27, haraken wrote:
> > 
> > - This line causes a crash in almost all content_unittests. I'm not
succeeding
> > in getting stack trace of the crash.
> > 
> > - The crash does not happen in layout tests.
> > 
> > - If I comment out this line, the crash in content_unittests disappears.
> 
> See WebKit.cpp.  We don't setup Oilpan heap of the main thread if it's in unit
> tests. cleanupHTMLParserThread will be deadlocked in such case.

Thanks for pointing it out! It looks like that is the cause of the crash.

I don't still understand the following behavior, but either way it seems that
the crash happens when oilpan is not set up on the main thread.

- setHTMLParserThread doesn't cause the crash, but cleanupHTMLParserThread does.

- "./out/Debug/content_unittests" doesn't cause the crash, but
"./out/Debug/content_unittests --brave-new-test-launcher
--test-launcher-bot-mode" does.


So how can we fix the issue? In the current WebKit.cpp, we fail to set up oilpan
on the main thread when Platform::current()->currentThread() returns 0. What can
we do when currentThread() returns 0? Or do we need to guarantee that
currentThread() does not return 0 when executing unit tests?

[Mads Ager (chromium), 2014-04-02 06:23:25]

On 2014/04/02 05:57:22, haraken wrote:
> On 2014/04/02 05:33:34, tkent wrote:
> >
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> > File Source/core/html/parser/HTMLParserThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> > Source/core/html/parser/HTMLParserThread.cpp:70:
> >
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
> > s_sharedThread, taskSynchronizer));
> > On 2014/04/02 05:16:27, haraken wrote:
> > > 
> > > - This line causes a crash in almost all content_unittests. I'm not
> succeeding
> > > in getting stack trace of the crash.
> > > 
> > > - The crash does not happen in layout tests.
> > > 
> > > - If I comment out this line, the crash in content_unittests disappears.
> > 
> > See WebKit.cpp.  We don't setup Oilpan heap of the main thread if it's in
unit
> > tests. cleanupHTMLParserThread will be deadlocked in such case.
> 
> Thanks for pointing it out! It looks like that is the cause of the crash.
> 
> I don't still understand the following behavior, but either way it seems that
> the crash happens when oilpan is not set up on the main thread.
> 
> - setHTMLParserThread doesn't cause the crash, but cleanupHTMLParserThread
does.
> 
> - "./out/Debug/content_unittests" doesn't cause the crash, but
> "./out/Debug/content_unittests --brave-new-test-launcher
> --test-launcher-bot-mode" does.
> 
> 
> So how can we fix the issue? In the current WebKit.cpp, we fail to set up
oilpan
> on the main thread when Platform::current()->currentThread() returns 0. What
can
> we do when currentThread() returns 0? Or do we need to guarantee that
> currentThread() does not return 0 when executing unit tests?

It would be nice if that split didn't exist. It is a bit strange to have tests
that use threads run in a setting where the main thread is a different type of
thread. However, that might not be easy to fix.

The other option would be to attempt to integrate with the unit test setup and
figure out how that works. Is there an event loop that we can interact with
there? That is what we did on the branch. We manually put in safe points in
various places in the unittest setup. We were pretty happy that we didn't have
the need for that when moving back to trunk. Maybe now we do again... :(

[Mads Ager (chromium), 2014-04-02 06:24:00]

On 2014/04/02 06:23:25, Mads Ager (chromium) wrote:
> On 2014/04/02 05:57:22, haraken wrote:
> > On 2014/04/02 05:33:34, tkent wrote:
> > >
> >
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> > > File Source/core/html/parser/HTMLParserThread.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/216203006/diff/140001/Source/core/html/parser...
> > > Source/core/html/parser/HTMLParserThread.cpp:70:
> > >
> s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
> > > s_sharedThread, taskSynchronizer));
> > > On 2014/04/02 05:16:27, haraken wrote:
> > > > 
> > > > - This line causes a crash in almost all content_unittests. I'm not
> > succeeding
> > > > in getting stack trace of the crash.
> > > > 
> > > > - The crash does not happen in layout tests.
> > > > 
> > > > - If I comment out this line, the crash in content_unittests disappears.
> > > 
> > > See WebKit.cpp.  We don't setup Oilpan heap of the main thread if it's in
> unit
> > > tests. cleanupHTMLParserThread will be deadlocked in such case.
> > 
> > Thanks for pointing it out! It looks like that is the cause of the crash.
> > 
> > I don't still understand the following behavior, but either way it seems
that
> > the crash happens when oilpan is not set up on the main thread.
> > 
> > - setHTMLParserThread doesn't cause the crash, but cleanupHTMLParserThread
> does.
> > 
> > - "./out/Debug/content_unittests" doesn't cause the crash, but
> > "./out/Debug/content_unittests --brave-new-test-launcher
> > --test-launcher-bot-mode" does.
> > 
> > 
> > So how can we fix the issue? In the current WebKit.cpp, we fail to set up
> oilpan
> > on the main thread when Platform::current()->currentThread() returns 0. What
> can
> > we do when currentThread() returns 0? Or do we need to guarantee that
> > currentThread() does not return 0 when executing unit tests?
> 
> It would be nice if that split didn't exist. It is a bit strange to have tests
> that use threads run in a setting where the main thread is a different type of
> thread. However, that might not be easy to fix.

different type of thread than in the shipping system, that is.

[tkent, 2014-04-02 06:26:50]

On 2014/04/02 05:57:22, haraken wrote:
> So how can we fix the issue? In the current WebKit.cpp, we fail to set up
oilpan
> on the main thread when Platform::current()->currentThread() returns 0. What
can
> we do when currentThread() returns 0? Or do we need to guarantee that
> currentThread() does not return 0 when executing unit tests?

I feel the right fix is to make Platform::current()->currentThread() available.
But we haven't done it for years and I guess it's very hard.
So, I recommend to avoid posting the shutdown task if
Platform::current()->currentThread()  is 0, just like WebKit.cpp.

[eseidel, 2014-04-02 06:30:23]

(Incidentally, the whole reason why the HTML parser has shutdown code at all is
to make sure it's dead before Platform::current(), as we got reports of
renderers crashing on teardown from the wild.)

[haraken, 2014-04-02 08:05:58]

On 2014/04/02 06:26:50, tkent wrote:
> On 2014/04/02 05:57:22, haraken wrote:
> > So how can we fix the issue? In the current WebKit.cpp, we fail to set up
> oilpan
> > on the main thread when Platform::current()->currentThread() returns 0. What
> can
> > we do when currentThread() returns 0? Or do we need to guarantee that
> > currentThread() does not return 0 when executing unit tests?
> 
> I feel the right fix is to make Platform::current()->currentThread()
available.
> But we haven't done it for years and I guess it's very hard.
> So, I recommend to avoid posting the shutdown task if
> Platform::current()->currentThread()  is 0, just like WebKit.cpp.

Discussed offline, the ideasounds reasonable. Fixed as you suggested. PTAL.

[tkent, 2014-04-02 08:09:47]

https://codereview.chromium.org/216203006/diff/160001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserThread.cpp (right):

https://codereview.chromium.org/216203006/diff/160001/Source/core/html/parser...
Source/core/html/parser/HTMLParserThread.cpp:74: delete s_sharedThread;
We should delete s_sharedThread even if currentThread() is 0 because we created
it unconditionally in HTMLParserThread::init().

[haraken, 2014-04-02 08:11:45]

Thanks for review.

>
https://codereview.chromium.org/216203006/diff/160001/Source/core/html/parser...
> File Source/core/html/parser/HTMLParserThread.cpp (right):
> 
>
https://codereview.chromium.org/216203006/diff/160001/Source/core/html/parser...
> Source/core/html/parser/HTMLParserThread.cpp:74: delete s_sharedThread;
> We should delete s_sharedThread even if currentThread() is 0 because we
created
> it unconditionally in HTMLParserThread::init().

Done.

[tkent, 2014-04-02 08:18:11]

lgtm

[haraken, 2014-04-02 08:23:36]

On 2014/04/02 08:18:11, tkent wrote:
> lgtm

Thanks. Now the only remaining issue is that
https://codereview.chromium.org/198673002/ broke webkit_unit_tests in the
Android Nexus4 bot. I'll comment the details on
https://codereview.chromium.org/198673002/.

[haraken, 2014-04-03 03:59:06]

On 2014/04/02 08:23:36, haraken wrote:
> On 2014/04/02 08:18:11, tkent wrote:
> > lgtm
> 
> Thanks. Now the only remaining issue is that
> https://codereview.chromium.org/198673002/ broke webkit_unit_tests in the
> Android Nexus4 bot. I'll comment the details on
> https://codereview.chromium.org/198673002/.

The Android bot issue is gone, and I think now it's safe to land this CL.

[haraken, 2014-04-03 03:59:14]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-03 03:59:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/180001

[commit-bot: I haz the power, 2014-04-03 05:11:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 05:11:23]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[haraken, 2014-04-03 05:21:17]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-03 05:21:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216203006/180001

[commit-bot: I haz the power, 2014-04-03 06:28:51]

Change committed as 170741