1080017 - Geolocation tests send messages during shutdown

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Andrew McCreight [:mccr8]]

dom/tests/unit/test_geolocation_timeout_wrap.js and dom/tests/unit/test_geolocation_reset_accuracy_wrap.js hit the MsgDropped case in ContentChild::ProcessingError().

It looks like we're destroying the Geolocation object in shutdown, then it decides to tell its parent process something, but we're late in shutdown so it doesn't work.

2  libxul.so!mozilla::dom::ContentChild::ProcessingError(mozilla::ipc::HasResultCodes::Result) [ContentChild.cpp:ee8765fbabcd : 1650 + 0x1e]
3  libxul.so!mozilla::dom::PContentChild::OnProcessingError(mozilla::ipc::HasResultCodes::Result) [PContentChild.cpp:ee8765fbabcd : 5679 + 0xb]
4  libxul.so!mozilla::ipc::MessageChannel::ReportConnectionError(char const*) const [MessageChannel.cpp:ee8765fbabcd : 1565 + 0x7]
5  libxul.so!mozilla::ipc::MessageChannel::Send(IPC::Message*) [MessageChannel.cpp:ee8765fbabcd : 463 + 0xe]
6  libxul.so!mozilla::dom::PContentChild::SendSetGeolocationHigherAccuracy(bool const&) [PContentChild.cpp:ee8765fbabcd : 2097 + 0xb]
7  libxul.so!nsGeolocationService::UpdateAccuracy(bool) [nsGeolocation.cpp:ee8765fbabcd : 921 + 0x10]
8  libxul.so!mozilla::dom::Geolocation::Shutdown() [nsGeolocation.cpp:ee8765fbabcd : 1073 + 0x11]
9  libxul.so!mozilla::dom::Geolocation::~Geolocation() [nsGeolocation.cpp:ee8765fbabcd : 1026 + 0x8]
10  libxul.so!mozilla::dom::Geolocation::DeleteCycleCollectable() [nsGeolocation.cpp:ee8765fbabcd : 1011 + 0x8]
11  libxul.so!SnowWhiteKiller::~SnowWhiteKiller() [nsCycleCollector.cpp:ee8765fbabcd : 2624 + 0xd]
12  libxul.so!nsCycleCollector::FreeSnowWhite(bool) [nsCycleCollector.cpp:ee8765fbabcd : 2796 + 0x8]
13  libxul.so!nsCycleCollector::Shutdown() [nsCycleCollector.cpp:ee8765fbabcd : 3789 + 0x9]
14  libxul.so!nsCycleCollector_shutdown() [nsCycleCollector.cpp:ee8765fbabcd : 4250 + 0xf]
15  libxul.so!mozilla::ShutdownXPCOM(nsIServiceManager*) [XPCOMInit.cpp:ee8765fbabcd : 944 + 0x4]

Comment 1

[Josh Matthews [:jdm]]

This isn't hard to fix - add an IsAlive method to ContentChild (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.h), make ContentChild::IsAlive return a boolean that is false after ContentChild::ActorDestroy is called, and check that the content child is alive in nsGeolocationService::UpdateAccuracy (http://mxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp).

Comment 2

[Felipe César]

Can I work in this bug?

Comment 3

[Josh Matthews [:jdm]]

Yes! Please ask questions if anything in my description is unclear. mccr8, what's the easiest way to reproduce the error you see?

Comment 4

[Felipe César]

Ok, I'll start studying the code

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: make errors show up more easily

Probably the easiest thing to do is to apply this patch.  This appears to make the error fatal on XPCShell.  Then you want to run one of those tests I mention in comment 0.  I'm not sure exactly how to do that.

Comment 6

[Josh Matthews [:jdm]]

You can do that with ./mach xpcshell-test dom/test/unit/test_geolocation_timeout_wrap.js . Note that if you're on OS X, the test won't run by default unless you modify the xpcshell.ini in the same directory.

Comment 7

