1334068 - make nsHttpChannel/HttpBaseChannel dtor thread safe

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Several attributes in nsHttpChannel and HttpBaseChannel is not thread-safe to Release(). We should use nsMainThreadPtrHolder to make sure Release() of those XPCOM ptr are called only on main thread.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1334068 - Part 1, make nsHttpChannel thread-safe for dtor

Review commit: https://reviewboard.mozilla.org/r/107394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107394/

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1334068 - Part 2, make HttpBaseChannel thread-safe for dtor

Review commit: https://reviewboard.mozilla.org/r/107396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107396/

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

In this WIP I only enforce the Release() for those XPCOM runs on main thread, but allows dereference to be invoked on other threads.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Unless you need to actually move the references around off the main thread it would be better not to use individual nsMainThreadPtrHandle references here.  Each one dispatches a separate runnable which can be inefficient and clog up the main thread.  As written I think this patch will dispatch 11 runnables if the channel is destroyed off the main thread.

Can you instead do either:

a) Create your own doomed runnable that takes ownership of the runnables and is dispatched at the main thread to release them.
b) Create a main thread object that holds the other references and just nsMainThreadPtrHandle that single object.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks for the advice. I'll prefer (b) because people can understand the main-thread-only restriction from reading code.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

After trying both (a) and (b), I'll go (a) since the modification is more reviewable.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel.

Review commit: https://reviewboard.mozilla.org/r/109968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109968/

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8833841 [details]
Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel.

https://reviewboard.mozilla.org/r/109968/#review112462

::: netwerk/protocol/http/HttpBaseChannel.cpp:2358
(Diff revision 1)
> +      do_CreateInstance(NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    message->SetTag(pair.first());
> +    message->SetCategory(pair.second());
> +    aMessages.AppendElement(message);

please add a comment why this change is needed regarding t-safety of the dtor.  it's not clear to me at all.

::: netwerk/protocol/http/nsHttpChannel.h:465
(Diff revision 1)
>  
>      void SetDoNotTrack();
>  
>  private:
> +    // this section is for main-thread-only object
> +    // all the references need to be proxy released in main thread.

on the main thread

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8833841 [details]
Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel.

https://reviewboard.mozilla.org/r/109968/#review112462

> please add a comment why this change is needed regarding t-safety of the dtor.  it's not clear to me at all.

Originally security console messages are stored in an `nsCOMArray<nsISecurityConsoleMessage>`. The implementation of nsISecurityConsoleMessage is not thread-safe refcounted and I didn't find a way to move the nsCOMArray without `AddRef` to the array items. Therefore I change it to a safer type to store, which needs to delay the XPCOM creation until `TakeAllSecurityMessages`.
Add comment in next revision.

https://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/netwerk/protocol/http/HttpBaseChannel.h#354

> on the main thread

fixed in the next revision.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8833841 [details]
Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109968/diff/1-2/

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #9)
> Comment on attachment 8833841 [details]
> Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and
> HttpBaseChannel.
> 
> https://reviewboard.mozilla.org/r/109968/#review112462
> 
> > please add a comment why this change is needed regarding t-safety of the dtor.  it's not clear to me at all.
> 
> Originally security console messages are stored in an
> `nsCOMArray<nsISecurityConsoleMessage>`. The implementation of
> nsISecurityConsoleMessage is not thread-safe refcounted and I didn't find a
> way to move the nsCOMArray without `AddRef` to the array items. Therefore I
> change it to a safer type to store, which needs to delay the XPCOM creation
> until `TakeAllSecurityMessages`.
> Add comment in next revision.
> 

Thanks!  Nit: maybe add MOZ_ASSERT(NS_IsMainThread()) to nsHttpChannel where appropriate around manipulation with console messages?

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8833841 [details]
Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109968/diff/2-3/

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #9)
> > Comment on attachment 8833841 [details]
> > Bug 1334068 - proxy release main-thread-only references in nsHttpChannel and
> > HttpBaseChannel.
> > 
> > https://reviewboard.mozilla.org/r/109968/#review112462
> > 
> > > please add a comment why this change is needed regarding t-safety of the dtor.  it's not clear to me at all.
> > 
> > Originally security console messages are stored in an
> > `nsCOMArray<nsISecurityConsoleMessage>`. The implementation of
> > nsISecurityConsoleMessage is not thread-safe refcounted and I didn't find a
> > way to move the nsCOMArray without `AddRef` to the array items. Therefore I
> > change it to a safer type to store, which needs to delay the XPCOM creation
> > until `TakeAllSecurityMessages`.
> > Add comment in next revision.
> > 
> 
> Thanks!  Nit: maybe add MOZ_ASSERT(NS_IsMainThread()) to nsHttpChannel where
> appropriate around manipulation with console messages?

Sure, added MOZ_ASSERT(NS_IsMainThread()) in `HttpBaseChannel::TakeAllSecurityMessages` and `HttpBaseChannel::AddSecurityMessage`.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8208dedaa305
proxy release main-thread-only references in nsHttpChannel and HttpBaseChannel. r=mayhemer

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8208dedaa305