880167 - [Bluetooth] Replace SendDiscoveryTask by non-blocking implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

We need to replace SendDiscoveryTask with a non-blocking implementation in order to fix bug 853550.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation

For bug 853550.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 761313 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation

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

Thanks, Thomas. Please see the following comments for my feedback.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1795,5 @@
> +OnSendDiscoveryMessageReply(DBusMessage *aReply, void *aData)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  nsRefPtr<BluetoothReplyRunnable> runnable = static_cast<BluetoothReplyRunnable*>(aData);

Please use |dont_AddRef(static_cast< BluetoothReplyRunnable* >(aData))|, we shouldn't add reference count again here.

@@ +1798,5 @@
> +
> +  nsRefPtr<BluetoothReplyRunnable> runnable = static_cast<BluetoothReplyRunnable*>(aData);
> +  BluetoothValue v = true;
> +  nsAutoString errorStr;
> +  DispatchBluetoothReply(runnable, v, errorStr);

We could make it shorter as following:

DispatchBluetoothReply(runnable, BluetoothValue(true), EmptyString());

@@ +1815,5 @@
>      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
>      return NS_OK;
>    }
>  
> +  aRunnable->AddRef();

We can use a nsRefPtr<BluetoothReplyRunnable> here, so that we don't need to AddRef/Release manually.

Please see CreatePairedDeviceInternal() for more details.

@@ +1820,5 @@
> +  dbus_bool_t success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> +                                             OnSendDiscoveryMessageReply, aRunnable,
> +                                             NS_ConvertUTF16toUTF8(sAdapterPath).get(),
> +                                             DBUS_ADAPTER_IFACE, aMessageName,
> +                                             DBUS_TYPE_INVALID);

Currently, we use boolean as return type in other functions which called dbus_func_args_async. We may want to keep them consistent.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

Thanks for the review.

> > +OnSendDiscoveryMessageReply(DBusMessage *aReply, void *aData)
> > +{
> > +  MOZ_ASSERT(!NS_IsMainThread());
> > +
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = static_cast<BluetoothReplyRunnable*>(aData);
> 
> Please use |dont_AddRef(static_cast< BluetoothReplyRunnable* >(aData))|, we
> shouldn't add reference count again here.

Fixed.
 
> @@ +1798,5 @@
> > +
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = static_cast<BluetoothReplyRunnable*>(aData);
> > +  BluetoothValue v = true;
> > +  nsAutoString errorStr;
> > +  DispatchBluetoothReply(runnable, v, errorStr);
> 
> We could make it shorter as following:
> 
> DispatchBluetoothReply(runnable, BluetoothValue(true), EmptyString());

Fixed.

> @@ +1815,5 @@
> >      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> >      return NS_OK;
> >    }
> >  
> > +  aRunnable->AddRef();
> 
> We can use a nsRefPtr<BluetoothReplyRunnable> here, so that we don't need to
> AddRef/Release manually.
> 
> Please see CreatePairedDeviceInternal() for more details.

Fixed. I don't know why I used this awkward pattern. :)

> @@ +1820,5 @@
> > +  dbus_bool_t success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > +                                             OnSendDiscoveryMessageReply, aRunnable,
> > +                                             NS_ConvertUTF16toUTF8(sAdapterPath).get(),
> > +                                             DBUS_ADAPTER_IFACE, aMessageName,
> > +                                             DBUS_TYPE_INVALID);
> 
> Currently, we use boolean as return type in other functions which called
> dbus_func_args_async. We may want to keep them consistent.

Fixed. And the interface in DbusUtils.h needs a cleanup to meet our coding style. I intent do this when I know what I need here for the conversion to non-blocking code.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v2)

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 761986 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v2)

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1816,5 @@
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> +                                      OnSendDiscoveryMessageReply, aRunnable,

I think we should pass |(void*)runnable| as an argument instead of |aRunnable|, right?

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #5)
> Comment on attachment 761986 [details] [diff] [review]
> [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation
> (v2)
> 
> Review of attachment 761986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1816,5 @@
> >  
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > +
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > +                                      OnSendDiscoveryMessageReply, aRunnable,
> 
> I think we should pass |(void*)runnable| as an argument instead of
> |aRunnable|, right?

Well, yes. I fixed this for v3, although it's more a cosmetic change.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v3)

Comment 8

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 761986 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v2)

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

Overall looks good. Please have me review again after revision. Thanks!

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1815,5 @@
>    }
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,

We can use mConnection instead of gThreadConnection->GetConnection() since it's on the main thread.

@@ +1816,5 @@
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> +                                      OnSendDiscoveryMessageReply, aRunnable,

IMO, using (void*)aRunnable is fine. In addition, we can get rid of variable [runnable]. As you can see, all things that [runnable] do is add reference count of the object it holds at line 1817 but no one will release it. So it would cause memory leak.

