988410 - updater xpchell tests assert due to off main thread use of directory service

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Dave Hylands [:dhylands]]

When the updater xpcshell tests are run (after bug 959327 is addressed) then I see the following crash:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 829.858]
0x40be4a46 in nsXPCWrappedJS::CallMethod (this=0x43016400, methodIndex=3, info=0x430d7f98, params=0x100ff790) at /home/work/B2G-emulator-arm/b2g-inbound/js/xpconnect/src/XPCWrappedJS.cpp:513
513	        MOZ_CRASH();
(gdb) bt
#0  0x40be4a46 in nsXPCWrappedJS::CallMethod (this=0x43016400, methodIndex=3, info=0x430d7f98, params=0x100ff790) at /home/work/B2G-emulator-arm/b2g-inbound/js/xpconnect/src/XPCWrappedJS.cpp:513
#1  0x405ec3e4 in PrepareAndDispatch (self=0x435403a0, methodIndex=<value optimized out>, args=0x100ff854)
    at /home/work/B2G-emulator-arm/b2g-inbound/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:93
#2  0x405ebb4c in SharedStub () from /home/work/B2G-emulator-arm/objdir-gecko-b-i/dist/bin/libxul.so
#3  0x405da4d0 in FindProviderFile (aElement=0x435403a0, aData=0x100ff884) at /home/work/B2G-emulator-arm/b2g-inbound/xpcom/io/nsDirectoryService.cpp:341
#4  0x405da5bc in nsDirectoryService::Get (this=0x40223100, prop=0x41828c7f "GreD", uuid=..., result=0x100ffbe4) at /home/work/B2G-emulator-arm/b2g-inbound/xpcom/io/nsDirectoryService.cpp:369
#5  0x40723aec in mozilla::ipc::GeckoChildProcessHost::PerformAsyncLaunchInternal (this=0x4350b8e0, aExtraOpts=<value optimized out>, arch=<value optimized out>)
    at /home/work/B2G-emulator-arm/b2g-inbound/ipc/glue/GeckoChildProcessHost.cpp:520
#6  0x40724126 in mozilla::ipc::GeckoChildProcessHost::PerformAsyncLaunch (this=0x4350b8e0, aExtraOpts=..., arch=base::PROCESS_ARCH_ARM)
    at /home/work/B2G-emulator-arm/b2g-inbound/ipc/glue/GeckoChildProcessHost.cpp:402
#7  0x407227d6 in mozilla::ipc::GeckoChildProcessHost::RunPerformAsyncLaunch (this=0x4350b8e0, aExtraOpts=..., aArch=base::PROCESS_ARCH_ARM)
    at /home/work/B2G-emulator-arm/b2g-inbound/ipc/glue/GeckoChildProcessHost.cpp:440
#8  0x4072281c in DispatchToMethod<mozilla::ipc::GeckoChildProcessHost, bool (mozilla::ipc::GeckoChildProcessHost::*)(std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, base::ProcessArchitecture), std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, base::ProcessArchitecture> (this=0x43d687e0)
    at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/tuple.h:400
#9  RunnableMethod<mozilla::ipc::GeckoChildProcessHost, bool (mozilla::ipc::GeckoChildProcessHost::*)(std::vector<std::string, std::allocator<std::string> >, base::ProcessArchitecture), Tuple2<std::vector<std::string, std::allocator<std::string> >, base::ProcessArchitecture> >::Run (this=0x43d687e0) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/task.h:307
#10 0x4071db38 in MessageLoop::RunTask (this=0x100ffde4, task=0x43d687c0) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:344
#11 0x4071e89a in MessageLoop::DeferOrRunPendingTask (this=0x43d687e0, pending_task=<value optimized out>) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:352
#12 0x4071f458 in MessageLoop::DoWork (this=0x100ffde4) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:430
#13 0x40715fce in base::MessagePumpLibevent::Run (this=0x40201790, delegate=0x100ffde4) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_pump_libevent.cc:311
#14 0x4071dafc in MessageLoop::RunInternal (this=0x43d687c0) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:226
#15 0x4071db7a in MessageLoop::RunHandler (this=0x100ffde4) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:219
#16 MessageLoop::Run (this=0x100ffde4) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/message_loop.cc:193
#17 0x4072058a in base::Thread::ThreadMain (this=0x402230c0) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/thread.cc:162
#18 0x407163a8 in ThreadFunc (closure=0x1) at /home/work/B2G-emulator-arm/b2g-inbound/ipc/chromium/src/base/platform_thread_posix.cc:39
#19 0x40056e4c in __thread_entry (func=0x407163a1 <ThreadFunc>, arg=0x402230c0, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217
#20 0x4005699c in pthread_create (thread_out=<value optimized out>, attr=0xbeced4f0, start_routine=0x407163a1 <ThreadFunc>, arg=0x402230c0) at bionic/libc/bionic/pthread.c:357
#21 0x00000000 in ?? ()