[m.sameer017]

i would to work on this bug

Comment 8

[Josh Matthews [:jdm]]

Please read bug comments carefully when investigating them - Felipe has already started working on this once.

Comment 9

[Felipe César]

How can I apply this patch sent by mccr8? Is there any command?

Comment 10

[Felipe César]

Analyzing the code of how to implement IsAlive, I thinking that I need to create a static boolean variable in ContentChild to stores if it's alive or not. So, the constructor of the class will set the boolean to true and when ActorDestroy was called it will set to false. Is there already a variable that is used to do a similar work?

Comment 11

[Felipe César]

Attachment: Fix of the bug

Here's my patch. Apparently, it worked fine on the tests

Comment 12

[Felipe César]

Comment on attachment 8503723 [details] [diff] [review]
Fix of the bug

># HG changeset patch
># Parent c84951e03eb5b9e9daa5e0c961df8d378b633539
># User felipecesar00 <[e-mail]>
>diff --git a/dom/geolocation/nsGeolocation.cpp b/dom/geolocation/nsGeolocation.cpp
>--- a/dom/geolocation/nsGeolocation.cpp
>+++ b/dom/geolocation/nsGeolocation.cpp
>@@ -912,19 +912,23 @@ nsGeolocationService::HighAccuracyReques
> }
> 
> void
> nsGeolocationService::UpdateAccuracy(bool aForceHigh)
> {
>   bool highRequired = aForceHigh || HighAccuracyRequested();
> 
>   if (XRE_GetProcessType() == GeckoProcessType_Content) {
>-    ContentChild* cpc = ContentChild::GetSingleton();
>-    cpc->SendSetGeolocationHigherAccuracy(highRequired);
>-    return;
>+  	if(Content::Child::IsAlive() == true) {
>+  		ContentChild* cpc = ContentChild::GetSingleton();
>+    	cpc->SendSetGeolocationHigherAccuracy(highRequired);
>+  	}
>+  	return;	
>+    
>   }
> 
>   if (!mHigherAccuracy && highRequired) {
>       mProvider->SetHighAccuracy(true);
>   }
> 
>   if (mHigherAccuracy && !highRequired) {
>       mProvider->SetHighAccuracy(false);
>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp
>@@ -584,16 +584,18 @@ ContentChild::Init(MessageLoop* aIOLoop,
>     SendBackUpXResources(FileDescriptor(xSocketFd));
> #endif
> 
> #ifdef MOZ_CRASHREPORTER
>     SendPCrashReporterConstructor(CrashReporter::CurrentThreadId(),
>                                   XRE_GetProcessType());
> #endif
> 
>+    static bool isAlive;
>+
>     GetCPOWManager();
> 
>     SendGetProcessAttributes(&mID, &mIsForApp, &mIsForBrowser);
>     InitProcessAttributes();
> 
>     return true;
> }
> 
>@@ -610,16 +612,18 @@ ContentChild::InitProcessAttributes()
>     if (mIsForApp && !mIsForBrowser) {
>         SetProcessName(NS_LITERAL_STRING("(Preallocated app)"), false);
>     } else {
>         SetProcessName(NS_LITERAL_STRING("Browser"), false);
>     }
> #else
>     SetProcessName(NS_LITERAL_STRING("Web Content"), true);
> #endif
>+
>+    isAlive = true;
> }
> 
> void
> ContentChild::SetProcessName(const nsAString& aName, bool aDontOverride)
> {
>     if (!mCanOverrideProcessName) {
>         return;
>     }
>@@ -648,16 +652,23 @@ ContentChild::SetProcessName(const nsASt
> }
> 
> void
> ContentChild::GetProcessName(nsAString& aName)
> {
>     aName.Assign(mProcessName);
> }
> 
>+bool
>+ContentChild::IsAlive()
>+{
>+    return isAlive;
>+}
>+
> void
> ContentChild::GetProcessName(nsACString& aName)
> {
>     aName.Assign(NS_ConvertUTF16toUTF8(mProcessName));
> }
> 
> /* static */ void
> ContentChild::AppendProcessId(nsACString& aName)
>@@ -1621,16 +1632,18 @@ ContentChild::ActorDestroy(ActorDestroyR
>     mIdleObservers.Clear();
> 
>     nsCOMPtr<nsIConsoleService> svc(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>     if (svc) {
>         svc->UnregisterListener(mConsoleListener);
>         mConsoleListener->mChild = nullptr;
>     }
> 
>+    isAlive = false;
>+
>     XRE_ShutdownChildProcess();
> }
> 
> void
> ContentChild::ProcessingError(Result what)
> {
>     switch (what) {
>     case MsgDropped:
>diff --git a/dom/ipc/ContentChild.h b/dom/ipc/ContentChild.h
>--- a/dom/ipc/ContentChild.h
>+++ b/dom/ipc/ContentChild.h
>@@ -86,16 +86,18 @@ public:
> 
>     const AppInfo& GetAppInfo() {
>         return mAppInfo;
>     }
> 
>     void SetProcessName(const nsAString& aName, bool aDontOverride = false);
>     void GetProcessName(nsAString& aName);
>     void GetProcessName(nsACString& aName);
>+    bool isAlive;
>+    bool IsAlive();
>     static void AppendProcessId(nsACString& aName);
> 
>     ContentBridgeParent* GetLastBridge() {
>         MOZ_ASSERT(mLastBridge);
>         ContentBridgeParent* parent = mLastBridge;
>         mLastBridge = nullptr;
>         return parent;
>     }

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8503723 [details] [diff] [review]
Fix of the bug

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

This is a good start! Did you figure out how to apply mccr8's patch and reproduce the problem? That's a good way to check if your changes are having an effect.

::: dom/geolocation/nsGeolocation.cpp
@@ +916,5 @@
>  {
>    bool highRequired = aForceHigh || HighAccuracyRequested();
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +  	if(Content::Child::IsAlive() == true) {

This won't compile, so I suspect that this code has not actually been compiled. We don't need a static method; we can use the result of GetSingleton without any problems. We just can't call methods that cause inter-process communication like SendSetGeolocationHigherAccuracy.

@@ +918,5 @@
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +  	if(Content::Child::IsAlive() == true) {
> +  		ContentChild* cpc = ContentChild::GetSingleton();
> +    	cpc->SendSetGeolocationHigherAccuracy(highRequired);

nit: please indent using spaces, not tabs.

@@ +921,5 @@
> +  		ContentChild* cpc = ContentChild::GetSingleton();
> +    	cpc->SendSetGeolocationHigherAccuracy(highRequired);
> +  	}
> +  	return;	
> +    

nit: please remove the trailing whitespace here

::: dom/ipc/ContentChild.cpp
@@ +588,5 @@
>      SendPCrashReporterConstructor(CrashReporter::CurrentThreadId(),
>                                    XRE_GetProcessType());
>  #endif
>  
> +    static bool isAlive;

This isn't necessary.

@@ +617,5 @@
>  #else
>      SetProcessName(NS_LITERAL_STRING("Web Content"), true);
>  #endif
> +
> +    isAlive = true;

This should be initialized in the constructor.

::: dom/ipc/ContentChild.h
@@ +90,5 @@
>  
>      void SetProcessName(const nsAString& aName, bool aDontOverride = false);
>      void GetProcessName(nsAString& aName);
>      void GetProcessName(nsACString& aName);
> +    bool isAlive;

Please move this declaration to the same places as other variables, and call it mIsAlive to follow the existing naming style.

Comment 14

[Felipe César]

Attachment: correction2.patch

I still trying to learn these Mercurial commands correctly (https://developer.mozilla.org/en-US/docs/Mercurial_Queues). But, to submit the second trial quickly I applied mccr8's patch manually and didn't occur critical errors.

Here's my second trial. Seems to work.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8503785 [details] [diff] [review]
correction2.patch

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

Almost there! For future reference, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style is the style that we attempt to follow when writing and reviewing Firefox patches.

::: dom/geolocation/nsGeolocation.cpp
@@ +917,5 @@
>    bool highRequired = aForceHigh || HighAccuracyRequested();
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>      ContentChild* cpc = ContentChild::GetSingleton();
> +    if(cpc->IsAlive() == true) {

nit: space before ( and no need for == true.

@@ +921,5 @@
> +    if(cpc->IsAlive() == true) {
> +      cpc->SendSetGeolocationHigherAccuracy(highRequired);
> +    }
> +    return; 
> +    

nit: there's still trailing whitespace on this and the previous line.

::: dom/ipc/ContentChild.cpp
@@ +524,5 @@
>  {
>      // This process is a content process, so it's clearly running in
>      // multiprocess mode!
>      nsDebugImpl::SetMultiprocessMode("Child");
> +    mlsAlive = true;

This can move into the initializer list right above the actual body of the method.

@@ +656,5 @@
>  
> +bool
> +ContentChild::IsAlive()
> +{
> +  return mlsAlive;

nit: please match the indentation of the other methods; ie. 4 spaces.

::: dom/ipc/ContentChild.h
@@ +55,5 @@
>      typedef mozilla::dom::ClonedMessageData ClonedMessageData;
>      typedef mozilla::ipc::OptionalURIParams OptionalURIParams;
>      typedef mozilla::ipc::PFileDescriptorSetChild PFileDescriptorSetChild;
>      typedef mozilla::ipc::URIParams URIParams;
> +    bool mlsAlive;

nit: please move this down with the other variables at the bottom of the file.

Comment 16

[Josh Matthews [:jdm]]

Oh, and please mark previous patches as obsolete when submitting new ones so it's clear what the current work is.

Comment 17

[Felipe César]

Attachment: correction3.patch

Thanks for the feedback Josh :).

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8504312 [details] [diff] [review]
correction3.patch

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

I was about to give this a review+ until I looked closely at the variable name. I swear the next version will be good.

::: dom/ipc/ContentChild.h
@@ +420,5 @@
>  
>      bool mIsForApp;
>      bool mIsForBrowser;
>      bool mCanOverrideProcessName;
> +    bool mlsAlive;

This.. is a lower-case L, isn't it? Can we rename this?

Comment 19

[Felipe César]

Attachment: correction3-1.patch

Wow. Good eyes! I haven't noticed. Here's the patch with the variable renamed :)

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 8504407 [details] [diff] [review]
correction3-1.patch

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

Ship it!

Comment 21

[Felipe César]

Thank you very much for all your patience and help to a newbie, Josh :)

I was searching if I have to do any more steps and, as found here (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch), I put "checkin-needed" and changed the "assigned to". Is anything missing?

Comment 22

[Josh Matthews [:jdm]]

Those are the right steps, but we'll need to demonstrate a successful tryserver push before the checkin-needed will be accepted. You can apply for access at https://www.mozilla.org/hacking/committer/, and in the meantime I'll push this to the tryserver for you.

Comment 23

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=3632cb3b86ca

Comment 24

[Felipe César]

Understood. Next time I'll fill out the form. Seems that the build was successfully. I'll tag again as "checkin-needed". Again, thank you :).

Comment 25

[Josh Matthews [:jdm]]

Oh, one last step you can take is to provide a commit message in your patch in the form "Bug 123456 - Short description of changes. r=jdm".

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcb210f36bc

Comment 27

[Andrew McCreight [:mccr8]]

Thanks for fixing this, Felipe!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Oh, I meant to say when I pushed this. I had to give your patch a commit message and fix your name in your hg commit information. Please make sure your future patches have that information properly included for future patches. The links below can give you some pointers for that.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Thanks for the patch!

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/0dcb210f36bc