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]

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).

Can I work in this bug?

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

Ok, I'll start studying the code

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.

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.

i would to work on this bug

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

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

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?

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

Comment on attachment 8503723 [details] [diff] [review] Fix of the bug ># HG changeset patch ># Parent c84951e03eb5b9e9daa5e0c961df8d378b633539 ># User felipecesar00 <felipe.cesar00@gmail.com> >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 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.

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 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.

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

Thanks for the feedback Josh :).

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?

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

Comment on attachment 8504407 [details] [diff] [review] correction3-1.patch Review of attachment 8504407 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!

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?

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.

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 :).

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".

Thanks for fixing this, Felipe!

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!