which is happening due to the directory service being overridden in JS (B2G does this) and being used off of the main thread.

Comment 1

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=2615c0c395b9

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Move directory service access back onto the main thread

emulator xpcshell updater tests fail without this patch.

Unfortunately, emulator xpchsell updater tests are currently off due to bug 959327

With this change, all of the test pass locally.

Once this has landed on m-c, I'll have another go at enabling updater tests and see if anything else fails.

Comment 3

[Dave Hylands [:dhylands]]

Attachment: Move directory service access back onto the main thread. v2

Made a few minor changes to get try greened up.

https://tbpl.mozilla.org/?tree=Try&rev=5898a896cff3

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8399600 [details] [diff] [review]
Move directory service access back onto the main thread. v2

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

This looks good to me!

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +64,5 @@
>  #endif
>    ;
>  
> +nsCOMPtr<nsIFile> GeckoChildProcessHost::sGreDir;
> +mozilla::DebugOnly<bool> GeckoChildProcessHost::sGreDirCached = false;

Nit: technically unnecessary since static data is zero'd.

@@ +135,3 @@
>  #ifdef OS_WIN
> +      nsString path;
> +      sGreDir->GetPath(path);

Bah, not your doing, but can you add a MOZ_ALWAYS_TRUE(NS_SUCCEEDED()) around that? And the GetNativePath three lines down?

@@ +273,5 @@
> +  // NS_GRE_DIR here and stash it.
> +
> +#ifdef MOZ_WIDGET_GONK
> +  // B2G overrides the directory service in JS, so this needs to be called
> +  // on the main thread.

It's actually not a gonk-specific problem. The firefox impl uses an unlocked hashtable so it's not safe to use it off the main thread anywhere...

@@ +554,5 @@
>    if (ShouldHaveDirectoryService()) {
> +    MOZ_ASSERT(sGreDirCached);
> +    if (sGreDir) {
> +      nsCString path;
> +      sGreDir->GetNativePath(path);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED()) here too.

::: ipc/glue/GeckoChildProcessHost.h
@@ +198,5 @@
>    // FIXME/cjones: this strongly indicates bad design.  Shame on us.
>    std::queue<IPC::Message> mQueue;
> +
> +  static void GetPathToBinary(FilePath& exePath);
> +  static void CacheGreDir();

Nit: Let's keep the methods separate from members, otherwise its easy to not notice one or the other when reading.

@@ +199,5 @@
>    std::queue<IPC::Message> mQueue;
> +
> +  static void GetPathToBinary(FilePath& exePath);
> +  static void CacheGreDir();
> +  static nsCOMPtr<nsIFile> sGreDir;

This should use StaticRefPtr to avoid static initializers.

Also, nsIFile should be forward-declared. Somehow you're getting it via #include seepage but that could change some day.

Comment 5

[Dave Hylands [:dhylands]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #4)
> @@ +273,5 @@
> > +  // NS_GRE_DIR here and stash it.
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > +  // B2G overrides the directory service in JS, so this needs to be called
> > +  // on the main thread.
> 
> It's actually not a gonk-specific problem. The firefox impl uses an unlocked
> hashtable so it's not safe to use it off the main thread anywhere...

I get try failures on the Mac platform if I add the assert.

So I'll file a followup bug to get the #ifdef removed and fix that the usage on the Mac. I don't currently have any way of investigating/debugging stuff on the Mac (other than by submitting to try).

Comment 6

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1b88de484816

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1b88de484816

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 8399600 [details] [diff] [review]
Move directory service access back onto the main thread. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not sure - happened many months ago
User impact if declined: none (although something could happen since updater tests have been disabled)
Testing completed (on m-c, etc.): updater tests are now running in master.
Risk to taking this patch (and alternatives if risky): Seems low
String or IDL/UUID changes made by this patch: None

Now that bug 994926 has landed, updater tests have been disabled on aurora (since mozharness is shared between master and aurora)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd93ca95b9b6