[Bluetooth] Replace SendDiscoveryTask by non-blocking implementation

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 5 obsolete attachments)

We need to replace SendDiscoveryTask with a non-blocking implementation in order to fix bug 853550.
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.
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.
Attachment #761313 - Attachment is obsolete: true
Attachment #761313 - Flags: review?(gyeh)
Attachment #761313 - Flags: review?(echou)
Attachment #761986 - Flags: review?(gyeh)
Attachment #761986 - Flags: review?(echou)
Attachment #761986 - Attachment description: [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation → [01] Bug 880167: Replace SendDiscoveryTask by non-blocking implementation (v2)
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?
(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.
Attachment #761986 - Attachment is obsolete: true
Attachment #761986 - Flags: review?(gyeh)
Attachment #761986 - Flags: review?(echou)
Attachment #762518 - Flags: review?(gyeh)
Attachment #762518 - Flags: review?(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.
Attachment #761986 - Attachment is obsolete: false
Oh, it seems that a new patch has been done while I was reviewing. Let me review again. :)
Attachment #761986 - Attachment is obsolete: true
(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 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.
(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?
(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.
(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
> > @@ +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.
(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?
(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. :)
Ok, I see. I fixed this in the v4.
Attachment #762518 - Attachment is obsolete: true
Attachment #762518 - Flags: review?(gyeh)
Attachment #762518 - Flags: review?(echou)
Attachment #762566 - Flags: review?(gyeh)
Attachment #762566 - Flags: review?(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!
Attachment #762566 - Flags: review?(echou) → review+
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. :(
I'm going to review based on v3 patch since it's closer to the idea in comment 20.
Ok, better to sort out these problems now than run into dubious crashes later. I'll supply an updated patch on Monday. :)
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.
Attachment #762566 - Attachment is obsolete: true
Attachment #762566 - Flags: review?(gyeh)
Attachment #763459 - Flags: review?(gyeh)
Attachment #763459 - Flags: review?(echou)
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.
Attachment #763459 - Attachment is obsolete: true
Attachment #763459 - Flags: review?(gyeh)
Attachment #763459 - Flags: review?(echou)
Attachment #763567 - Flags: review?(gyeh)
Attachment #763567 - Flags: review?(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.
Attachment #763567 - Flags: review?(echou) → review+
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.
Attachment #763567 - Flags: review?(gyeh) → review+
Thanks everyone. :)

https://hg.mozilla.org/projects/birch/rev/1f0d54745b33
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/1f0d54745b33
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.