904594 - Make the Networking Dashboard thread safe

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

In its current state, the Networking Dashboard is not thread safe and it can't even be called from the same thread multiple times (if the previous call hasn't ended)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: runnable.patch

Attaching a patch based on http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#365 that creates a runnable event with a given argument. It is related to Bug 622728 , but this patch doesn't cover all the corner cases listed in that bug.

Comment 2

[Catalin Iordache]

Attachment: MakingDashboardThreadSafe.patch

This patch contains implementation of new runnable event (NS_DashboardRunnable)  by Valentin and also its functionality over socket and http data.
I will not make Valentins patch obsolete because this way you can have a better perspective over my work and his

Comment 3

[Catalin Iordache]

There is still a lot of work to be done, but I wanted to see if I'm heading towards the right direction

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 794723 [details] [diff] [review]
MakingDashboardThreadSafe.patch

Review of attachment 794723 [details] [diff] [review]:
-----------------------------------------------------------------

Keep it up, Catalin. It looks pretty good so far.
Also, if you have the option of cleaning up the code (such as the comment below), please do.

::: netwerk/base/src/Dashboard.cpp
@@ +133,5 @@
>  
>      JS::RootedValue val(cx);
>      if (!dict.ToObject(cx, JS::NullPtr(), &val)) {
> +        sockData.cb = nullptr;
> +        sockData.data.Clear();

One of the best things about making it thread safe, is that we don't have to clear the callback or the data anymore.

Comment 5

[Catalin Iordache]

Attachment: MakingDashboardThreadSafeV2.patch

So far I worked over socket, http and web socket data. After your feedback I've got rid of data.Clean() and callback = nullptr, because as you said it is thread safe. Also, as we discussed I've also got rid of GetWebSocket.. function so now for this part we only use RequestWebsocketConnections with a callback parameter.

I would continued this work, but I now realise that I've done some mess. I accidently intercalated DNS with DNS Lookup tool, so the code doesn't look good.

I will talk and  ask  Patrick if he can chekin dns lookup new patch and proxy settings test tool and after that I will finish this bug. 

Until that will happen, if there are other changes that you would want me to do, just ask me :)

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 794766 [details] [diff] [review]
MakingDashboardThreadSafeV2.patch

Review of attachment 794766 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure if this really needs to be re-entrant and fully parallel :)

might it be simpler just to annotate the various IDL-defined interfaces with 

{ 
 MutexAutoLock lock(mLock);
 if (mInProgress)
  return NS_ERROR_IN_PROGRESS
 mInProgress = true;
}

??

::: netwerk/base/src/Dashboard.cpp
@@ +28,5 @@
> +    {
> +        mMethod = method;
> +    }
> +
> +    NS_IMETHODIMP Run()

moz_override

@@ +40,5 @@
> +    P mParam;
> +};
> +
> +template<typename PtrType, typename Method, typename Param>
> +nsRunnable * NS_DashboardRunnable(PtrType * ptr, Method method, Param p)

you don't want the NS prefix.. NewDashboardRunnable() perhaps

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> I'm not sure if this really needs to be re-entrant and fully parallel :)

Any addon that might want to use the interface in the future would run into serious problems - and maybe prevent about:networking from working.

Is there any reason we wouldn't want to make this re-entrant?

Comment 8

[Patrick McManus [:mcmanus]]

I didn't say we shouldn't make it safe against concurrent access - my little check was meant to do that part.

you can go ahead with the patch if you like - I didn't read it line by line but t seems like a reasonable approach I would definitely merge. its just a lot of complexity in order to effectively make an about: page parallel - so I offered an alternative. but that's cool if you want it.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 794766 [details] [diff] [review]
MakingDashboardThreadSafeV2.patch

Review of attachment 794766 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. Wait until Bug 622728 lands (hopefully this week), then use NS_NewRunnableMethodWithArg instead of NS_DashboardRunnable and submit it for review.

