Closed Bug 962310 Opened 6 years ago Closed 6 years ago

Nfc Handover: SendFile BT API crashes

Categories

(Firefox OS Graveyard :: Bluetooth, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.3T verified, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3T --- verified
b2g-v1.4 --- fixed

People

(Reporter: psiddh, Assigned: echou)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 6 obsolete files)

This is a follow up bug for http://bugzilla.mozilla.org/show_bug.cgi?id=933093

Even with 3s timer after pairing of BT devices happen, SendFile (BT DOM) still seems to crash.
Attached patch Gaia: Test Patch for SendFile (obsolete) — Splinter Review
Attached patch SendFile_Gaia_WIP.patch (obsolete) — Splinter Review
Attachment #8363283 - Attachment is obsolete: true
In order to reproduce the issue, apply the patches 
1. https://bugzilla.mozilla.org/show_bug.cgi?id=959437 (These are about to be landed) + 
2. SendFile_Gaia_WIP.patch

Launch Nfc Demo and bring another FxOS (or) Android nfc enabled device closer to firefox device under test.

Observation: Nfc Demo should be shrunk

Action : Swipe on shrunk UI to trigger the 'sendFile' operation.

At this point BT DOM crashes and restarts gecko.
Blocks: b2g-nfc
Requesting Eric from BT team to look into the issue. Please note that the blob that we are trying to send is in the following manner

var dummyBlob = new Blob(['empty-image'], {'type': 'image/jpeg'});
Flags: needinfo?(echou)
I'll take a look today.
Assignee: nobody → echou
Flags: needinfo?(echou)
Hi Sid,

> In order to reproduce the issue, apply the patches 
> 1. https://bugzilla.mozilla.org/show_bug.cgi?id=959437 (These are about to
> be landed) + 
> 2. SendFile_Gaia_WIP.patch

I couldn't apply this patch on the latest Gaia:master. Moreover, there is no
'js' folder under gaia/apps/nfc-demo. Which branch are you using as your base?
Hi Eric,

Forgot to mention this. Please pull down latest nfc-demo from our svic repo and please apply the patch

https://github.com/svic/gaia/tree/master/apps/nfc-demo

Thanks
See Also: → 915602
Hi Sid,

I applied the patch and followed your STR "Launch Nfc Demo and bring another FxOS (or) Android nfc enabled device closer to firefox device under test." but nothing happened. The screen of Nfc Demo wasn't shrunk.
Hi Eric , did you turn on Nfc from settings app ?
If it still doesn't work can you attach the logs please?
Sid, any feedback.
Flags: needinfo?(psiddh)
Attachment #8363287 - Attachment is obsolete: true
Flags: needinfo?(psiddh)
Attached patch Part 2: SendFile_Gaia.patch (obsolete) — Splinter Review
ok, after trying many times I finally found how this happened.

In function BluetoothAdapter::SendFile(), we try to get BlobChild* actor by  ContentChild::GetSingleton()->GetOrCreateActorForBlob(aBlob). However ContentChild::GetSingleton() returns a nullptr. This is because nfc manager is running on parent process. I took some time to investigate but no idea how to get an BlobParent* from a nsIDOMBlob. I'm going to ask professionals like Ben Turner about this next Monday.
Component: NFC → Bluetooth
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Here is the best way to reproduce the issue on your end.

STEP 1: Sync to latest mozilla's gecko & gaia master branches
STEP 2: Apply the patch - 'Part 1: SendFile_Gecko.patch' to Gecko
STEP 3: Get nfc-demo from svic repo : https://github.com/svic/gaia/tree/master/apps/nfc-demo
STEP 4: Apply the patch - 'Part 2: SendFile_Gaia.patch' to Gaia

Before running the testcase:
1) Build, flash, boot. Then turn on Nfc settings from settings app
2) Atleast one picture should be present in Gallery.

