ServiceWorker: Run functions in the browsertest on appropriate thread

ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox --gtest_filter="ServiceWorkerVersionBrowserTest.*"

Committed: https://crrev.com/45268ac16ef424a359dbc5d130a2225dd7f13368
Cr-Commit-Position: refs/heads/master@{#410300}

[shimazu, 2016-08-02 08:33:58]

shimazu@chromium.org changed reviewers:
+ nhiroki@chromium.org

[shimazu, 2016-08-02 08:33:59]

PTAL

[nhiroki, 2016-08-02 09:15:43]

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:386: const GURL&
source_url) override {
Can you add DCHECK_CURRENTLY_ON(BrowserThread::IO)?

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:392: }
How about running these operations on the UI thread instead of using a lock?

void OnReportConsoleMessageOnUI(const base::string16& message) {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  messages_.push_back(message);
  if (!quit_.is_null() && messages_.size() == expected_message_count_) {
    base::Closure quit = quit_;
    quit_.Reset();
    quit.Run();
  }
}

// In OnReportConsoleMessage()
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
&ConsoleListener::OnReportConsoleMessageOnUI);

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:421: base::Closure
quit_;
We may want to add a comment like "// These must be accessed on the IO thread.".

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:976: void
OnThreadStarted() override {
DCHECK_CURRENTLY_ON(BrowserThread::IO)

[nhiroki, 2016-08-02 09:16:32]

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:392: }
On 2016/08/02 09:15:43, nhiroki wrote:
> How about running these operations on the UI thread instead of using a lock?
> 
> void OnReportConsoleMessageOnUI(const base::string16& message) {
>   DCHECK_CURRENTLY_ON(BrowserThread::UI);
>   messages_.push_back(message);
>   if (!quit_.is_null() && messages_.size() == expected_message_count_) {
>     base::Closure quit = quit_;
>     quit_.Reset();
>     quit.Run();
>   }
> }
> 
> // In OnReportConsoleMessage()
> BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
> &ConsoleListener::OnReportConsoleMessageOnUI);

BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
&ConsoleListener::OnReportConsoleMessageOnUI, message);

[nhiroki, 2016-08-02 09:23:22]

> those were not actual problems in this browsertest because each code blocks
are blocked by hand to avoid real race conditions.

I guess these have been potential problems, but merely haven't happened until
the IPC change.

[shimazu, 2016-08-02 23:34:49]

Thanks, ptal again:)

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:386: const GURL&
source_url) override {
On 2016/08/02 09:15:43, nhiroki wrote:
> Can you add DCHECK_CURRENTLY_ON(BrowserThread::IO)?

Done.

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:392: }
On 2016/08/02 09:16:32, nhiroki wrote:
> On 2016/08/02 09:15:43, nhiroki wrote:
> > How about running these operations on the UI thread instead of using a lock?
> > 
> > void OnReportConsoleMessageOnUI(const base::string16& message) {
> >   DCHECK_CURRENTLY_ON(BrowserThread::UI);
> >   messages_.push_back(message);
> >   if (!quit_.is_null() && messages_.size() == expected_message_count_) {
> >     base::Closure quit = quit_;
> >     quit_.Reset();
> >     quit.Run();
> >   }
> > }
> > 
> > // In OnReportConsoleMessage()
> > BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
> > &ConsoleListener::OnReportConsoleMessageOnUI);
> 
> BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
> &ConsoleListener::OnReportConsoleMessageOnUI, message);
> 

Done.

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:421: base::Closure
quit_;
On 2016/08/02 09:15:42, nhiroki wrote:
> We may want to add a comment like "// These must be accessed on the IO
thread.".

Done.

https://codereview.chromium.org/2204873002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:976: void
OnThreadStarted() override {
On 2016/08/02 09:15:43, nhiroki wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO)

Done.

[nhiroki, 2016-08-03 00:00:58]

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:410: void
OnReportCOnsoleMessageOnUI(const base::string16& message) {
s/COnsole/Console/

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:414:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, quit_);
This PostTask wouldn't be necessary because we're already on the UI thread.

[shimazu, 2016-08-03 00:15:58]

