Ensure ThreatDetails is always destroyed on the UI thread.

Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.

Committed: https://crrev.com/c77858f66b35ae8d28f72c356cf2d2408da19028
Cr-Commit-Position: refs/heads/master@{#368518}

[Anand Mistry (off Chromium), 2016-01-08 04:53:25]

amistry@chromium.org changed reviewers:
+ shess@chromium.org

[Anand Mistry (off Chromium), 2016-01-08 04:53:25]

I noticed this while running TSAN:
WARNING: ThreadSanitizer: data race (pid=23497)
  Read of size 8 at 0x7d6c001c0fb0 by main thread:
    #0 end
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../buildtools/third_party/libc++/trunk/include/vector:1481:30
(libcontent.so+0x000000d559de)
    #1 RemoveObserver
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/observer_list.h:175
(libcontent.so+0x000000d559de)
    #2 content::WebContentsImpl::RemoveObserver(content::WebContentsObserver*)
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../content/browser/web_contents/web_contents_impl.cc:1488
(libcontent.so+0x000000d559de)
    #3 content::WebContentsObserver::~WebContentsObserver()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../content/public/browser/web_contents_observer.cc:24:5
(libcontent.so+0x00000052d99f)
    #4 ~InterstitialObserver
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../content/public/test/browser_test_utils.cc:121:37
(browser_tests+0x000002e4a2f2)
...

  Previous write of size 8 at 0x7d6c001c0fb0 by thread T18:
    #0 __destruct_at_end
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../buildtools/third_party/libc++/trunk/include/vector:424:68
(libcontent.so+0x000000d55a7b)
    #1 __destruct_at_end
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../buildtools/third_party/libc++/trunk/include/vector:812
(libcontent.so+0x000000d55a7b)
    #2 erase
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../buildtools/third_party/libc++/trunk/include/vector:1677
(libcontent.so+0x000000d55a7b)
    #3 RemoveObserver
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/observer_list.h:179
(libcontent.so+0x000000d55a7b)
    #4 content::WebContentsImpl::RemoveObserver(content::WebContentsObserver*)
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../content/browser/web_contents/web_contents_impl.cc:1488
(libcontent.so+0x000000d55a7b)
    #5 content::WebContentsObserver::~WebContentsObserver()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../content/public/browser/web_contents_observer.cc:24:5
(libcontent.so+0x00000052d99f)
    #6 safe_browsing::ThreatDetails::~ThreatDetails()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../chrome/browser/safe_browsing/threat_details.cc:114:34
(browser_tests+0x0000017a0c59)
    #7 safe_browsing::(anonymous
namespace)::FakeThreatDetails::~FakeThreatDetails()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:282:33
(browser_tests+0x0000012e1df9)
    #8 DeleteInternal
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/memory/ref_counted.h:193:44
(browser_tests+0x0000017a3600)
...
    #19 base::internal::CallbackBase::~CallbackBase()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/callback_internal.cc:43
(libbase.so+0x00000006be47)
    #20 base::PendingTask::~PendingTask()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/pending_task.cc:34:1
(libbase.so+0x0000000c6fca)
    #21 base::MessageLoop::DoWork()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/message_loop/message_loop.cc:612:5
(libbase.so+0x0000000af36a)

It looks like the IO thread has the last reference to the ThreatDetails and is
destroying it. I'm not familiar with this code or the typical lifecycle of
ThreatDetails, so it's possible this is only an issue with tests.

[Scott Hess - ex-Googler, 2016-01-08 05:36:11]

shess@chromium.org changed reviewers:
+ nparker@chromium.org

[Scott Hess - ex-Googler, 2016-01-08 05:36:11]

On 2016/01/08 04:53:25, Anand Mistry wrote:
> I'm not familiar with this code or the typical lifecycle of
> ThreatDetails, so it's possible this is only an issue with tests.

You and I both!  Bouncing to Nathan for purposes of redirecting to someone
knowledgeable.