::: netwerk/base/src/Dashboard.h
@@ +91,5 @@
>              return data.IndexOf(temp);
>          }
>          nsTArray<LogData> data;
>          mozilla::Mutex lock;
> +    }webSockData;

mWebSocketData would be better.

@@ +124,3 @@
>      struct DnsData mDns;
>      struct DnsLookup mDnsup;
>      struct ConnectionData mConn;

You should remove these.

@@ +132,5 @@
> +    nsresult GetHttpDispatch(struct HttpData);
> +    void GetDnsInfoDispatch();
> +    void StartTimer(uint32_t aTimeout);
> +    void StopTimer();
> +    nsresult TestNewConnection(const nsACString& aHost, uint32_t aPort,

const nsACString &aHost - so we're consistent

Comment 10

[Catalin Iordache]

Attachment: makingDashboardThreadSafeV3.patch

Comment 11

[Catalin Iordache]

Attachment: makingDashboardThreadSafeV4.patch

I've moved DnsLookup struct from dashboard.h in dashboard.cpp

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 805493 [details] [diff] [review]
makingDashboardThreadSafeV3.patch

Review of attachment 805493 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +41,2 @@
>  {
> +    struct SocketData mSocketData;

This is not a member field. Please call it simply socketData or something similar.

@@ +50,4 @@
>      gSocketTransportService->Dispatch(event, NS_DISPATCH_NORMAL);
>      return NS_OK;
>  }
>  

// Comment that this is called on the socket thread.
// Same for the methods below

::: netwerk/base/src/Dashboard.h
@@ +35,5 @@
>      NS_DECL_NSITIMERCALLBACK
>      NS_DECL_NSIDNSLISTENER
>  
>      Dashboard();
>      friend class DashConnStatusRunnable;

Remove this

@@ +41,4 @@
>  
>  private:
> +    bool mEnableLogging;
> +    

nit: Trailing white.

@@ +94,5 @@
>          nsTArray<LogData> data;
>          mozilla::Mutex lock;
>          nsCOMPtr<NetDashboardCallback> cb;
> +        nsIThread *thread;
> +    }mWebSocketData;

nit: Leave a space between } and the variable name.

@@ +135,5 @@
> +    /* Helper methods that pass the JSON to the callback function. */
> +    nsresult GetSockets(struct SocketData);
> +    nsresult GetHttpConnections(struct HttpData);
> +    nsresult GetDNSCacheEntries(struct DnsData);
> +    nsresult GetConnectionStatus(struct ConnStatus aStatus);

nit: no aStatus here. Just to keep things consistent.

Comment 13

[Catalin Iordache]

Attachment: makingDashboardThreadSafeV5.patch

Made changes which were specified in review

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 806119 [details] [diff] [review]
makingDashboardThreadSafeV5.patch

Review of attachment 806119 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from the issues with the comments, it seems pretty ok.
Fix those and ask Patrick for a final review (there might be things I missed)

::: netwerk/base/src/Dashboard.cpp
@@ +55,3 @@
>  
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<SocketData>(this, &Dashboard::GetSocketsDispatch, socketData);
> +    // Called on the socket thread

I meant comment the GetSocketsDispatch method to say that it's called on the socket thread.

@@ +69,5 @@
>      }
> +
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<SocketData>(this, &Dashboard::GetSockets, socketData);
> +    // Called on the socket thread
> +    socketData.thread->Dispatch(event, NS_DISPATCH_NORMAL);

This is wrong. socketData.thread will be the original != socket thread
Just remove the comment.