Thanks, Fixed.

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:410: void
OnReportCOnsoleMessageOnUI(const base::string16& message) {
On 2016/08/03 00:00:58, nhiroki wrote:
> s/COnsole/Console/

Done.

https://codereview.chromium.org/2204873002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:414:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, quit_);
On 2016/08/03 00:00:58, nhiroki wrote:
> This PostTask wouldn't be necessary because we're already on the UI thread.

Done, and also removed unnecessary check (messages.size() is monotonically
increasing)

[nhiroki, 2016-08-03 00:29:45]

On 2016/08/02 09:23:22, nhiroki wrote:
> > those were not actual problems in this browsertest because each code blocks
> are blocked by hand to avoid real race conditions.
> 
> I guess these have been potential problems, but merely haven't happened until
> the IPC change.

Sorry, I meant to ask you to update the CL description. Let me clarify this
more... I think thread blocking didn't avoid race conditions because
OnReportConsoleMessage() could be called before the UI thread starts
base::RunLoop. According to the issue 631311, the IPC change completes IPC
channel setup earlier than it was completed before. Probably, after the change,
OnReportConsoleMessage() tends to be called earlier than base::RunLoop::Run() on
the UI thread, and TSan detects it.

[shimazu, 2016-08-03 00:34:01]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions; those were not actual problems in this
browsertest because each code blocks are blocked by hand to avoid real race
conditions. TSan failures will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions; previously some variables (|quit_| and
|messages_|) were shared among IO thread and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread. TSan
failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

[shimazu, 2016-08-03 00:34:53]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions; previously some variables (|quit_| and
|messages_|) were shared among IO thread and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread. TSan
failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions; previously some variables (|quit_| and
|messages_|) were shared among IO thread and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

[shimazu, 2016-08-03 03:08:09]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions; previously some variables (|quit_| and
|messages_|) were shared among IO thread and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

[shimazu, 2016-08-03 03:35:18]

On 2016/08/03 00:29:45, nhiroki wrote:
> On 2016/08/02 09:23:22, nhiroki wrote:
> > > those were not actual problems in this browsertest because each code
blocks
> > are blocked by hand to avoid real race conditions.
> > 
> > I guess these have been potential problems, but merely haven't happened
until
> > the IPC change.
> 
> Sorry, I meant to ask you to update the CL description. Let me clarify this
> more... I think thread blocking didn't avoid race conditions because
> OnReportConsoleMessage() could be called before the UI thread starts
> base::RunLoop. According to the issue 631311, the IPC change completes IPC
> channel setup earlier than it was completed before. Probably, after the
change,
> OnReportConsoleMessage() tends to be called earlier than base::RunLoop::Run()
on
> the UI thread, and TSan detects it.

Ah, apparently I didn't understand the actual problem around the IPC change.
Updated the description. 
PTAL again.

[nhiroki, 2016-08-03 06:09:30]

lgtm

Reg: CL subject and description, s/appropreate/appropriate/

[shimazu, 2016-08-03 06:17:09]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropreate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

[shimazu, 2016-08-03 06:17:42]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-08-03 06:17:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 06:24:51]

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

[commit-bot: I haz the power, 2016-08-03 06:24:53]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shimazu, 2016-08-08 02:57:47]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-08-08 02:58:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 04:48:50]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

[commit-bot: I haz the power, 2016-08-08 04:48:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-08 04:50:27]

Description was changed from

==========
ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"
==========

to

==========
ServiceWorker: Run functions in the browsertest on appropriate thread

This patch fixes some race conditions which are exposed by fixes of mojo;
previously some variables (|quit_| and |messages_|) were shared among IO thread
and UI thread in race condition, and
EmbeddedWorkerInstance::AddListener/RemoveListener was called on UI thread.
TSan failures caused by these problems will be cleared by this patch.

BUG=631323, 631311
TEST=/out/tsan/content_browsertests --no-sandbox
--gtest_filter="ServiceWorkerVersionBrowserTest.*"

Committed: https://crrev.com/45268ac16ef424a359dbc5d130a2225dd7f13368
Cr-Commit-Position: refs/heads/master@{#410300}
==========

[commit-bot: I haz the power, 2016-08-08 04:50:28]

Patchset 5 (id:??) landed as
https://crrev.com/45268ac16ef424a359dbc5d130a2225dd7f13368
Cr-Commit-Position: refs/heads/master@{#410300}