Revert "Oilpan: Support the HTML parser thread in oilpan"

Revert "Oilpan: Support the HTML parser thread in oilpan"

Speculative rollout of r170564 to see if unit_tests and content_unittests start working again.

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

[rafaelw, 2014-04-01 21:27:44]

Committed patchset #2 manually as r170602 (tree was closed).

[rafaelw1, 2014-04-01 22:29:28]

Ok, so this caused pretty extensive problems in the blink builders. It
seems that it confused the test harnesses so that they started thinking
that basically all of the tests failed.

Dirk has some ideas of how this may have happened, but Kentaro, if you
attempt to land this again, please

a) try to repro this problem locally and find a solution before landing
b) alert the gardener at the time that the change is going in so they can
not spend as much time as we did trying to figure out what had happened.




On Tue, Apr 1, 2014 at 2:27 PM, <rafaelw@chromium.org> wrote:

> Reviewers: haraken, dpranke,
>
> Message:
> Committed patchset #2 manually as r170602 (tree was closed).
>
> Description:
> Revert "Oilpan: Support the HTML parser thread in oilpan"
>
> Speculative rollout of r170564 to see if unit_tests and content_unittests
> start
> working again.
>
> TBR=haraken
> BUG=340522
>
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170602
>
> Please review this at https://codereview.chromium.org/219243014/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+3, -36 lines):
>   M Source/core/html/parser/HTMLParserThread.h
>   M Source/core/html/parser/HTMLParserThread.cpp
>   M Source/heap/ThreadState.h
>
>
> Index: Source/core/html/parser/HTMLParserThread.cpp
> diff --git a/Source/core/html/parser/HTMLParserThread.cpp
> b/Source/core/html/parser/HTMLParserThread.cpp
> index 0f1f0d4b27e2ed6038961482efafdd6e02e64337..
> fe033fbd8d9d02b0b47d254b75d5eca89c041310 100644
> --- a/Source/core/html/parser/HTMLParserThread.cpp
> +++ b/Source/core/html/parser/HTMLParserThread.cpp
> @@ -32,7 +32,6 @@
>  #include "core/html/parser/HTMLParserThread.h"
>
>  #include "platform/Task.h"
> -#include "platform/TaskSynchronizer.h"
>  #include "public/platform/Platform.h"
>  #include "wtf/PassOwnPtr.h"
>
> @@ -54,35 +53,13 @@ void HTMLParserThread::init()
>      s_sharedThread = new HTMLParserThread;
>  }
>
> -void HTMLParserThread::setupHTMLParserThread()
> -{
> -    m_pendingGCRunner = adoptPtr(new PendingGCRunner);
> -    m_messageLoopInterruptor = adoptPtr(new MessageLoopInterruptor(&
> platformThread()));
> -    platformThread().addTaskObserver(m_pendingGCRunner.get());
> -    ThreadState::attach();
> -    ThreadState::current()->addInterruptor(m_
> messageLoopInterruptor.get());
> -}
> -
>  void HTMLParserThread::shutdown()
>  {
>      ASSERT(s_sharedThread);
> -    TaskSynchronizer taskSynchronizer;
> -   
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
> s_sharedThread, &taskSynchronizer));
> -    taskSynchronizer.waitForTaskCompletion();
>      delete s_sharedThread;
>      s_sharedThread = 0;
>  }
>
> -void HTMLParserThread::cleanupHTMLParserThread(TaskSynchronizer*
> taskSynchronizer)
> -{
> -    ThreadState::current()->removeInterruptor(m_
> messageLoopInterruptor.get());
> -    ThreadState::detach();
> -    platformThread().removeTaskObserver(m_pendingGCRunner.get());
> -    taskSynchronizer->taskCompleted();
> -    m_pendingGCRunner = nullptr;
> -    m_messageLoopInterruptor = nullptr;
> -}
> -
>  HTMLParserThread* HTMLParserThread::shared()
>  {
>      return s_sharedThread;
> @@ -90,10 +67,8 @@ HTMLParserThread* HTMLParserThread::shared()
>
>  blink::WebThread& HTMLParserThread::platformThread()
>  {
> -    if (!m_thread) {
> +    if (!m_thread)
>          m_thread = adoptPtr(blink::Platform::current()->createThread("
> HTMLParserThread"));
> -        postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
> this));
> -    }
>      return *m_thread;
>  }
>
> Index: Source/core/html/parser/HTMLParserThread.h
> diff --git a/Source/core/html/parser/HTMLParserThread.h
> b/Source/core/html/parser/HTMLParserThread.h
> index 1421e0dacc5dc236a406d9a7666552cf9dc86f13..
> 1feb999166443ea7d96874bbd24895acc56a7c6b 100644
> --- a/Source/core/html/parser/HTMLParserThread.h
> +++ b/Source/core/html/parser/HTMLParserThread.h
> @@ -31,16 +31,12 @@
>  #ifndef HTMLParserThread_h
>  #define HTMLParserThread_h
>
> -#include "heap/glue/MessageLoopInterruptor.h"
> -#include "heap/glue/PendingGCRunner.h"
> -#include "public/platform/WebThread.h"
>  #include "wtf/Functional.h"
>  #include "wtf/OwnPtr.h"
> +#include "public/platform/WebThread.h"
>
>  namespace WebCore {
>
> -class TaskSynchronizer;
> -
>  class HTMLParserThread {
>  public:
>      static void init();
> @@ -55,12 +51,8 @@ public:
>  private:
>      HTMLParserThread();
>      ~HTMLParserThread();
> -    void setupHTMLParserThread();
> -    void cleanupHTMLParserThread(TaskSynchronizer*);
>
>      OwnPtr<blink::WebThread> m_thread;
> -    OwnPtr<PendingGCRunner> m_pendingGCRunner;
> -    OwnPtr<MessageLoopInterruptor> m_messageLoopInterruptor;
>  };
>
>  } // namespace WebCore
> Index: Source/heap/ThreadState.h
> diff --git a/Source/heap/ThreadState.h b/Source/heap/ThreadState.h
> index df8de4ae37f55402e12d6393120e012a5ce171a0..
> 3e799158c144d011152f6ef167165fbfacc7b935 100644
> --- a/Source/heap/ThreadState.h
> +++ b/Source/heap/ThreadState.h
> @@ -80,7 +80,7 @@ enum ThreadAffinity {
>  class Node;
>  class CSSValue;
>
> -template<typename T, bool derivesNode = WTF::IsSubclass<T, Node>::value>
> struct DefaultThreadingTrait;
> +template<typename T, bool derivesNodeOrCSSValue = WTF::IsSubclass<T,
> Node>::value || WTF::IsSubclass<T, CSSValue>::value > struct
> DefaultThreadingTrait;
>
>  template<typename T>
>  struct DefaultThreadingTrait<T, false> {
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dpranke, 2014-04-01 22:37:11]

Note that in this case we only see the problems in content_unittests and
unit_tests -- which are really quite dramatically broken -- but those are
not run by the CQ by default.

I *think* it is now possible to run the non-blink configurations against
blink patches (e.g., 'linux_chromium_rel') and that would've caught this.
We should test this to see if it works, and add at least one
non-layout_test bot to the deafult set to see if that helps.


On Tue, Apr 1, 2014 at 3:29 PM, Rafael Weinstein <rafaelw@google.com> wrote:

> Ok, so this caused pretty extensive problems in the blink builders. It
> seems that it confused the test harnesses so that they started thinking
> that basically all of the tests failed.
>
> Dirk has some ideas of how this may have happened, but Kentaro, if you
> attempt to land this again, please
>
> a) try to repro this problem locally and find a solution before landing
> b) alert the gardener at the time that the change is going in so they can
> not spend as much time as we did trying to figure out what had happened.
>
>
>
>
> On Tue, Apr 1, 2014 at 2:27 PM, <rafaelw@chromium.org> wrote:
>
>> Reviewers: haraken, dpranke,
>>
>> Message:
>> Committed patchset #2 manually as r170602 (tree was closed).
>>
>> Description:
>> Revert "Oilpan: Support the HTML parser thread in oilpan"
>>
>> Speculative rollout of r170564 to see if unit_tests and content_unittests
>> start
>> working again.
>>
>> TBR=haraken
>> BUG=340522
>>
>> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170602
>>
>> Please review this at https://codereview.chromium.org/219243014/
>>
>> SVN Base: svn://svn.chromium.org/blink/trunk
>>
>> Affected files (+3, -36 lines):
>>   M Source/core/html/parser/HTMLParserThread.h
>>   M Source/core/html/parser/HTMLParserThread.cpp
>>   M Source/heap/ThreadState.h
>>
>>
>> Index: Source/core/html/parser/HTMLParserThread.cpp
>> diff --git a/Source/core/html/parser/HTMLParserThread.cpp
>> b/Source/core/html/parser/HTMLParserThread.cpp
>> index 0f1f0d4b27e2ed6038961482efafdd6e02e64337..
>> fe033fbd8d9d02b0b47d254b75d5eca89c041310 100644
>> --- a/Source/core/html/parser/HTMLParserThread.cpp
>> +++ b/Source/core/html/parser/HTMLParserThread.cpp
>> @@ -32,7 +32,6 @@
>>  #include "core/html/parser/HTMLParserThread.h"
>>
>>  #include "platform/Task.h"
>> -#include "platform/TaskSynchronizer.h"
>>  #include "public/platform/Platform.h"
>>  #include "wtf/PassOwnPtr.h"
>>
>> @@ -54,35 +53,13 @@ void HTMLParserThread::init()
>>      s_sharedThread = new HTMLParserThread;
>>  }
>>
>> -void HTMLParserThread::setupHTMLParserThread()
>> -{
>> -    m_pendingGCRunner = adoptPtr(new PendingGCRunner);
>> -    m_messageLoopInterruptor = adoptPtr(new MessageLoopInterruptor(&
>> platformThread()));
>> -    platformThread().addTaskObserver(m_pendingGCRunner.get());
>> -    ThreadState::attach();
>> -    ThreadState::current()->addInterruptor(m_
>> messageLoopInterruptor.get());
>> -}
>> -
>>  void HTMLParserThread::shutdown()
>>  {
>>      ASSERT(s_sharedThread);
>> -    TaskSynchronizer taskSynchronizer;
>> -   
s_sharedThread->postTask(WTF::bind(&HTMLParserThread::cleanupHTMLParserThread,
>> s_sharedThread, &taskSynchronizer));
>> -    taskSynchronizer.waitForTaskCompletion();
>>      delete s_sharedThread;
>>      s_sharedThread = 0;
>>  }
>>
>> -void HTMLParserThread::cleanupHTMLParserThread(TaskSynchronizer*
>> taskSynchronizer)
>> -{
>> -    ThreadState::current()->removeInterruptor(m_
>> messageLoopInterruptor.get());
>> -    ThreadState::detach();
>> -    platformThread().removeTaskObserver(m_pendingGCRunner.get());
>> -    taskSynchronizer->taskCompleted();
>> -    m_pendingGCRunner = nullptr;
>> -    m_messageLoopInterruptor = nullptr;
>> -}
>> -
>>  HTMLParserThread* HTMLParserThread::shared()
>>  {
>>      return s_sharedThread;
>> @@ -90,10 +67,8 @@ HTMLParserThread* HTMLParserThread::shared()
>>
>>  blink::WebThread& HTMLParserThread::platformThread()
>>  {
>> -    if (!m_thread) {
>> +    if (!m_thread)
>>          m_thread = adoptPtr(blink::Platform::current()->createThread("
>> HTMLParserThread"));
>> -        postTask(WTF::bind(&HTMLParserThread::setupHTMLParserThread,
>> this));
>> -    }
>>      return *m_thread;
>>  }
>>
>> Index: Source/core/html/parser/HTMLParserThread.h
>> diff --git a/Source/core/html/parser/HTMLParserThread.h
>> b/Source/core/html/parser/HTMLParserThread.h
>> index 1421e0dacc5dc236a406d9a7666552cf9dc86f13..
>> 1feb999166443ea7d96874bbd24895acc56a7c6b 100644
>> --- a/Source/core/html/parser/HTMLParserThread.h
>> +++ b/Source/core/html/parser/HTMLParserThread.h
>> @@ -31,16 +31,12 @@
>>  #ifndef HTMLParserThread_h
>>  #define HTMLParserThread_h
>>
>> -#include "heap/glue/MessageLoopInterruptor.h"
>> -#include "heap/glue/PendingGCRunner.h"
>> -#include "public/platform/WebThread.h"
>>  #include "wtf/Functional.h"
>>  #include "wtf/OwnPtr.h"
>> +#include "public/platform/WebThread.h"
>>
>>  namespace WebCore {
>>
>> -class TaskSynchronizer;
>> -
>>  class HTMLParserThread {
>>  public:
>>      static void init();
>> @@ -55,12 +51,8 @@ public:
>>  private:
>>      HTMLParserThread();
>>      ~HTMLParserThread();
>> -    void setupHTMLParserThread();
>> -    void cleanupHTMLParserThread(TaskSynchronizer*);
>>
>>      OwnPtr<blink::WebThread> m_thread;
>> -    OwnPtr<PendingGCRunner> m_pendingGCRunner;
>> -    OwnPtr<MessageLoopInterruptor> m_messageLoopInterruptor;
>>  };
>>
>>  } // namespace WebCore
>> Index: Source/heap/ThreadState.h
>> diff --git a/Source/heap/ThreadState.h b/Source/heap/ThreadState.h
>> index df8de4ae37f55402e12d6393120e012a5ce171a0..
>> 3e799158c144d011152f6ef167165fbfacc7b935 100644
>> --- a/Source/heap/ThreadState.h
>> +++ b/Source/heap/ThreadState.h
>> @@ -80,7 +80,7 @@ enum ThreadAffinity {
>>  class Node;
>>  class CSSValue;
>>
>> -template<typename T, bool derivesNode = WTF::IsSubclass<T, Node>::value>
>> struct DefaultThreadingTrait;
>> +template<typename T, bool derivesNodeOrCSSValue = WTF::IsSubclass<T,
>> Node>::value || WTF::IsSubclass<T, CSSValue>::value > struct
>> DefaultThreadingTrait;
>>
>>  template<typename T>
>>  struct DefaultThreadingTrait<T, false> {
>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

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

Thanks for the revert. LGTM.

I'm trying to reproduce the issue locally and fix.