1014485 - [Flame][NFC]: NfcConnector::Create() still keep calling socket() to consume performance even device has no nfcd.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect, P1)

Description

[Vincent Liu[:vliu]]

The issue happens on v1.4 branch. As I know ffos doesn't include nfcd in v1.4 branch.

Please also see bug 1008000 Comment 33 to know the detail.

From gdb tracking, NfcConnector::Create() kept calling socket() to connect even it had connect fail with the error code |No such file or directory| in previous calling. 

Breakpoint 1, socket () at bionic/libc/arch-arm/syscalls/socket.S:8
8	    ldr     r7, =__NR_socket
(gdb) bt
#0  socket () at bionic/libc/arch-arm/syscalls/socket.S:8
#1  0xb4f4d3ec in (anonymous namespace)::NfcConnector::Create (this=0xae7ff274) at ../../../gecko/ipc/nfc/Nfc.cpp:201
#2  0xb4f4e2b0 in mozilla::ipc::UnixSocketImpl::Connect (this=0xae47c420) at ../../../gecko/ipc/unixsocket/UnixSocket.cpp:497
#3  0xb4ea4d90 in MessageLoop::RunTask (this=0xb3fa4de0, task=0xaccf3558) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:344
#4  0xb4ea5452 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=<optimized out>) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:352
#5  0xb4ea6484 in DoWork (this=<optimized out>) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:430
#6  MessageLoop::DoWork (this=0xb3fa4de0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:409
#7  0xb4e9ddbe in base::MessagePumpLibevent::Run (this=0xb6b01ee0, delegate=0xb3fa4de0) at ../../../gecko/ipc/chromium/src/base/message_pump_libevent.cc:311
#8  0xb4ea4d1e in MessageLoop::RunInternal (this=<optimized out>) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:226
#9  0xb4ea4dd0 in RunHandler (this=0xb3fa4de0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:219
#10 MessageLoop::Run (this=0xb3fa4de0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:193
#11 0xb4ea75c2 in base::Thread::ThreadMain (this=0xb6b1a500) at ../../../gecko/ipc/chromium/src/base/thread.cc:162
#12 0xb4e9e270 in ThreadFunc (closure=<optimized out>) at ../../../gecko/ipc/chromium/src/base/platform_thread_posix.cc:39
#13 0xb6ef5ba4 in __thread_entry (func=0xb4e9e269 <ThreadFunc(void*)>, arg=0xb6b1a500, tls=0xb3fa4f00) at bionic/libc/bionic/pthread_create.cpp:92
#14 0xb6ef5d20 in pthread_create (thread_out=0xb6b1a508, attr=<optimized out>, start_routine=0x78, arg=0xb6b1a500) at bionic/libc/bionic/pthread_create.cpp:201
#15 0xb1084800 in ?? ()

Comment 1

[Vincent Liu[:vliu]]

Since nfcd put into my local build and performance becomes better. The issue of bug 1008000 also fixed. 
Nfc users ro.moz.nfc.enabled in device folder to enable this function. No matter Gecko in flame device runs for master or v1.4 branch, it both uses the same ro.moz.nfc.enabled property value. It causes a problem if we decide disabling for v1.4 but still enabling for master.

Discussed with @dlee, and we decide also put nfcd into v1.4.

Comment 2

[Ken Chang[:kenkai]]

I think QA want to test flame with 1.4 as well. But why cannot we have different branch for device folder?
This problem would happen again in this feature because we always develop a flying feature in main trunk and don't want to enable it in branch. It is necessary to have a method to handle this kind of situation.

Comment 3

[Vincent Liu[:vliu]]