@@ +145,4 @@
>  {
> +    HttpInfo::GetHttpConnectionData(&httpData.data);
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<HttpData>(this, &Dashboard::GetHttpConnections, httpData);
> +    // Called on the socket thread

wrong! Remove this comment.

@@ +394,5 @@
> +    if (dnsData.serv)
> +        dnsData.serv->GetDNSCacheEntries(&dnsData.data);
> +
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<DnsData>(this, &Dashboard::GetDNSCacheEntries, dnsData);
> +    // Called on the socket thread

Remove this.

@@ +556,5 @@
>      if (NS_FAILED(rv)) {
>          ConnStatus status;
>          CopyASCIItoUTF16(GetErrorString(rv), status.creationSts);
> +        nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<ConnStatus>(this, &Dashboard::GetConnectionStatus, status);
> +        // Called on the socket thread

Remove this.

@@ +628,5 @@
>      ConnStatus status;
>      CopyASCIItoUTF16(GetErrorString(aStatus), status.creationSts);
>      nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<ConnStatus>(this, &Dashboard::GetConnectionStatus, status);
> +    // Called on the socket thread
> +    mConnectionData.thread->Dispatch(event, NS_DISPATCH_NORMAL);

Remove this.

@@ +647,5 @@
>  
>      ConnStatus status;
>      status.creationSts.Assign(NS_LITERAL_STRING("NS_ERROR_NET_TIMEOUT"));
>      nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<ConnStatus>(this, &Dashboard::GetConnectionStatus, status);
> +    // Called on the socket thread

Remove this.

Comment 15

[Catalin Iordache]

Attachment: makingDashboardThreadSafeV6.patch

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 806443 [details] [diff] [review]
makingDashboardThreadSafeV6.patch

Review of attachment 806443 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +30,5 @@
> +    nsCOMPtr<nsIDNSService> serv;
> +    nsCOMPtr<nsICancelable> cancel;
> +    nsCOMPtr<NetDashboardCallback> cb;
> +} dnsLookup;
> +

can you explain how we've got a global variable (struct) and that this makes the code threadsafe? Caching the service is one thing, but the cancel and cb members are per instance.

minimally it should be static and named gDNSLookup, but I suspect this needs to be dynamically allocated

@@ +38,5 @@
>  }
>  
>  Dashboard::~Dashboard()
>  {
> +    if (dnsLookup.cancel)

shouldn't you call stoptimer in the dtor too?

@@ +381,5 @@
>      }
>  
> +    nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<DnsData>(this, &Dashboard::GetDnsInfoDispatch, dnsData);
> +    // Called on the socket thread
> +    // gSocketTransportService->Dispatch(event, NS_DISPATCH_NORMAL);

why is this commented out?

::: netwerk/base/src/Dashboard.h
@@ +124,5 @@
> +    nsresult TestNewConnection(const nsACString &aHost, uint32_t aPort,
> +                               const char *aProtocol, uint32_t aTimeout);
> +
> +    /* Helper methods that pass the JSON to the callback function. */
> +    nsresult GetSockets(struct SocketData);

I know these are implemented with NS_GetRunnableMethodWithArg() and that function marshalls the structure across threads for you... but to do that it has to copy the whole thing to the stack, and in this (maybe all of your cases) that structure carries an array and so is probably bigger than we want to be doing that with.

can you please just dynamically allocate the structure and pass a pointer to the runnable method and have it free the struct up?

Comment 17

[Catalin Iordache]

Attachment: makingDashboardThreadSafeV7.patch

I've made dnsLookup struct static, hope it's ok. We want to declare this in dashboard.cpp because we want to keep dashboard.h more simpler - for the moment this doesn't make the code threadsafe.

Also now mConnectionData.timer is called in dtor and canceled

SocketData, HttpData and DnsData structures are now dinamically alocated.

Comment 18

[Patrick McManus [:mcmanus]]

thanks catalin - those changes were a lot of work. much better now.

why do you say that making the dnslookup state be part of the dashboard members, rather than static variables, would complicate dashboard.h?

that change seems to make this code less thread safe, rather than more :)

Comment 19

[Catalin Iordache]

I don't really know :). Should I live dnslookup struct the way it was (in dashboard.h) ?

