New issue
Advanced search Search tips

Issue 872742 link

Starred by 1 user
Status: Fixed
Owner:
shimazu@chromium.org
Closed: Nov 20
Components:
EstimatedDays: ----
NextAction: 2018-11-19
OS: ----
Pri: 2
Type: Bug
Flaky-Test: DoNotTrackTest.Worker



Sign in to add a comment

DoNotTrackTest.Worker data race during teardown

Project Member Reported by ellyjo...@chromium.org, Aug 9 
This test sometimes fails on linux tsan like this: <https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/24626>

[ RUN      ] DoNotTrackTest.Worker
Xlib:  extension "RANDR" missing on display ":99".
DevTools listening on ws://127.0.0.1:39774/devtools/browser/4d625df8-aad2-4b6f-9c9a-91d222594969
==================
WARNING: ThreadSanitizer: data race (pid=24623)
  Read of size 8 at 0x7b1000023cc8 by thread T2:
    #0 browser content/public/common/content_client.h:82:44 (content_browsertests+0x46e1060)
    #1 content::MimeSniffingResourceHandler::OnReadCompleted(int, std::__1::unique_ptr<content::ResourceController, std::__1::default_delete<content::ResourceController> >) content/browser/loader/mime_sniffing_resource_handler.cc:244 (content_browsertests+0x46e1060)
    #2 content::LayeredResourceHandler::OnReadCompleted(int, std::__1::unique_ptr<content::ResourceController, std::__1::default_delete<content::ResourceController> >) content/browser/loader/layered_resource_handler.cc:62:18 (content_browsertests+0x46deaa3)
    #3 content::ResourceLoader::CompleteRead(int) content/browser/loader/resource_loader.cc:807:13 (content_browsertests+0x47079b0)
    #4 content::ResourceLoader::OnReadCompleted(net::URLRequest*, int) content/browser/loader/resource_loader.cc:485:3 (content_browsertests+0x470777e)
    #5 content::ResourceLoader::ReadMore(bool) content/browser/loader/resource_loader.cc:767:5 (content_browsertests+0x47061aa)
    #6 content::ResourceLoader::PrepareToReadMore(bool) content/browser/loader/resource_loader.cc:742:5 (content_browsertests+0x4708839)
    #7 content::ResourceLoader::CompleteResponseStarted() content/browser/loader/resource_loader.cc:725:5 (content_browsertests+0x4707695)
    #8 content::ResourceLoader::OnResponseStarted(net::URLRequest*, int) content/browser/loader/resource_loader.cc:467:3 (content_browsertests+0x470706e)
    #9 net::URLRequest::NotifyResponseStarted(net::URLRequestStatus const&) net/url_request/url_request.cc:856:16 (content_browsertests+0x573e20c)
    #10 net::URLRequestJob::NotifyHeadersComplete() net/url_request/url_request_job.cc:494:13 (content_browsertests+0x574bcb0)
    #11 net::URLRequestHttpJob::NotifyHeadersComplete() net/url_request/url_request_http_job.cc:523:18 (content_browsertests+0x574fa28)
    #12 net::URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int) net/url_request/url_request_http_job.cc:810:3 (content_browsertests+0x5752820)
    #13 net::URLRequestHttpJob::OnStartCompleted(int) net/url_request/url_request_http_job.cc:1034:5 (content_browsertests+0x57518dd)
    #14 Invoke<void (net::URLRequestHttpJob::*)(int), net::URLRequestHttpJob *, int> base/bind_internal.h:516:12 (content_browsertests+0x575553f)
    #15 MakeItSo<void (net::URLRequestHttpJob::*const &)(int), net::URLRequestHttpJob *, int> base/bind_internal.h:616 (content_browsertests+0x575553f)
    #16 RunImpl<void (net::URLRequestHttpJob::*const &)(int), const std::__1::tuple<base::internal::UnretainedWrapper<net::URLRequestHttpJob> > &, 0> base/bind_internal.h:689 (content_browsertests+0x575553f)
    #17 base::internal::Invoker<base::internal::BindState<void (net::URLRequestHttpJob::*)(int), base::internal::UnretainedWrapper<net::URLRequestHttpJob> >, void (int)>::Run(base::internal::BindStateBase*, int) base/bind_internal.h:671 (content_browsertests+0x575553f)
    #18 Run base/callback.h:99:12 (content_browsertests+0x553297f)
    #19 net::HttpCache::Transaction::DoLoop(int) net/http/http_cache_transaction.cc:1032 (content_browsertests+0x553297f)
    #20 net::HttpCache::Transaction::OnIOComplete(int) net/http/http_cache_transaction.cc:3558:3 (content_browsertests+0x5530e10)
    #21 Invoke<void (net::HttpCache::Transaction::*)(int), const base::WeakPtr<net::HttpCache::Transaction> &, int> base/bind_internal.h:516:12 (content_browsertests+0x55427d8)
    #22 MakeItSo<void (net::HttpCache::Transaction::*const &)(int), const base::WeakPtr<net::HttpCache::Transaction> &, int> base/bind_internal.h:636 (content_browsertests+0x55427d8)
    #23 RunImpl<void (net::HttpCache::Transaction::*const &)(int), const std::__1::tuple<base::WeakPtr<net::HttpCache::Transaction> > &, 0> base/bind_internal.h:689 (content_browsertests+0x55427d8)
    #24 base::internal::Invoker<base::internal::BindState<void (net::HttpCache::Transaction::*)(int), base::WeakPtr<net::HttpCache::Transaction> >, void (int)>::Run(base::internal::BindStateBase*, int) base/bind_internal.h:671 (content_browsertests+0x55427d8)
    #25 Run base/callback.h:99:12 (content_browsertests+0x54cb625)
    #26 RunCallback net/disk_cache/blockfile/in_flight_backend_io.cc:93 (content_browsertests+0x54cb625)
    #27 disk_cache::InFlightBackendIO::OnOperationComplete(disk_cache::BackgroundIO*, bool) net/disk_cache/blockfile/in_flight_backend_io.cc:578 (content_browsertests+0x54cb625)
    #28 InvokeCallback net/disk_cache/blockfile/in_flight_io.cc:101:3 (content_browsertests+0x54c6af7)
    #29 disk_cache::BackgroundIO::OnIOSignalled() net/disk_cache/blockfile/in_flight_io.cc:25 (content_browsertests+0x54c6af7)
    #30 Invoke<void (disk_cache::BackgroundIO::*)(), const scoped_refptr<disk_cache::BackgroundIO> &> base/bind_internal.h:516:12 (content_browsertests+0x54c7539)
    #31 MakeItSo<void (disk_cache::BackgroundIO::*const &)(), const scoped_refptr<disk_cache::BackgroundIO> &> base/bind_internal.h:616 (content_browsertests+0x54c7539)
    #32 RunImpl<void (disk_cache::BackgroundIO::*const &)(), const std::__1::tuple<scoped_refptr<disk_cache::BackgroundIO> > &, 0> base/bind_internal.h:689 (content_browsertests+0x54c7539)
    #33 base::internal::Invoker<base::internal::BindState<void (disk_cache::BackgroundIO::*)(), scoped_refptr<disk_cache::BackgroundIO> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:671 (content_browsertests+0x54c7539)
    #34 Run base/callback.h:99:12 (content_browsertests+0x52b9e67)
    #35 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:101 (content_browsertests+0x52b9e67)
    #36 base::MessageLoop::RunTask(base::PendingTask*) base/message_loop/message_loop.cc:431:46 (content_browsertests+0x52b7f35)
    #37 DeferOrRunPendingTask base/message_loop/message_loop.cc:442:5 (content_browsertests+0x52b87eb)
    #38 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:514 (content_browsertests+0x52b87eb)
    #39 base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_libevent.cc:210:31 (content_browsertests+0x539eef4)
    #40 Run base/message_loop/message_loop.cc:383:12 (content_browsertests+0x52b7990)
    #41 non-virtual thunk to base::MessageLoop::Run(bool) base/message_loop/message_loop.cc (content_browsertests+0x52b7990)
    #42 base::RunLoop::Run() base/run_loop.cc:102:14 (content_browsertests+0x52e946a)
    #43 base::Thread::Run(base::RunLoop*) base/threading/thread.cc:262:13 (content_browsertests+0x5345ea9)
    #44 content::BrowserProcessSubThread::IOThreadRun(base::RunLoop*) content/browser/browser_process_sub_thread.cc:178:11 (content_browsertests+0x43c6f06)
    #45 content::BrowserProcessSubThread::Run(base::RunLoop*) content/browser/browser_process_sub_thread.cc:128:7 (content_browsertests+0x43c6e68)
    #46 base::Thread::ThreadMain() base/threading/thread.cc:357:3 (content_browsertests+0x534615e)
    #47 base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:76:13 (content_browsertests+0x539aaad)
  Previous write of size 8 at 0x7b1000023cc8 by main thread:
    #0 SetBrowser content/public/common/content_client.cc:23:24 (content_browsertests+0x2f5a175)
    #1 content::SetBrowserClientForTesting(content::ContentBrowserClient*) content/public/common/content_client.cc:49 (content_browsertests+0x2f5a175)
    #2 content::(anonymous namespace)::DoNotTrackTest::TearDownOnMainThread() content/browser/do_not_track_browsertest.cc:39:7 (content_browsertests+0x1afd328)
    #3 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() content/public/test/browser_test_base.cc:409:5 (content_browsertests+0x4dd056b)
    #4 Invoke<void (content::BrowserTestBase::*)(), content::BrowserTestBase *> base/bind_internal.h:516:12 (content_browsertests+0x4dd1868)
    #5 MakeItSo<void (content::BrowserTestBase::*const &)(), content::BrowserTestBase *> base/bind_internal.h:616 (content_browsertests+0x4dd1868)
    #6 RunImpl<void (content::BrowserTestBase::*const &)(), const std::__1::tuple<base::internal::UnretainedWrapper<content::BrowserTestBase> > &, 0> base/bind_internal.h:689 (content_browsertests+0x4dd1868)
    #7 base::internal::Invoker<base::internal::BindState<void (content::BrowserTestBase::*)(), base::internal::UnretainedWrapper<content::BrowserTestBase> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:671 (content_browsertests+0x4dd1868)
    #8 Run base/callback.h:129:12 (content_browsertests+0x4eae4b0)
    #9 content::ShellBrowserMainParts::PreMainMessageLoopRun() content/shell/browser/shell_browser_main_parts.cc:199 (content_browsertests+0x4eae4b0)
    #10 content::BrowserMainLoop::PreMainMessageLoopRun() content/browser/browser_main_loop.cc:1016:13 (content_browsertests+0x43b8fb0)
    #11 Invoke<int (content::BrowserMainLoop::*)(), content::BrowserMainLoop *> base/bind_internal.h:516:12 (content_browsertests+0x43bc138)
    #12 MakeItSo<int (content::BrowserMainLoop::*const &)(), content::BrowserMainLoop *> base/bind_internal.h:616 (content_browsertests+0x43bc138)
    #13 RunImpl<int (content::BrowserMainLoop::*const &)(), const std::__1::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > &, 0> base/bind_internal.h:689 (content_browsertests+0x43bc138)
    #14 base::internal::Invoker<base::internal::BindState<int (content::BrowserMainLoop::*)(), base::internal::UnretainedWrapper<content::BrowserMainLoop> >, int ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:671 (content_browsertests+0x43bc138)
    #15 Run base/callback.h:129:12 (content_browsertests+0x49b884e)
    #16 content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:43 (content_browsertests+0x49b884e)
    #17 content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:927:25 (content_browsertests+0x43b71f4)
    #18 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner_impl.cc:141:15 (content_browsertests+0x43bc8da)
    #19 ShellBrowserMain(content::MainFunctionParams const&, std::__1::unique_ptr<content::BrowserMainRunner, std::__1::default_delete<content::BrowserMainRunner> > const&) content/shell/browser/shell_browser_main.cc:22:32 (content_browsertests+0x4eadac7)
    #20 content::ShellMainDelegate::RunProcess(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&) content/shell/app/shell_main_delegate.cc:360:16 (content_browsertests+0x4e4a2d2)
    #21 RunBrowserProcessMain content/app/content_main_runner_impl.cc:526:29 (content_browsertests+0x4258236)
    #22 content::ContentMainRunnerImpl::Run(bool) content/app/content_main_runner_impl.cc:888 (content_browsertests+0x4258236)
    #23 content::ContentServiceManagerMainDelegate::RunEmbedderProcess() content/app/content_service_manager_main_delegate.cc:53:32 (content_browsertests+0x3323fdf)
    #24 service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:472:29 (content_browsertests+0x71c2eb5)
    #25 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10 (content_browsertests+0x332475b)
    #26 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:322:3 (content_browsertests+0x4dd004a)
    #27 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:104:20 (content_browsertests+0x4dbfd9b)
    #28 testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f3a6fb)
    #29 testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2682:11 (content_browsertests+0x2f3b92c)
    #30 testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2800:28 (content_browsertests+0x2f3c1b6)
    #31 testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5124:43 (content_browsertests+0x2f4c586)
    #32 testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f4be6a)
    #33 RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2331:46 (content_browsertests+0x4e1e115)
    #34 base::TestSuite::Run() base/test/test_suite.cc:277 (content_browsertests+0x4e1e115)
    #35 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:118:48 (content_browsertests+0x4dc5861)
    #36 content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:645:31 (content_browsertests+0x4df98b1)
    #37 main content/test/content_test_launcher.cc:148:10 (content_browsertests+0x4dc57ed)
  Location is heap block of size 64 at 0x7b1000023cc0 allocated by main thread:
    #0 operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/tsan/rtl/tsan_new_delete.cc:65:3 (content_browsertests+0x19f0a4d)
    #1 content::ShellMainDelegate::BasicStartupComplete(int*) content/shell/app/shell_main_delegate.cc (content_browsertests+0x4e49f2e)
    #2 content::ContentMainRunnerImpl::Initialize(content::ContentMainParams const&) content/app/content_main_runner_impl.cc:656:18 (content_browsertests+0x4257598)
    #3 content::ContentServiceManagerMainDelegate::Initialize(service_manager::MainDelegate::InitializeParams const&) content/app/content_service_manager_main_delegate.cc:38:32 (content_browsertests+0x3323d47)
    #4 service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:416:27 (content_browsertests+0x71c3084)
    #5 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10 (content_browsertests+0x332475b)
    #6 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:322:3 (content_browsertests+0x4dd004a)
    #7 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:104:20 (content_browsertests+0x4dbfd9b)
    #8 testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f3a6fb)
    #9 testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2682:11 (content_browsertests+0x2f3b92c)
    #10 testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2800:28 (content_browsertests+0x2f3c1b6)
    #11 testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5124:43 (content_browsertests+0x2f4c586)
    #12 testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f4be6a)
    #13 RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2331:46 (content_browsertests+0x4e1e115)
    #14 base::TestSuite::Run() base/test/test_suite.cc:277 (content_browsertests+0x4e1e115)
    #15 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:118:48 (content_browsertests+0x4dc5861)
    #16 content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:645:31 (content_browsertests+0x4df98b1)
    #17 main content/test/content_test_launcher.cc:148:10 (content_browsertests+0x4dc57ed)
  Thread T2 'Chrome_IOThread' (tid=24695, running) created by main thread at:
    #0 pthread_create /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:965:3 (content_browsertests+0x1986a65)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:115:13 (content_browsertests+0x539a637)
    #2 base::PlatformThread::CreateWithPriority(unsigned long, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:200:10 (content_browsertests+0x539a535)
    #3 base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:119:15 (content_browsertests+0x53459d9)
    #4 content::BrowserProcessSubThread::CreateIOThread() content/browser/browser_process_sub_thread.cc:91:19 (content_browsertests+0x43c6c7e)
    #5 content::ContentMainRunnerImpl::Run(bool) content/app/content_main_runner_impl.cc:868:29 (content_browsertests+0x42580bc)
    #6 content::ContentServiceManagerMainDelegate::RunEmbedderProcess() content/app/content_service_manager_main_delegate.cc:53:32 (content_browsertests+0x3323fdf)
    #7 service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:472:29 (content_browsertests+0x71c2eb5)
    #8 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10 (content_browsertests+0x332475b)
    #9 content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:322:3 (content_browsertests+0x4dd004a)
    #10 content::ContentBrowserTest::SetUp() content/public/test/content_browser_test.cc:104:20 (content_browsertests+0x4dbfd9b)
    #11 testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f3a6fb)
    #12 testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2682:11 (content_browsertests+0x2f3b92c)
    #13 testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2800:28 (content_browsertests+0x2f3c1b6)
    #14 testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5124:43 (content_browsertests+0x2f4c586)
    #15 testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc (content_browsertests+0x2f4be6a)
    #16 RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2331:46 (content_browsertests+0x4e1e115)
    #17 base::TestSuite::Run() base/test/test_suite.cc:277 (content_browsertests+0x4e1e115)
    #18 content::ContentTestLauncherDelegate::RunTestSuite(int, char**) content/test/content_test_launcher.cc:118:48 (content_browsertests+0x4dc5861)
    #19 content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:645:31 (content_browsertests+0x4df98b1)
    #20 main content/test/content_test_launcher.cc:148:10 (content_browsertests+0x4dc57ed)
