1063318 - switch Chromium IPC code to Mozilla's ref/auto ptr types, fix thread safety bug

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Aas]

Attachment: fix v1.0

1) This removes scoped_refptr from our Chromium IPC code, replacing it with mozilla::RefPtr.

2) This removes some usage of scoped_ptr, replacing it with mozilla::UniquePtr, but does not remove the scoped_ptr definition. Chromium code uses scoped_ptr_malloc, which takes a custom free function, so we can't get rid of this stuff entirely until we have a replacement for scoped_ptr_malloc.

3) This fixes a multithreading bug caught by assertions in our non-thread-safe refcounting code. StoreRef refcounting was not thread-safe, and when I switched it to non-thread-safe Mozilla refcounting an assertion was raised that the object was deleted on a different thread from where it was created. The fix is to switch it to thread-safe refcounting.

Comment 1

[Andrew McCreight [:mccr8]]

>  Chromium code uses scoped_ptr_malloc, which takes a custom free function, so we can't get rid of this stuff entirely until we have a replacement for scoped_ptr_malloc.

Is the custom deleter argument D to UniquePtr insufficient?

Comment 2

[Josh Aas]

Forgot to mention that this also removes Chromium's RefCounted (and thread-safe version) code, replacing it with Mozilla stuff like NS_INLINE_DECL_THREADSAFE_REFCOUNTING.

Comment 3

[Josh Aas]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> >  Chromium code uses scoped_ptr_malloc, which takes a custom free function, so we can't get rid of this stuff entirely until we have a replacement for scoped_ptr_malloc.
> 
> Is the custom deleter argument D to UniquePtr insufficient?

Interesting, that might work. Originally I didn't know about UniquePtr and I was using nsAutoPtr, which doesn't have such functionality. I forgot to re-check when I swtiched to UniquePtr. Thanks! I'll probably take care of that in a follow-up patch.

Comment 4

[Josh Aas]

Attachment: fix v1.1

This knocks out Chromium's scoped_ptr entirely, used UniquePtr to replace scoped_array as well.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8484794 [details] [diff] [review]
fix v1.1

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

r=me with the changes below.  If the destructors called out in the review really do need to be virtual, that's fine, but the diff suggests that they don't.  (Tagging the classes as MOZ_FINAL would let the compiler check for you.)

Using MakeUnique in the places suggested is optional; I only mention it because you used MakeUnique in other places in the patch.  MakeUnique could also be used for the |new[]| calls if you were so inclined.

::: ipc/chromium/src/base/message_loop.cc
@@ +6,5 @@
>  
>  #include <algorithm>
>  
>  #include "mozilla/Atomics.h"
> +#include "mozilla/RefPtr.h"

Please use nsAutoPtr.h / nsRefPtr instead of mozilla/RefPtr.h throughout this patch.

::: ipc/chromium/src/base/message_pump_libevent.cc
@@ +272,5 @@
>    // to jump through the same hoops as WatchFileDescriptor().  Not
>    // needed at present
>    DCHECK(NULL == sigevent->event_);
>  
> +  mozilla::UniquePtr<event> evt(new event);

Use MakeUnique?

::: ipc/chromium/src/base/process_util_bsd.cc
@@ +225,5 @@
>                 const environment_map& env_vars_to_set,
>                 ChildPrivileges privs,
>                 bool wait, ProcessHandle* process_handle,
>                 ProcessArchitecture arch) {
> +  mozilla::UniquePtr<char*> argv_cstr(new char*[argv.size() + 1]);

This change looks incorrect: you're allocating the array with |new[]|, but you'll free it with |delete|.  The template type should be |char*[]|, shouldn't it?

::: ipc/chromium/src/base/process_util_linux.cc
@@ +207,5 @@
>                      const environment_map& env_vars_to_set,
>                      ChildPrivileges privs,
>                      ProcessHandle* process_handle) {
>    size_t i;
> +  mozilla::UniquePtr<char*> argv_cstr(new char*[argv.size() + 1]);

Likewise with the template type here.

@@ +213,5 @@
>      argv_cstr[i] = const_cast<char*>(argv[i].c_str());
>    }
>    argv_cstr[argv.size()] = nullptr;
>  
> +  mozilla::UniquePtr<char*> env_cstr(new char*[env_vars_to_set.size() + 1]);