Also, do you have other ideas to improve this patch, in order to make the dashboard more thread safe?

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Catalin Iordache from comment #19)
> I don't really know :). Should I live dnslookup struct the way it was (in
> dashboard.h) ?
> 

now I'm confused. I'm asking why you wrote the patch like that. who else would know?

> Also, do you have other ideas to improve this patch, in order to make the
> dashboard more thread safe?

Comment 21

[Catalin Iordache]

Sorry for that. I didn't want it to sound like this. My idea was that if someone would want to use our dashboard.h, dnslookup struct being part of a diagnostic tool wouldn't be for much help, so I thought it would be better to put it in dashboard.cpp. Like I was saying - this change would not make it more thread safe, but I didn't know it would make it worse.

I think it would be better to live it the way it was. Sorry again for misunderstanding.

Comment 22

[Catalin Iordache]

I'm also thinking that after I will get a review on proxy settings diagnostic tool patch, I should implement the dns lookup functionality directly from javascript - but I'm waiting to see how that patch would go.

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 807485 [details] [diff] [review]
makingDashboardThreadSafeV7.patch

Review of attachment 807485 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +31,5 @@
> +    {
> +        nsCOMPtr<nsIDNSService> serv;
> +        nsCOMPtr<nsICancelable> cancel;
> +        nsCOMPtr<NetDashboardCallback> cb;
> +    } gDNSLookup;

as discussed, this needs to be dynamically allocated on every call of asnycresolv() - as they each have a unique cancel and callback.

the service should just be removed from the structure and used as a local stack variable.