SUMMARY: ThreadSanitizer: data race content/public/common/content_client.h:82:44 in browser

Comment 1 by shimazu@chromium.org, Aug 10

Components: Blink>Workers
Flaky-Test: DoNotTrackTest.Worker
Labels: Test-Flaky
Thanks for filing this.
I think this happens due to competing destruction of the test and the worker script loading. The test finishes just after the EmbeddedTestServer receives a request for the script, and at the time BrowserClient is changed back to the original one. This might be causing the threading issue.
I'm guessing a possible solution of this would be waiting until finishing script loads by each test.

Comment 2 by nhiroki@chromium.org, Nov 14 

It looks like the test is still flaky:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=content_browsertests&tests=DoNotTrackTest.Worker

shimazu@: Can you fix this or disable the test for "Linux TSan Tests"?

Comment 3 by shimazu@chromium.org, Nov 15

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdf490e8a9dc719f929359222b2cc2a295be1c46

commit cdf490e8a9dc719f929359222b2cc2a295be1c46
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Fri Nov 16 06:46:49 2018

Deflake content_browsertest: DoNotTrackTest using workers

Previously these tests finish before the worker is started, and
TearDownOnMainThread could be called during loading the script. After this CL,
each tests waits until the worker is ready to ensure that no task is running on
TearDownOnMainThread.

Bug:  872742 
Change-Id: I5fc33048f852f6c7088f5a971a4a7d4a4ffa410b
Reviewed-on: https://chromium-review.googlesource.com/c/1337146
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608677}
[modify] https://crrev.com/cdf490e8a9dc719f929359222b2cc2a295be1c46/content/browser/do_not_track_browsertest.cc
[modify] https://crrev.com/cdf490e8a9dc719f929359222b2cc2a295be1c46/content/test/data/workers/create_shared_worker.html
[modify] https://crrev.com/cdf490e8a9dc719f929359222b2cc2a295be1c46/content/test/data/workers/create_worker.html

Comment 5 by shimazu@chromium.org, Nov 16

NextAction: 2018-11-19
Let's check if it's fixed next week.

Comment 6 by monor...@bugs.chromium.org, Nov 19 

The NextAction date has arrived: 2018-11-19

Comment 7 by shimazu@chromium.org, Nov 20

Labels: M-72
Status: Fixed (was: Started)
Looks good!
►

Sign in to add a comment