962310 - Nfc Handover: SendFile BT API crashes

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[psiddh]

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.

Comment 1

[psiddh]

Attachment: Gaia: Test Patch for SendFile

Comment 2

[psiddh]

Attachment: SendFile_Gaia_WIP.patch

Comment 3

[psiddh]

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.

Comment 4

[psiddh]

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'});

Comment 5

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

I'll take a look today.

Comment 6

[Eric Chou [:ericchou] [: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?

Comment 7

[psiddh]

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

Comment 8

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

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.

Comment 9

[psiddh]

Hi Eric , did you turn on Nfc from settings app ?

Comment 10

[psiddh]

If it still doesn't work can you attach the logs please?

Comment 11

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

Sure. adb logcat: http://www.pastebin.mozilla.org/4071843

Comment 12

[Ken Chang[:kenkai]]

Sid, any feedback.

Comment 13

[psiddh]

Attachment: Part 1: SendFile_Gecko.patch

Comment 14

[psiddh]

Attachment: Part 2: SendFile_Gaia.patch

Comment 15

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

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.

Comment 16

[psiddh]

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

Comment 17

[psiddh]

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

Comment 18

[psiddh]

Attachment: Part 2: SendFile_Gaia.patch

Updated previous with Gaia patch with 'bind' fix

Comment 20

[Ken Chang[:kenkai]]

Eric, does that mean bug 915602 is fixed? Can Sid keep fixing this bug on the latest firefox OS?

Comment 21

[Eric Chou [:ericchou] [: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.

Comment 22

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

Attachment: patch 1: Using ContentParent::GetOrCreateActorForBlob() to get a BlobParent* from a nsIDOMBlob

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

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 24

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

(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. :)

Comment 25

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

Attachment: patch 1: v2: Support in-process bt file transfer

Hi Ben,

I took your advice and make another SendFile() for in-process transfer. Would you mind taking a look?

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> 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.

Comment 28

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Nominate to 1.4? since this blocks NFC sharing functions.

Comment 29

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

 > ::: 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 30

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

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?

Comment 31

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

Attachment: patch 1: v3: Support in-process bt file transfer

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.

Comment 32

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

(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.

Comment 33

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

(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. :)

Comment 34

[Ken Chang[:kenkai]]

It is a 1.4+ bug.

Comment 35

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Eric, 
Possible to assess ETA for this bug? 
Also, is Bug 964693 a dependency to this?

Comment 36

[Eric Chou [:ericchou] [: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.

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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"

Comment 38

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

Attachment: patch 1: final: Support in-process bt file transfer, f=bent, r=gyeh

* nit picked. Thank you both!

Comment 39

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

try: https://tbpl.mozilla.org/?tree=Try&rev=e5724ba9d3f2

Comment 40

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

https://hg.mozilla.org/integration/b2g-inbound/rev/c511e6633686

Comment 41

[Carsten Book [:Tomcat]]

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

Comment 42

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

try again: https://tbpl.mozilla.org/?tree=Try&rev=63614df31bc4

Comment 43

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

try again for ICS emulator build break: https://tbpl.mozilla.org/?tree=Try&rev=d308798a14d3

Comment 44

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

What is the ETA of this bug?
This blocks essential scenario for upcoming NFC demo, which we really need to get it done asap.

Comment 45

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

(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.

Comment 46

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Thanks Eric, and apologize for redundant checking of ETA.

Comment 47

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

tyr again: https://tbpl.mozilla.org/?tree=Try&rev=3092621efbcf

Comment 48

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

https://hg.mozilla.org/integration/b2g-inbound/rev/90e04ebf9793

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90e04ebf9793

Comment 50

[[:fabrice] Fabrice Desré]

Uplifted to 1.3t to fix bug 1016472:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/af54fa7247bb

Comment 52

[James Zhang (Spreadtrum)]

Thanks, Fabrice.

Comment 53

[Natalya Kot [:nkot]]

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