[Anand Mistry (off Chromium), 2016-01-08 07:36:30]

Description was changed from

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreadDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.
==========

to

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.
==========

[Nathan Parker, 2016-01-08 19:46:20]

nparker@chromium.org changed reviewers:
+ jialiul@chromium.org

[Nathan Parker, 2016-01-08 19:46:21]

+jialiu, who has touched this most recently

[Jialiu Lin, 2016-01-08 20:38:07]

On 2016/01/08 19:46:21, nparker wrote:
> +jialiu, who has touched this most recently

Hi Anand,
Thanks for working on this. 
Does the error/warning message before or after your CL?

[Anand Mistry (off Chromium), 2016-01-08 22:03:32]

On 2016/01/08 20:38:07, JialiuLin wrote:
> On 2016/01/08 19:46:21, nparker wrote:
> > +jialiu, who has touched this most recently
> 
> Hi Anand,
> Thanks for working on this. 
> Does the error/warning message before or after your CL?

It occurs before, when running browser_tests under TSAN. This change fixes the
cause of the error by ensuring ThreatDeatils is destroyed on the UI thread.

[chromium-reviews, 2016-01-08 22:05:29]

Great. Thanks for fixing it! lgtm  (well, you probably still need lgtm from
nparker@)

On Fri, Jan 8, 2016 at 2:03 PM, <amistry@chromium.org> wrote:

> On 2016/01/08 20:38:07, JialiuLin wrote:
>
>> On 2016/01/08 19:46:21, nparker wrote:
>> > +jialiu, who has touched this most recently
>>
>
> Hi Anand,
>> Thanks for working on this.
>> Does the error/warning message before or after your CL?
>>
>
> It occurs before, when running browser_tests under TSAN. This change fixes
> the
> cause of the error by ensuring ThreatDeatils is destroyed on the UI thread.
>
> https://codereview.chromium.org/1569993003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jialiu Lin, 2016-01-08 22:05:57]

On 2016/01/08 22:05:29, chromium-reviews wrote:
> Great. Thanks for fixing it! lgtm  (well, you probably still need lgtm from
> nparker@)
> 
> On Fri, Jan 8, 2016 at 2:03 PM, <mailto:amistry@chromium.org> wrote:
> 
> > On 2016/01/08 20:38:07, JialiuLin wrote:
> >
> >> On 2016/01/08 19:46:21, nparker wrote:
> >> > +jialiu, who has touched this most recently
> >>
> >
> > Hi Anand,
> >> Thanks for working on this.
> >> Does the error/warning message before or after your CL?
> >>
> >
> > It occurs before, when running browser_tests under TSAN. This change fixes
> > the
> > cause of the error by ensuring ThreatDeatils is destroyed on the UI thread.
> >
> > https://codereview.chromium.org/1569993003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

lgtm

[Nathan Parker, 2016-01-08 22:26:50]

lgtm

[Anand Mistry (off Chromium), 2016-01-09 02:52:59]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-01-09 02:53:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569993003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569993003/1

[commit-bot: I haz the power, 2016-01-09 03:30:12]

Description was changed from

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.
==========

to

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.
==========

[commit-bot: I haz the power, 2016-01-09 03:30:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-09 03:31:06]

Description was changed from

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.
==========

to

==========
Ensure ThreatDetails is always destroyed on the UI thread.

Since ThreatDetails subclasses content::WebContentsObserver, it must be
destroyed on the UI thread since ~WebContentsObserver unregisters itself
from a WebContents.

Committed: https://crrev.com/c77858f66b35ae8d28f72c356cf2d2408da19028
Cr-Commit-Position: refs/heads/master@{#368518}
==========

[commit-bot: I haz the power, 2016-01-09 03:31:07]

Patchset 1 (id:??) landed as
https://crrev.com/c77858f66b35ae8d28f72c356cf2d2408da19028
Cr-Commit-Position: refs/heads/master@{#368518}