844705 - [b2g-bluetooth] Should send socket data in main thread

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

After reading file from input stream (in a non-main thread), we should go back to main thread and then send put packet since SendSocketData() should be called in main thread.

Comment 1

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

Attachment: Patch 1(v1): Should send socket data in main thread

Comment 2

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

Comment on attachment 717741 [details] [diff] [review]
Patch 1(v1): Should send socket data in main thread

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

Looks good.

Comment 3

[Mozilla RelEng Bot]

Try run for c02e596339cb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c02e596339cb
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c02e596339cb

Comment 4

[Mozilla RelEng Bot]

Try run for 67782e6565d5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=67782e6565d5
Results (out of 18 total builds):
    success: 17
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-67782e6565d5

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e002cb8693

Comment 6

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

Attachment: m-c: Final patch, Should send socket data in main thread r=echou

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0e002cb8693

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why did the nsAutoArrayPtr go away? Looks like you'll leak if Read() fails.

Comment 9

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

Let's reopen it and fix it with a new patch.

Comment 10

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

Attachment: Patch 1(v2): Should send socket data in main thread

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 718334 [details] [diff] [review]
Patch 1(v2): Should send socket data in main thread

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

Thanks.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +160,5 @@
>      MOZ_ASSERT(!NS_IsMainThread());
>  
>      uint32_t numRead;
> +    nsAutoArrayPtr<char> buf;
> +    buf = new char[mAvailablePacketSize];

You could also do

nsAutoArrayPtr<char> buf(new char[mAvailablePacketSize]);

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/94a4a675601e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94a4a675601e

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Requesting tef+ because this blocks 851046, see reasoning in https://bugzilla.mozilla.org/show_bug.cgi?id=851046#c50

Comment 15

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

Attachment: b2g18 Patch 3 (v1): Should send socket data in main thread, r=echou

This is the b2g18-specific patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a6c459e49121
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b3c347007706

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for build bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/24b27125d907
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/89b4f2ae8807

https://tbpl.mozilla.org/php/getParsedLog.php?id=21542033&tree=Mozilla-B2g18_v1_0_1

/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.cpp: In member function 'virtual nsresult SendSocketDataTask::Run()':
/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.cpp:136: error: no matching function for call to 'mozilla::dom::bluetooth::BluetoothOppManager::SendPutRequest(nsAutoArrayPtr<unsigned char>&, uint32_t&)'
/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.h:66: note: candidates are: void mozilla::dom::bluetooth::BluetoothOppManager::SendPutRequest(mozilla::ipc::UnixSocketImpl*, uint8_t*, int)
/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.cpp: In member function 'virtual void ReadFileTask::Run()':
/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.cpp:184: error: 'class nsAutoArrayPtr<char>' has no member named 'forgot'
/builds/slave/m-b18H1-ics_a7_g-d-00000000000/build/dom/bluetooth/BluetoothOppManager.cpp:187: error: return-statement with a value, in function returning 'void'
make[6]: *** [BluetoothOppManager.o] Error 1

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Eric/gina: Why did the b2g18 patch need changes? The m-c patch came in cleanly if 845148 was backed out first (Which I forgot to add a patch to do here). Was there other things in it I was missing?

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 patch 1: revert bug 845148

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 734743 [details] [diff] [review]
b2g18 patch 1: revert bug 845148

Adding revert patch just in case it is needed. If not, just r- and ignore me. However, we don't take it, we'll need to update the patches in 851046 because it doesn't expect the UnixSocketImpl* parameter in ReadFileTask.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 734743 [details] [diff] [review]
b2g18 patch 1: revert bug 845148

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

Please leave the first two hunks from UnixSocket.cpp out when you check this in, as they're still correct (leaving them shouldn't cause any more conflicts, either). So the only change to UnixSocket.cpp should be the removal of RawSendSocketData.

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 patch 1 (v2): revert bug 845148; r=mrbkap

Already r+'d by mrbkap, updated with his nits.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 patch 2 (v1): mReadFileThread comment; r=mrbkap

Adding a comment so we know why mReadFileThread came back. r=mrbkap via IRC.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - mReadFileThread comment; r=mrbkap; DONTBUILD

Adding patch for m-c landing.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b1ab5e023f4a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/274016699efe

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b96b8badb168
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/52f139c30efc

I'll land the m-c follow-up patch the next time I'm pushing other things on inbound.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e12aede3338

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment #734349 [details] [diff] shouldn't have been obsoleted. It still needs to be fixed and landed. 

I realize that probably wasn't obvious. This whole thing is a serious mess. :|

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 Patch 3 (v2): Should send socket data in main thread, r=echou

Fixing from forgot to forget. r=echou stands, plus r=me

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/44fa08451292
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/51a0fbcd25b8

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e12aede3338