Comment 9

[Eric Chou [:ericchou] [:echou]]

Oh, it seems that a new patch has been done while I was reviewing. Let me review again. :)

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE) from comment #8)
> Comment on attachment 761986 [details] [diff] [review]
> [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation
> (v2)
> 
> Review of attachment 761986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good. Please have me review again after revision. Thanks!
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1815,5 @@
> >    }
> >  
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > +
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> 
> We can use mConnection instead of gThreadConnection->GetConnection() since
> it's on the main thread.

Ok.

> @@ +1816,5 @@
> >  
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > +
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > +                                      OnSendDiscoveryMessageReply, aRunnable,
> 
> IMO, using (void*)aRunnable is fine. In addition, we can get rid of variable
> [runnable]. As you can see, all things that [runnable] do is add reference
> count of the object it holds at line 1817 but no one will release it. So it
> would cause memory leak.

aRunnable enters the method with a refcount of 1 and get's decremented in the caller afterwards., right? The DBus call runs asynchronously, so we need this extra reference for getting the runnable safely into the DBus callback. Maybe we need to release this reference after the call to DispatchBluetoothReply, although it crashed when I last tried.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 762518 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v3)

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

Almost the same as I commented for the last patch. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1813,5 @@
>      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
>      return NS_OK;
>    }
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;

We don't really need this variable. As you can see, the only thing that [runnable] does is to add reference count of the object it holds here, and calling forget() won't release it. So it would cause memory leak.

@@ +1815,5 @@
>    }
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> +
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,

We can use mConnection instead of gThreadConnection->GetConnection() since it's on the main thread.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE) from comment #11)
> Comment on attachment 762518 [details] [diff] [review]
> [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation
> (v3)
> 
> Review of attachment 762518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost the same as I commented for the last patch. :)
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1813,5 @@
> >      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> >      return NS_OK;
> >    }
> >  
> > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> 
> We don't really need this variable. As you can see, the only thing that
> [runnable] does is to add reference count of the object it holds here, and
> calling forget() won't release it. So it would cause memory leak.

Hmm, did you write this after reading my explanation in comment 11?

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> (In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE)
> from comment #11)
> > Comment on attachment 762518 [details] [diff] [review]
> > [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation
> > (v3)
> > 
> > Review of attachment 762518 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Almost the same as I commented for the last patch. :)
> > 
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +1813,5 @@
> > >      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> > >      return NS_OK;
> > >    }
> > >  
> > > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > 
> > We don't really need this variable. As you can see, the only thing that
> > [runnable] does is to add reference count of the object it holds here, and
> > calling forget() won't release it. So it would cause memory leak.
> 
> Hmm, did you write this after reading my explanation in comment 11?

Sorry, I meant comment 10.

Comment 14

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > (In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE)
> > from comment #11)
> > > Comment on attachment 762518 [details] [diff] [review]
> > > [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation
> > > (v3)
> > > 
> > > Review of attachment 762518 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Almost the same as I commented for the last patch. :)
> > > 
> > > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > > @@ +1813,5 @@
> > > >      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> > > >      return NS_OK;
> > > >    }
> > > >  
> > > > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > > 
> > > We don't really need this variable. As you can see, the only thing that
> > > [runnable] does is to add reference count of the object it holds here, and
> > > calling forget() won't release it. So it would cause memory leak.
> > 
> > Hmm, did you write this after reading my explanation in comment 11?
> 
> Sorry, I meant comment 10.

No, I wrote before reading it. :P

Comment 15

[Eric Chou [:ericchou] [:echou]]

> > @@ +1816,5 @@
> > >  
> > > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > > +
> > > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > > +                                      OnSendDiscoveryMessageReply, aRunnable,
> > 
> > IMO, using (void*)aRunnable is fine. In addition, we can get rid of variable
> > [runnable]. As you can see, all things that [runnable] do is add reference
> > count of the object it holds at line 1817 but no one will release it. So it
> > would cause memory leak.
> 
> aRunnable enters the method with a refcount of 1 and get's decremented in
> the caller afterwards., right? 

Ah, the main problem is that caller (in ipc/BluetoothParent.cpp) will not decrease the ref count. It's held as a member variable of BluetoothRequestParent.

> The DBus call runs asynchronously, so we need
> this extra reference for getting the runnable safely into the DBus callback.
> Maybe we need to release this reference after the call to
> DispatchBluetoothReply, although it crashed when I last tried.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE) from comment #15)
> > > @@ +1816,5 @@
> > > >  
> > > > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > > > +
> > > > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > > > +                                      OnSendDiscoveryMessageReply, aRunnable,
> > > 
> > > IMO, using (void*)aRunnable is fine. In addition, we can get rid of variable
> > > [runnable]. As you can see, all things that [runnable] do is add reference
> > > count of the object it holds at line 1817 but no one will release it. So it
> > > would cause memory leak.
> > 
> > aRunnable enters the method with a refcount of 1 and get's decremented in
> > the caller afterwards., right? 
> 
> Ah, the main problem is that caller (in ipc/BluetoothParent.cpp) will not
> decrease the ref count. It's held as a member variable of
> BluetoothRequestParent.

