Closed Bug 1063318 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: IPC, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #8484726 - Flags: review?(bent.mozilla)
>  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?
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.
(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.
Attached patch fix v1.1 (obsolete) — Splinter Review
This knocks out Chromium's scoped_ptr entirely, used UniquePtr to replace scoped_array as well.
Attachment #8484726 - Attachment is obsolete: true
Attachment #8484726 - Flags: review?(bent.mozilla)
Attachment #8484794 - Flags: review?(bent.mozilla)
Attachment #8484794 - Flags: review?(bent.mozilla) → review?(nfroyd)
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?
Attachment #8484794 - Flags: review?(nfroyd) → review+
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.
(M9 is green because of bug 962871, M12 is green because of bug 1068280)
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.
Flags: needinfo?(joshmoz)
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?
(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.
Attached patch fix v1.3Splinter Review
Upped leak threshhold by 64 bytes.
Attachment #8491856 - Attachment is obsolete: true
Attachment #8491961 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/f5a2e2a16e84
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.