Step to reproduce the issue: 
1) Open nfc-demo app
2) Bring say, Google nexus-4 device in close proximity to your FxOS device
3) nfc-demo should shrink
4) Swipe up on nfc-demo
5) Picker should launch
6) Choose Gallery and any picture and select 'done'
7) At this point BT pairing should have started on both sides
8) Pair both the devices with an explicit user action.
9) After this is done, select image blob should have started to transfer
10) Andorid phone eventually says, "Incoming beam failed"
11) FxOS device crashes / reboots

NOTE: I have done the exact steps to reproduce the issue

If you think there is an issue with blob being sent,
you could simply modify gaia/apps/nfc-demo/js/nfc_main.js at 
     'window.navigator.mozNfc.onpeerready = function(event) { .....'

I also have tested it by simply sending "var fakeBlob1 = new Blob([], {type: 'image/png'});"

As per my observation, there is some issue with 'blob' that I am creating.
Blob flow is something like this
  nfc-demo(gaia) --> Chrome process --> System gaia --> BT DOM
Yes Eric, I am glad that you are able to reproduce the issue that I am referring to. However I have also given elaborate explanation for your reference
Updated previous with Gaia patch with 'bind' fix
Attachment #8364992 - Attachment is obsolete: true
See Also: 915602
Eric, does that mean bug 915602 is fixed? Can Sid keep fixing this bug on the latest firefox OS?
Flags: needinfo?(echou)
(In reply to Ken Chang[:ken] from comment #20)
> Eric, does that mean bug 915602 is fixed? Can Sid keep fixing this bug on
> the latest firefox OS?

Unfortunately, no. I removed it from 'see also' list because they are not the same problem.
Flags: needinfo?(echou)
Hi Ben,

nfc_manager is a js file lives in chrome process. When BluetoothAdapter::SendFile() is called from nfc_manager.js, system crashes because ContentChild::GetSingleton() returns null. This patch is trying to get a BlobParent from ContentParent, however I'm not sure if I use it correctly.

Would you mind taking a look? It's quite a small patch. :)

Thanks,

Eric
Attachment #8365865 - Flags: review?(bent.mozilla)
Blocks: 903253, 948362, 948363
Comment on attachment 8365865 [details] [diff] [review]
patch 1: Using ContentParent::GetOrCreateActorForBlob() to get a BlobParent* from a nsIDOMBlob

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +746,5 @@
>      return nullptr;
>    }
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    nsRefPtr<ContentParent> parent = ContentParent::GetNewOrUsed();

Hi Eric, this is not correct. You almost never want to call this as it creates a new ContentParent (or recycles an old one).

In this case you're not trying to share a blob with another process, right? Actors always represent an object in another process (or another thread), so if that's the case then you won't have an actor for that blob.

You probably need an additional SendFile() function that operates on an in-process blob.
Attachment #8365865 - Flags: review?(bent.mozilla) → review-
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #23)
> Comment on attachment 8365865 [details] [diff] [review]
> patch 1: Using ContentParent::GetOrCreateActorForBlob() to get a BlobParent*
> from a nsIDOMBlob
> 
> Review of attachment 8365865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +746,5 @@
> >      return nullptr;
> >    }
> > +
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> > +    nsRefPtr<ContentParent> parent = ContentParent::GetNewOrUsed();
> 
> Hi Eric, this is not correct. You almost never want to call this as it
> creates a new ContentParent (or recycles an old one).
> 
> In this case you're not trying to share a blob with another process, right?
> Actors always represent an object in another process (or another thread), so
> if that's the case then you won't have an actor for that blob.
> 
> You probably need an additional SendFile() function that operates on an
> in-process blob.

I see. Thanks for efficient review, Ben. :)
Hi Ben,

I took your advice and make another SendFile() for in-process transfer. Would you mind taking a look?
Attachment #8365865 - Attachment is obsolete: true
Attachment #8367146 - Flags: review?(gyeh)
Attachment #8367146 - Flags: feedback?(bent.mozilla)
Comment on attachment 8367146 [details] [diff] [review]
patch 1: v2: Support in-process bt file transfer

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