Oh, I see. But I was looking at BluetoothAdapter::StartStopDiscovery, which apparently releases its reference to the runnable. Or is there some magic that I don't see?

Comment 17

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> (In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE)
> from comment #15)
> > > > @@ +1816,5 @@
> > > > >  
> > > > > +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;
> > > > > +
> > > > > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 0,
> > > > > +                                      OnSendDiscoveryMessageReply, aRunnable,
> > > > 
> > > > IMO, using (void*)aRunnable is fine. In addition, we can get rid of variable
> > > > [runnable]. As you can see, all things that [runnable] do is add reference
> > > > count of the object it holds at line 1817 but no one will release it. So it
> > > > would cause memory leak.
> > > 
> > > aRunnable enters the method with a refcount of 1 and get's decremented in
> > > the caller afterwards., right? 
> > 
> > Ah, the main problem is that caller (in ipc/BluetoothParent.cpp) will not
> > decrease the ref count. It's held as a member variable of
> > BluetoothRequestParent.
> 
> Oh, I see. But I was looking at BluetoothAdapter::StartStopDiscovery, which
> apparently releases its reference to the runnable. Or is there some magic
> that I don't see?

You got it. BleutoothAdapter is running on child process. The BluetoothVoidReplyRunnable created in BluetoothAdapter.cpp would stay alive until its corresponding object (mReplyRunnable) running.

Check class BluetoothRequestParent::ReplyRunnable at the top of BluetoothParent.cpp and you'll find out how the ipc mechanism recycles allocated objects. :)

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v4)

Ok, I see. I fixed this in the v4.

Comment 19

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 762566 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v4)

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

Looks good. Thanks!

Comment 20

[Gina Yeh [:gyeh] [:ginayeh]]

Hi Thomas,

After discussion with Eric, we changed our mind. The key point is that we must add reference count of |aRunnable| before passing it into dbus call, and decrease the reference count in the callback function. Or, the runnable might be destroyed before execution. 

Here's an example. After making a dbus call, the BluetoothParentRequest is destroyed somehow, and then the reference count of runnable is decreased to zero and it is recycled. After that, the callback of dbus call is revoked and we try to access the runnable again. As a result, something bad happened. :(

Comment 21

[Gina Yeh [:gyeh] [:ginayeh]]

I'm going to review based on v3 patch since it's closer to the idea in comment 20.

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ok, better to sort out these problems now than run into dubious crashes later. I'll supply an updated patch on Monday. :)

Comment 23

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 762518 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v3)

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

Please update based on v3 patch. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1796,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  BluetoothReplyRunnable* runnable =
> +    static_cast<BluetoothReplyRunnable*>(aData);

No need to add reference count here. Please use |nsRefPtr<BluetoothReplyRunnable*> runnable = dont_AddRef(static_cast< BluetoothReplyRunnable* >(aData))|.

After leaving this function, |runnable| is null out and the reference count is decreased by 1.

@@ +1813,5 @@
>      DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
>      return NS_OK;
>    }
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable = aRunnable;

The reference count is increased by 1 since the assignment. It can make sure that the runnable won't be destroyed before the callback is invoked.

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v5)

Comment 25

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 763459 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v5)

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1798,5 @@
> +
> +  nsRefPtr<BluetoothReplyRunnable> runnable =
> +    dont_AddRef<BluetoothReplyRunnable>(static_cast<BluetoothReplyRunnable*>(aData));
> +
> +  DispatchBluetoothReply(runnable.get(), BluetoothValue(true), EmptyString());

I think we can use |runnable| directly.

@@ +1815,5 @@
>    }
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable(aRunnable);
> +
> +  bool success = dbus_func_args_async(mConnection, 0,

Can we set timeout to "-1"? Then the default timeout will be applied.

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v6)

Comment 27

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 763567 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v6)

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

Looks good to me. Thanks.

Comment 28

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 763567 [details] [diff] [review]
[01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v6)

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

Thanks for your patience. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1799,5 @@
> +  nsAutoString errorStr;
> +
> +  if (!aReply) {
> +    errorStr.AssignLiteral("SendDiscovery failed");
> +  }

Nice catch! I forgot this part in the previous review.

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks everyone. :)

https://hg.mozilla.org/projects/birch/rev/1f0d54745b33

Comment 30

[Ed Morley [:emorley]]

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