977995 - B2G: remove mNetdWorker from SystemWorkerManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #864931 +++

NetworkService no longer owns a ChromeWorker.  No one is really connected to NetworkService via nsISystemWorkerManager.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

try all: https://tbpl.mozilla.org/?tree=Try&rev=7f90e4693a66

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Based on bug 975813.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8383524 [details] [diff] [review]
patch

Busted B2G xpcshell tests.  Got to fix test_networkstats_service.js first.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

There are multiple defects in NetworkWorker and the related parts since the C++ rewrite.  1) NetworkService holds a reference to NetworkWorker and never releases it.  It has to wait until the cycle collector comes up to resolve their ownership loop and free NetworkWorker manually.  However 2) nsINetworkWorker::shutdown is never called, and that leaves everything living till the end, inclusive of that gNetdConsumer in Netd.cpp.  3) when GC comes to free NetworkWorker, it calls its parent destructor ~NetConsumer(), which in turn calls ~RefCounted<NetdConsumer>().  Having a valid gNetdConsumer in Netd.cpp follows its refCnt is not zero and this triggers an assertion in ~RefCounted<NetdConsumer>().

So, some obvious treatments here.  A) NetworkService should call nsINetworkWorker::shutdown upon receiving a shutdown observer event and release the reference to NetworkWorker.  B) NetworkWorker should never be double ref-counted.  Move NetdConsumer implementation into a separated class.

Passed local dbg-xpcshell.  Full try: https://tbpl.mozilla.org/?tree=Try&rev=506f32bcfcc2

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8403195 [details] [diff] [review]
patch

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

Thanks for your help to fix this. Nice work. 

According to your explanation, I am wondering why removing netd code from systemWorkerManager causes xpcshell test fail. 
Maybe kyle can help to give us the answer ? :-)

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8403195 [details] [diff] [review]
patch

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

::: dom/system/gonk/NetworkWorker.cpp
@@ +177,5 @@
>  
>    nsresult rv;
>  
> +  nsCOMPtr<nsIThread> workerThread;
> +  rv = NS_NewThread(getter_AddRefs(workerThread));

NS_NewNamedThread please.  And just assign directly into gWorkerThread.

@@ +203,4 @@
>    StopNetd();
>  
> +  gWorkerThread->Shutdown();
> +  gWorkerThread = nullptr;

You should also be able to replace all the ClearOnShutdown calls in this file with code to null out pointers here.  Feel free to do that in another bug.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Vincent Chang[:vchang] from comment #5)
> Comment on attachment 8403195 [details] [diff] [review]
> patch
> 
> Review of attachment 8403195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your help to fix this. Nice work. 
> 
> According to your explanation, I am wondering why removing netd code from
> systemWorkerManager causes xpcshell test fail. 
> Maybe kyle can help to give us the answer ? :-)

What xpcshell test failure?  The try link in comment 4 looks fine.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

> What xpcshell test failure?  The try link in comment 4 looks fine.

test_networkstats_service.js mentioned in comment 3.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is that not what part #3 of comment 4 fixes?

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Huey [:khuey] ([e-mail]) (Away 4/19-5/7) from comment #9)
> Is that not what part #3 of comment 4 fixes?

The first revision patch, attachment 8383524 [details] [diff] [review], crashed debug build at the end of xpcshell tests (results in comment 1).  Then I have a second revision, attachment 8403195 [details] [diff] [review], to address that crash (comment 4).

So Vincent is wondering, why does "removing additional reference from NetworkService and SystemWorkerManager in that first revision" result in the crash.  We all know the near cause of that crash as I've described in comment 4, but we don't know the whole picture, eg. when does every refCnt of all the involved instances get decreased?  I don't have a satisfying answer for him.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

Address review comments.  I'll file a follow-up for ClearOnShutdown stuff because I think that will touch object references and it would be better have its own bug for further discuss & review.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9df1bf74a0c6

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

My guess is that if you never call Shutdown on the NetworkWorker we would not call StopNetd() which means that the destructor would not run until static destructors run and gNetDConsumer is destroyed.  I wouldn't be surprised if destroying objects that late causes crashes in debug builds (where we try to do things like leak tracking, cycle collected objects assert if they are destroyed too late, etc).

This is just speculation though ... looking at the stack of the failure and poking around with a debugger would get you much further.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9df1bf74a0c6

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> I'll file a follow-up for ClearOnShutdown stuff
> because I think that will touch object references and it would be better
> have its own bug for further discuss & review.

Filed as bug 995525.

Comment 16

[Vincent Chen [:vichen]]

Bug 988132 is fixed after apply the patch. Try result: https://tbpl.mozilla.org/?tree=Try&rev=7e212dae6207
No crash found in B2G emulator reftests.