Likewise with the template type here.

::: ipc/chromium/src/base/revocable_store.h
@@ +24,5 @@
>      void set_store(RevocableStore* store) { store_ = store; }
>      RevocableStore* store() const { return store_; }
>  
> +   protected:
> +    virtual ~StoreRef() {}

Why is this virtual?  Is StoreRef subclassed?

::: ipc/chromium/src/base/sys_info_win.cc
@@ +56,5 @@
>    DWORD value_length = GetEnvironmentVariable(var, NULL, 0);
>    if (value_length == 0) {
>      return L"";
>    }
> +  mozilla::UniquePtr<wchar_t> value(new wchar_t[value_length]);

Template type again.

::: ipc/chromium/src/base/waitable_event.h
@@ +155,5 @@
>      const bool manual_reset_;
>      bool signaled_;
>      std::list<Waiter*> waiters_;
> +   protected:
> +    virtual ~WaitableEventKernel() {}

Why does this need to be virtual?  Is WaitableEventKernel subclassed?

::: ipc/chromium/src/base/waitable_event_watcher_posix.cc
@@ +46,5 @@
>      return flag_;
>    }
>  
> + protected:
> +  virtual ~Flag() {}

Again with the virtual destructor question.

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ +372,5 @@
>  
>  bool Channel::ChannelImpl::EnqueueHelloMessage() {
> +  mozilla::UniquePtr<Message> msg(new Message(MSG_ROUTING_NONE,
> +                                              HELLO_MESSAGE_TYPE,
> +                                              IPC::Message::PRIORITY_NORMAL));

Use MakeUnique?

::: ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ +202,5 @@
>  
>  bool Channel::ChannelImpl::EnqueueHelloMessage() {
> +  mozilla::UniquePtr<Message> m(new Message(MSG_ROUTING_NONE,
> +                                            HELLO_MESSAGE_TYPE,
> +                                            IPC::Message::PRIORITY_NORMAL));

Use MakeUnique?

::: xpcom/build/nsXPComInit.cpp
@@ +527,1 @@
>        new BrowserProcessSubThread(BrowserProcessSubThread::IO));

Use MakeUnique?

Comment 6

[Josh Aas]

Attachment: fix v1.2

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d77733726cae

https://hg.mozilla.org/integration/mozilla-inbound/rev/d77733726cae

Comment 7

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

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/14fc678064b3 for b2g mochitest leaks: 

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=b2g_emulator_vm%20mozilla-inbound%20debug%20test%20mochitest-debug-&rev=d77733726cae

Comment 8

[Andrew McCreight [:mccr8]]

That's just an increase of 16 bytes, FWIW.  You should see what we leak before and after your patch to figure out if some data structure is just slightly bigger or we're actually leaking more.

Comment 9

[Andrew McCreight [:mccr8]]

(M9 is green because of bug 962871, M12 is green because of bug 1068280)

Comment 10

[Josh Aas]

It's more than 16 bytes. This is what's leaking with my patch that isn't leaking without it:

16:30:24     INFO -  TEST-INFO | leakcheck | leaked 2 MessagePump (16 bytes)
16:30:24     INFO -  TEST-INFO | leakcheck | leaked 1 StoreRef (8 bytes)
16:30:24     INFO -  TEST-INFO | leakcheck | leaked 2 WaitableEventKernel (40 bytes)

These are all objects I touched in the patch.

Comment 11

[Josh Aas]

Been thinking about this for a few minutes, looked over the patch again. There isn't much room for this patch to have caused new leaks unless there is some major difference in the pointer types that I'm not aware of.

My suspicion is that we were always leaking these objects, we just didn't know because the Chromium pointer types aren't instrumented for leak detection. If this is the case, what do we do about it? Can we have the expectations for the test changed?

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Josh Aas (Mozilla Corporation) from comment #11)
> If this is the case, what do we do about it? Can we have the
> expectations for the test changed?

The leak threshold is set here:
  http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#746

If you are sure these aren't additional leaks, then you can increase that amount and get khuey to r+ the patch.

Comment 13

[Josh Aas]

Attachment: fix v1.3

Upped leak threshhold by 64 bytes.

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/f5a2e2a16e84