(In reply to Ken Chang[:ken] from comment #2)
> I think QA want to test flame with 1.4 as well. But why cannot we have
> different branch for device folder?

Hi mwu,

Do you think it a good way to do for the future branch?

Comment 4

[Dimi Lee [:dimi]]

Checking with Ken, Vincent, disable NFC function in 1.4 might be a better solution.
So I will take this bug.

Comment 5

[Ken Chang[:kenkai]]

I had nominated this bug as 1.4+ before. We don't ni?kevin.

Comment 6

[Dimi Lee [:dimi]]

Attachment: bug_1014485_v1.patch

Disable NFC function in 1.4

Comment 7

[Michael Wu [:mwu]]

(In reply to Ken Chang[:ken] from comment #2)
> I think QA want to test flame with 1.4 as well. But why cannot we have
> different branch for device folder?
> This problem would happen again in this feature because we always develop a
> flying feature in main trunk and don't want to enable it in branch. It is
> necessary to have a method to handle this kind of situation.

The flame device repo not having a 1.4 branch is entirely accidental. I'm not familiar with how the branches are generated, but it looks like it got missed.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8428546 [details] [diff] [review]
bug_1014485_v1.patch

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

From mwu's comment,
I also think this should be fixed in Gonk.

Comment 9

[Vincent Liu[:vliu]]

(In reply to Michael Wu [:mwu] from comment #7)

> The flame device repo not having a 1.4 branch is entirely accidental. I'm
> not familiar with how the branches are generated, but it looks like it got
> missed.

Does the sentence "how the branches are generated" means by 1.4 or b2g-4.3_r2.1? Which one do you prefer? I went through many different device repos and couldn't see any rule. I can create PR to you to do it if you think flame need this branch.

Comment 10

[Michael Wu [:mwu]]

1.4. b2g-4.3_r2.1 is *only* for branches from AOSP tags like android-4.3_r2.1. For everything else where work is done on the master branch, the usual version names apply.

Comment 11

[Vincent Liu[:vliu]]

(In reply to Michael Wu [:mwu] from comment #10)
> 1.4. b2g-4.3_r2.1 is *only* for branches from AOSP tags like
> android-4.3_r2.1. For everything else where work is done on the master
> branch, the usual version names apply.

Thanks. Can you please clone a device repo from master branch? It seems I don't have permission to create one or by PR? Thanks.

Comment 12

[Michael Wu [:mwu]]

New branches can't be created by PR. I've added device-flame to the Chefs team in mozilla-b2g so you should be able to push a new branch.

Comment 13

[Ken Chang[:kenkai]]

Hi Vincent, It seems that you are working on this bug. If not, please reassign it.

Comment 14

[Vincent Liu[:vliu]]

When I run |git push origin v1.4|, I got the below message

ERROR: Permission to mozilla-b2g/device-flame.git denied to msliu.
fatal: The remote end hung up unexpectedly

Can you please help me out? Thanks

Comment 15

[Michael Wu [:mwu]]

Do you have level 3 push access? If so, I can add you to the group.

Comment 16

[Michael Wu [:mwu]]

I've generated a v1.4 branch based off current master on device-flame.

Comment 17

[Vincent Liu[:vliu]]

Attachment: pull request for bug 1014485 to v1.4 branch.html

(In reply to Michael Wu [:mwu] from comment #16)
> I've generated a v1.4 branch based off current master on device-flame.

Many thanks. I have raised a pull request for the patch of this bug. Can you please have a review?

Comment 18

[Michael Wu [:mwu]]

Post a PR for the master branch and I'll review that.

You'll also need to provide a PR to switch flame on 1.4 to using the 1.4 branch of device-flame. I only made a branch. Nothing points to it.

Comment 19

[Vincent Liu[:vliu]]

(In reply to Michael Wu [:mwu] from comment #18)
> Post a PR for the master branch and I'll review that.
> 

On master branch, |ro.moz.nfc.enabled| has already defined as true. This PR intends to set |ro.moz.nfc.enabled| as false. Based on this, does this PR is expected to do this?

> You'll also need to provide a PR to switch flame on 1.4 to using the 1.4
> branch of device-flame. I only made a branch. Nothing points to it.

Sure! I will prepare another PR for this purpose. Thanks.

Comment 20

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #18)
> > Post a PR for the master branch and I'll review that.
> > 
> 
> On master branch, |ro.moz.nfc.enabled| has already defined as true. This PR
> intends to set |ro.moz.nfc.enabled| as false. Based on this, does this PR is
> expected to do this?
> 

Oops. it seems the sentence is not clear to express. Rewrite it again.

On master branch, |ro.moz.nfc.enabled| has already defined as true. This PR intends to set |ro.moz.nfc.enabled| as false on v1.4 branch. Based on this, does this PR is
 expected for you to do this? Thanks.

Comment 21

[Michael Wu [:mwu]]

(In reply to Vincent Liu[:vliu] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #18)
> > Post a PR for the master branch and I'll review that.
> > 
> 
> On master branch, |ro.moz.nfc.enabled| has already defined as true. This PR
> intends to set |ro.moz.nfc.enabled| as false. Based on this, does this PR is
> expected to do this?
> 

Ok, makes sense then.

Comment 22

[Vincent Liu[:vliu]]

Attachment: b2g-manifest-for- v1.4 branch.html

(In reply to Vincent Liu[:vliu] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #18)
> > You'll also need to provide a PR to switch flame on 1.4 to using the 1.4
> > branch of device-flame. I only made a branch. Nothing points to it.
> 
> Sure! I will prepare another PR for this purpose. Thanks.

Another PR for the modification of manifest file. Can you please have a review? Thanks.

Comment 23

[Vincent Liu[:vliu]]

Hi Ryan,

I would like to remind of you that both of these two pull requests need to be landed. Thanks for your great help to land the patches.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

device-flame/v1.4: https://github.com/mozilla-b2g/device-flame/commit/fcadcb3e4db366ca3d0973ae9f1a36125109dff4

b2g-manifest/v1.4: https://github.com/mozilla-b2g/b2g-manifest/commit/c125b5cfccca8ed65fc1f1cd5fd30c5401ffa5d8