In general I think this approach is fine but there are a few things you should take a look at here:

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +749,5 @@
> +    // In-process transfer
> +    bs->SendFile(aDeviceAddress, aBlob, results);
> +  } else {
> +    BlobChild* actor =
> +      ContentChild::GetSingleton()->GetOrCreateActorForBlob(aBlob);

This can fail in some cases (I can't remember which offhand) so you should keep the existing null check I think.

@@ +750,5 @@
> +    bs->SendFile(aDeviceAddress, aBlob, results);
> +  } else {
> +    BlobChild* actor =
> +      ContentChild::GetSingleton()->GetOrCreateActorForBlob(aBlob);
> +    bs->SendFile(aDeviceAddress, nullptr, actor, results);

Just curious, is there every any place where you call this function with a non-null value for the BlobParent parameter? It seems like you should remove that entirely.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +348,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  return SendFile(aDeviceAddress, aActor->GetBlob().get());

Hm, careful, I think this leaks.

|aActor->GetBlob()| returns an |already_AddRefed<nsIDOMBlob>| and unless you assign that into a nsCOMPtr nothing will ever release it... It looks like this was leaking already?

In general whenever you see |already_AddRefed| make sure it goes into a nsCOMPtr/nsRefPtr rather than calling |get()| on it.

::: dom/bluetooth/bluez/BluetoothOppManager.cpp
@@ +364,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  return SendFile(aDeviceAddress, aActor->GetBlob().get());

Same leak potential here.
Attachment #8367146 - Flags: feedback?(bent.mozilla) → feedback+
> Hm, careful, I think this leaks.
> 
> |aActor->GetBlob()| returns an |already_AddRefed<nsIDOMBlob>| and unless you
> assign that into a nsCOMPtr nothing will ever release it... It looks like
> this was leaking already?
> 
> In general whenever you see |already_AddRefed| make sure it goes into a
> nsCOMPtr/nsRefPtr rather than calling |get()| on it.

I filed bug 965498 for this.
Nominate to 1.4? since this blocks NFC sharing functions.
blocking-b2g: --- → 1.4?
 > ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +749,5 @@
> > +    // In-process transfer
> > +    bs->SendFile(aDeviceAddress, aBlob, results);
> > +  } else {
> > +    BlobChild* actor =
> > +      ContentChild::GetSingleton()->GetOrCreateActorForBlob(aBlob);
> 
> This can fail in some cases (I can't remember which offhand) so you should
> keep the existing null check I think.

ok

> 
> @@ +750,5 @@
> > +    bs->SendFile(aDeviceAddress, aBlob, results);
> > +  } else {
> > +    BlobChild* actor =
> > +      ContentChild::GetSingleton()->GetOrCreateActorForBlob(aBlob);
> > +    bs->SendFile(aDeviceAddress, nullptr, actor, results);
> 
> Just curious, is there every any place where you call this function with a
> non-null value for the BlobParent parameter? It seems like you should remove
> that entirely.
> 

In fact here is the only place SendFile() is called. I'll do the cleanup.

> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +348,4 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  return SendFile(aDeviceAddress, aActor->GetBlob().get());
> 
> Hm, careful, I think this leaks.
> 
> |aActor->GetBlob()| returns an |already_AddRefed<nsIDOMBlob>| and unless you
> assign that into a nsCOMPtr nothing will ever release it... It looks like
> this was leaking already?
> 
> In general whenever you see |already_AddRefed| make sure it goes into a
> nsCOMPtr/nsRefPtr rather than calling |get()| on it.
> 

Thanks for pointing this out. Thanks to Kyle's help for firing bug 965498, too.
Comment on attachment 8367146 [details] [diff] [review]
patch 1: v2: Support in-process bt file transfer

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

Please rebase the patch since bug 965498 has fixed the memory leak part. Also, there's no need to override |SendFile|, please remove one of the methods.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1242,5 @@
> +                                    nsIDOMBlob* aBlob,
> +                                    BluetoothReplyRunnable* aRunnable)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

Question: Shall we stop discovery before sending the files?
Attachment #8367146 - Flags: review?(gyeh) → review+
Hi Ben,

Nits picked except not removing the BlobParent* parameter from SendFile(). The reason is that, when the request is received on chrome process, it calls SendFile() with a BlobParent* having value and a BlobChild* has no value. (http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothParent.cpp#511)

I tried to modify the interface but the current version seems reasonable to me. Would you mind taking a look and giving me some advice? Thank you.
Attachment #8367146 - Attachment is obsolete: true
Attachment #8370636 - Flags: feedback?(bent.mozilla)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #30)
> Comment on attachment 8367146 [details] [diff] [review]
> patch 1: v2: Support in-process bt file transfer
> 
> Review of attachment 8367146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please rebase the patch since bug 965498 has fixed the memory leak part.

Done.

> Also, there's no need to override |SendFile|, please remove one of the
> methods.

Actually we do need two SendFile() for in-process and cross-process file transferring -- at lease I can't think of a better way to use only one method for both cases. :(

> 
> ::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
> @@ +1242,5 @@
> > +                                    nsIDOMBlob* aBlob,
> > +                                    BluetoothReplyRunnable* aRunnable)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> 
> Question: Shall we stop discovery before sending the files?

Should be done in Gaia. I'm not a fan for doing this implicitly from Gecko side.
(In reply to Eric Chou [:ericchou] [:echou] from comment #32)
> > 
> > ::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
> > @@ +1242,5 @@
> > > +                                    nsIDOMBlob* aBlob,
> > > +                                    BluetoothReplyRunnable* aRunnable)
> > > +{
> > > +  MOZ_ASSERT(NS_IsMainThread());
> > > +
> > 
> > Question: Shall we stop discovery before sending the files?
> 
> Should be done in Gaia. I'm not a fan for doing this implicitly from Gecko
> side.

Great! Just found we've removed this part in current code base. :)
It is a 1.4+ bug.
blocking-b2g: 1.4? → 1.4+
Eric, 
Possible to assess ETA for this bug? 
Also, is Bug 964693 a dependency to this?
Flags: needinfo?(echou)
(In reply to Wesley Huang [:wesley_huang] from comment #35)
> Eric, 
> Possible to assess ETA for this bug? 

Target milestone is set.

> Also, is Bug 964693 a dependency to this?

No, they are separate. Both bugs have to be fixed to make NFC file-sharing work.
Flags: needinfo?(echou)
Target Milestone: --- → 1.4 S1 (14feb)
Comment on attachment 8370636 [details] [diff] [review]
patch 1: v3: Support in-process bt file transfer

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

Ok, if you need it then that's fine :)

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp
@@ +261,5 @@
> +  const nsAString& aDeviceAddress,
> +  nsIDOMBlob* aBlobChild,
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  // Chrome-process-only method

Nit: I would call it the "parent process", not the "chrome process"
Attachment #8370636 - Flags: feedback?(bent.mozilla) → feedback+
* nit picked. Thank you both!
Attachment #8370636 - Attachment is obsolete: true
sorry had to back out this change, seems this caused issues on device builds and others like https://tbpl.mozilla.org/php/getParsedLog.php?id=34280830&tree=B2g-Inbound
try again for ICS emulator build break: https://tbpl.mozilla.org/?tree=Try&rev=d308798a14d3
What is the ETA of this bug?
This blocks essential scenario for upcoming NFC demo, which we really need to get it done asap.
(In reply to Wesley Huang [:wesley_huang] from comment #44)
> What is the ETA of this bug?

Please see comment 36. The target milestone has been set.

> This blocks essential scenario for upcoming NFC demo, which we really need
> to get it done asap.

I've been already on it for a while. Will land the patch once it gets all green.
Thanks Eric, and apologize for redundant checking of ETA.
https://hg.mozilla.org/mozilla-central/rev/90e04ebf9793
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Duplicate of this bug: 1016472
Thanks, Fabrice.
Verified Fixed on Tarako 1.3t - no crash when sending files via BT

Tarako 1.3t
BuildID: 20140528175422
Gaia: 303e375a1b4c721984dcb68dfca24d6f50c291f2
Gecko: fcabeab5ebec
Version: 28.1
sp6821a-Gonk4.0-5-12
You need to log in before you can comment on or make changes to this bug.