@@ +44,5 @@
>  {
> +    if (gDNSLookup.cancel)
> +        gDNSLookup.cancel->Cancel(NS_ERROR_ABORT);
> +
> +    if (mConnectionData.timer)

i'm sorry I didn't mention this sooner, but mConnectionData also needs to be allocated on a per transaction basis - right? Otherwise concurrent calls to RequestCnnection() step on each other.

Comment 24

[Patrick McManus [:mcmanus]]

the same problem appears to exist in for mWebSocketData.. anything which is state for an API call can't exist just one time (be it as a static, or as a member of a singleton service) - otherwise you couldn't have multiple concurrent API calls to the same service. (correct me if I'm wrong - but that's the point of the patch, right?)

Comment 25

[Valentin Gosu [:valentin] (he/him)]

(In reply to Patrick McManus [:mcmanus] from comment #24)
> the same problem appears to exist in for mWebSocketData.. anything which is
> state for an API call can't exist just one time (be it as a static, or as a
> member of a singleton service) - otherwise you couldn't have multiple
> concurrent API calls to the same service. (correct me if I'm wrong - but
> that's the point of the patch, right?)

I think mWebSocketData must stay, since that's where we accumulate events from websockets.
The other members, which we use for diagnostics, will be a bit harder to make "thread-safe" since we need to keep them around for a while. Ping for example, must persist until it reaches the connected state, or it times out.
I think we can fix this with some sort of request queue, or something like that, but maybe Catalin should focus on getting the basics be thread-safe for now. I think this might be causing our existing test to fail.

Comment 26

[Patrick McManus [:mcmanus]]

There is a stop-gap solution for the failing test in comment #6. You could use that while you work out the nitty gritty details of this patch.

Comment 27

[Valentin Gosu [:valentin] (he/him)]

Attachment: Patch

Comment 28

[Valentin Gosu [:valentin] (he/him)]

I have updated the patch with the latest format for the JSON result. I especially need review for the manual Addref-ing of nsRefPtr<LookupHelper> helper in Dashboard::RequestDNSLookup/LookupHelper::ConstructAnswer.

Thanks!

Comment 29

[Patrick McManus [:mcmanus]]

Comment on attachment 8347608 [details] [diff] [review]
Patch

Review of attachment 8347608 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +23,4 @@
>  namespace mozilla {
>  namespace net {
>  
> +class SocketData : public nsISupports

usual naming for all new classes please.. mFoo not foo for member vars

@@ +224,5 @@
> +NS_IMETHODIMP
> +LookupHelper::OnLookupComplete(nsICancelable *aRequest,
> +                               nsIDNSRecord *aRecord, nsresult aStatus)
> +{
> +    nsRefPtr<nsIDNSRecord> record = aRecord;

dont need that line anymore

@@ +228,5 @@
> +    nsRefPtr<nsIDNSRecord> record = aRecord;
> +    MOZ_ASSERT(aRequest == cancel);
> +    cancel = nullptr;
> +    status = aStatus;
> +

nsRefPtr<LookupArgument> arg = new LookupArgument(aRecord, this);

// the LookupArgument class should have 2 smart ptrs holding an nsIDNSRecord, and a LookupHelper

@@ +231,5 @@
> +    status = aStatus;
> +
> +     nsCOMPtr<nsIRunnable> event =
> +        NS_NewRunnableMethodWithArg<nsRefPtr<nsIDNSRecord> >
> +        (this, &LookupHelper::ConstructAnswer, record);

NS_NewRunnableMethodWithArg<nsRefPtr<LookupArgument> >
 (this, &LookupHelper::ConstructAnswer, arg);

@@ +677,5 @@
>  
> +    nsRefPtr<LookupHelper> helper = new LookupHelper();
> +    helper->cb = cb;
> +    helper->thread = NS_GetCurrentThread();
> +    helper->AddRef();

delete this addref line

@@ +684,2 @@
>      if (NS_FAILED(rv)) {
> +        helper->Release();

delete this release() line

::: netwerk/base/src/Dashboard.h
@@ +83,3 @@
>  
>      bool mEnableLogging;
> +    WebSocketData mWs;

this needs to live inside class websocketrequest right? If there is one global one for the dashboard it will get screwed up with concurrent requests to the service.

Comment 30

[Valentin Gosu [:valentin] (he/him)]

(In reply to Patrick McManus [:mcmanus] from comment #29)
> ::: netwerk/base/src/Dashboard.h
> @@ +83,3 @@
> >  
> >      bool mEnableLogging;
> > +    WebSocketData mWs;
> 
> this needs to live inside class websocketrequest right? If there is one
> global one for the dashboard it will get screwed up with concurrent requests
> to the service.

Actually, we use WebSocketData to collect information about the open websockets. It gets updated by calls to Dashboard::AddHost, NewMsgSent, ( http://mzl.la/1cRQeH4 ) and we use a mutex to access it. Do you think the name and type for mWs is too ambiguous?

Starting work on the other issues. Thanks

Comment 31

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make the Networking Dashboard thread safe

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 8384798 [details] [diff] [review]
Make the Networking Dashboard thread safe

Review of attachment 8384798 [details] [diff] [review]:
-----------------------------------------------------------------

see if you can improve the silly whitespace comments and then land it! r=mcmanus

::: netwerk/base/src/Dashboard.cpp
@@ +137,5 @@
> +    }
> +
> +    virtual ~ConnectionData()
> +    {
> +        if (mTimer)

nit .. trying to enforce brace syntax for even one liners now.. can you scan the whole patch for this?

if (mTimer) {
 Cancel();
}

@@ +160,5 @@
> +NS_IMPL_ISUPPORTS2(ConnectionData, nsITransportEventSink, nsITimerCallback)
> +
> +NS_IMETHODIMP
> +ConnectionData::OnTransportStatus(nsITransport *aTransport, nsresult aStatus,
> +                             uint64_t aProgress, uint64_t aProgressMax)

nit indent

Comment 33

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make the Networking Dashboard thread safe

Comment 34

[Valentin Gosu [:valentin] (he/him)]

Thanks!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3a8c5cbeccea

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a8c5cbeccea