Closed Bug 1054191 Opened 7 years ago Closed 7 years ago

[B2G] Cannot enable usb tethering on flame

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S4 (12sep)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: hchang, Assigned: hchang)

Details

(Keywords: regression, Whiteboard: [p=3][2.1-flame-test-run-1][2.1-flame-test-run-2])

Attachments

(2 files, 1 obsolete file)

STR on flame:

1) Flash to m-c
2) Enable data
3) Enable USB tethering

See the following error message:

E/GeckoConsole(  321): [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 1 [nsINetworkService.enableUsbRndis]" {file: "jar:file:///system/b2g/omni.ja!/components/NetworkManager.js" line: 1024}]
OS: Linux → Gonk (Firefox OS)
Just change the uuid of nsIEnableUsbRndisCallback 
then it works like a charm...

The error path is:

XPCWrappedNative::CallMethod
CallMethodHelper::Call()
ConvertIndependentParams
ConvertIndependentParam (i = 1)
XPCConvert::JSData2Native
JSObject2NativeInterface
nsXPCWrappedJS::GetNewOrUsed
nsXPCWrappedJSClass::GetNewOrUsed
nsXPConnect::XPConnect()->GetInfoForIID(&aIID, getter_AddRefs(info))

where info is nullptr...
QA Wanted for branch checks.
Keywords: qawanted
QA Contact: ckreinbring
The bug repros on Flame 2.1 and Open C 2.1
Actual result: When activating USB Tethering on the device, the connected PC does not recognize the new connection.

Flame 2.1
BuildID: 20140814191115
Gaia: 9979fccc6321be72cd17370f3a20c65bc376af33
Gecko: 9827d3cf6f69
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Open C 2.1
BuildID: 20140814191115
Gaia: 9979fccc6321be72cd17370f3a20c65bc376af33
Gecko: 9827d3cf6f69
Platform Version: 34.0a1
Firmware Version: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

--------------------------------------------------------------------------------------------------------

The bug does not repro of Flame 2.0 or Buri 2.1
Actual result: When activating USB Tethering on the device, the connected PC recognizes the new wired connection.

Flame 2.0
BuildID: 20140814204728
Gaia: 7fdb37b8a6688ef39ceaa41b56c4a9339802e78e
Gecko: 7505293883fb
Platform Version: 32.0
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Buri 2.1
BuildID: 20140814191916
Gaia: 9979fccc6321be72cd17370f3a20c65bc376af33
Gecko: c9f8cc9ce89c
Platform Version: 34.0a1
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: qawanted
[Blocking Requested - why for this release]:
Bad functionality regression.

Requesting a window.
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Note: The pushlog for B2G inbound is large due to the fact that the window straddles the bug where the screen goes black right after flashing and none of the apps load properly.

Regression window
Last working
BuildID: 20140801110356
Gaia: 9689218473b6fc4dd927ad6aa7b06c05f0843824
Gecko: ff1e09666c42
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First broken
BuildID: 20140801122949
Gaia: 1f20f326f317feda0ec59aff011b54a7c62cb9bf
Gecko: 9fa94a0f6c57
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Working Gaia / Broken Gecko = Repro
Gaia: 9689218473b6fc4dd927ad6aa7b06c05f0843824
Gecko: 9fa94a0f6c57
Broken Gaia / Working Gecko = No repro
Gaia: 1f20f326f317feda0ec59aff011b54a7c62cb9bf
Gecko: ff1e09666c42
Gecko push log: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ff1e09666c42&tochange=9fa94a0f6c57


B2G-inbound
Last working
BuildID: 20140731035207
Gaia: 29793743960686ee8d5c4617a36172241c8ae0d0
Gecko: a5fc614872dd
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First broken
BuildID: 20140801100457
Gaia: 22846bf06d34bd42d124952402ab121e6ae05c27
Gecko: f2acc6ff7aee
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Working Gaia / Broken Gecko = Repro
Gaia: 29793743960686ee8d5c4617a36172241c8ae0d0
Gecko: f2acc6ff7aee
Broken Gaia / Working Gecko = No repro
Gaia: 22846bf06d34bd42d124952402ab121e6ae05c27
Gecko: a5fc614872dd
Gecko pushlog: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=a5fc614872dd&tochange=f2acc6ff7aee
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Chris can you also get the mozilla-inbound perhaps we can get a smaller window by comparing the pushlogs.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage-]
Flags: needinfo?(pbylenga) → needinfo?(ckreinbring)
Mozilla-inbound
Last working
BuildID: 20140801121007
Gaia: 9689218473b6fc4dd927ad6aa7b06c05f0843824
Gecko: ceb3128f93e9
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First broken
BuildID: 20140801130150
Gaia: 1f20f326f317feda0ec59aff011b54a7c62cb9bf
Gecko: a628362a2af8
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Working Gaia / Broken Gecko = Repro
Gaia: 9689218473b6fc4dd927ad6aa7b06c05f0843824
Gecko: a628362a2af8
Broken Gaia / Working Gecko = No repro
Gaia: 1f20f326f317feda0ec59aff011b54a7c62cb9bf
Gecko: ceb3128f93e9
Gecko pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ceb3128f93e9&tochange=a628362a2af8
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?
Flags: needinfo?(ckreinbring) → needinfo?(dharris)
QA Whiteboard: [QAnalyst-Triage? → [QAnalyst-Triage+]
Flags: needinfo?(dharris) → needinfo?
Assignee: nobody → hchang
Flags: needinfo?
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S3 (29aug)
Not working  39f167a45afc  Thinker K.F. Li — Bug 977026 - Part 3: Prelo....
...          eabafb319aae  Thinker K.F. Li — Bug 977026 - Part 2: B2G lo....
...          6bcd4c911e68  Thinker Li — Bug 977026 - Part 1: Allow ...
Working      07c91b928cd7  Viral Wang — Bug 932698 - Hold a wakelock when ....

One of the patches for Bug 977026 is the regressor.
Blocks: 997026
Component: General → DOM: Content Processes
Product: Firefox OS → Core
Blocks: 977026
No longer blocks: 997026
39f167a45afc is verified to be the regressor and I am looking into it.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
[1] and [2] doesn't make mDisplayName a null-terminated string
so that [3] would overrun the buffer and the hash table
used to store the IID info [4] would be corrupted. 

The upstream seems to have solved this issue [5].

Will attach the hwcomposer.msm8610.so I rebuilt with
fixes (simply strcpy instead strcpy) to verify my argument.

$ adb push hwcomposer.msm8610.so system/lib/hw/hwcomposer.msm8610.so

and usb tethering will works.

[1] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/tree/libhwcomposer/hwc_dump_layers.cpp?h=jb_3.2_rb5.60#n61
[2] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/tree/libhwcomposer/hwc_dump_layers.cpp?h=jb_3.2_rb5.60#n61
[3] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/tree/libhwcomposer/hwc_dump_layers.cpp?h=jb_3.2_rb5.60#n65
[4] http://hg.mozilla.org/mozilla-central/file/ffdd1a398105/xpcom/reflect/xptinfo/XPTInterfaceInfoManager.h#l104
[5] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/diff/libhwcomposer/hwc_dump_layers.cpp?id=36bd527bc8a2abba8a003879c46f76bc5cfcdca6
Attached file hwcomposer.msm8610.so
(In reply to Henry Chang [:henry] from comment #11)
> [1] and [2] doesn't make mDisplayName a null-terminated string
> so that [3] would overrun the buffer and the hash table
> used to store the IID info [4] would be corrupted. 
> 
> The upstream seems to have solved this issue [5].
> 
> Will attach the hwcomposer.msm8610.so I rebuilt with
> fixes (simply strcpy instead strcpy) to verify my argument.
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
simply use strcpy instead of strncpy. (use sizeof(mDisplayName) is also an option)
By the way, since 39f167a45afc change the timing of parsing xpt, XPTInterfaceInfoManager::mWorkingSet:: mIIDTable becomes the victim.
Component: DOM: Content Processes → Graphics: Layers
After pushing my modified hwcomposer.so, usb tethering still 
doesn't work due to 

http://hg.mozilla.org/mozilla-central/rev/19e59c14b0aa
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(dharris)
Whiteboard: [p=3] → [p=3][2.1-flame-test-run-1]
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(dharris)
Attached patch Backout.patch (obsolete) — Splinter Review
Partially backed out http://hg.mozilla.org/mozilla-central/rev/19e59c14b0aa to fix usb tethering.

Dave, usb tethering was broken due to 19e59c14b0aa and is still not fixed with the latest m-c. Backing out AutoMounter only is good enough to fix usb tethering so there might be some issue regarding MTP/Mass storage/USB coexistence. Do you have any comment on this issue? Thanks!

By the way, since usb tethering is also broken by an gonk issue (HwcDebug buffer overflow), you have to push the attached hwcomposer.msm8610.so to the device first.

STR:
1) Flash flame with v123 base image the flash the latest gecko.
2) Push the attachment hwcomposer.msm8610.so to system/lib/hw
3) Settings => Internet sharing => usb tethering

At this time usb tethering will not work

4) Apply patch, rebuild and reflash gecko.

Usb tethering will work now.
Flags: needinfo?(dhylands)
I updated the AutoMounter state machine to accomodate rndis, so usb tethering should work properly now.
Attachment #8482513 - Attachment is obsolete: true
Attachment #8483179 - Flags: review?(hchang)
Flags: needinfo?(dhylands)
(In reply to Henry Chang [:henry] from comment #13)
> (In reply to Henry Chang [:henry] from comment #11)
> > [1] and [2] doesn't make mDisplayName a null-terminated string
> > so that [3] would overrun the buffer and the hash table
> > used to store the IID info [4] would be corrupted. 
> > 
> > The upstream seems to have solved this issue [5].
> > 
> > Will attach the hwcomposer.msm8610.so I rebuilt with
> > fixes (simply strcpy instead strcpy) to verify my argument.
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> simply use strcpy instead of strncpy. (use sizeof(mDisplayName) is also an
> option)

We should NEVER use strcpy or strcat. Generally speaking these are what attackers look for when trying to find buffer overflow attacks.

You're MUCH better off to use strlcpy/strlcat.
(In reply to Dave Hylands [:dhylands] from comment #18)
> (In reply to Henry Chang [:henry] from comment #13)
> > (In reply to Henry Chang [:henry] from comment #11)
> > > [1] and [2] doesn't make mDisplayName a null-terminated string
> > > so that [3] would overrun the buffer and the hash table
> > > used to store the IID info [4] would be corrupted. 
> > > 
> > > The upstream seems to have solved this issue [5].
> > > 
> > > Will attach the hwcomposer.msm8610.so I rebuilt with
> > > fixes (simply strcpy instead strcpy) to verify my argument.
> >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > simply use strcpy instead of strncpy. (use sizeof(mDisplayName) is also an
> > option)
> 
> We should NEVER use strcpy or strcat. Generally speaking these are what
> attackers look for when trying to find buffer overflow attacks.
> 
> You're MUCH better off to use strlcpy/strlcat.

The upstream uses strlcpy instead. I am just making a 
temporarily fix to make it work for testing :p

Thanks!
Comment on attachment 8483179 [details] [diff] [review]
AutoMounter should accomodate rndis

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

This patch is verified to fix broken usb tethering! Good job!
But I think I am not familiar with AutoMounter enough to review
this patch. Forwarding to Eric who is the original reviewer.
Thanks!
Attachment #8483179 - Flags: review?(hchang)
Attachment #8483179 - Flags: review?(echou)
Attachment #8483179 - Flags: feedback+
Kyle, did you mean to tag this bug as Graphics:Layers?
Flags: needinfo?(khuey)
Reclassifying. This is definitely Firefox OS::General
Component: Graphics: Layers → General
Flags: needinfo?(khuey)
Product: Core → Firefox OS
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
(In reply to Milan Sreckovic [:milan] from comment #21)
> Kyle, did you mean to tag this bug as Graphics:Layers?

No, I didn't.  I probably left the bug open too long before CCing myself.
Component: General → GonkIntegration
Comment on attachment 8483179 [details] [diff] [review]
AutoMounter should accomodate rndis

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

Looks good to me. The original UMS/MTP functionality doesn't seem to be affected and the patch has already been verified by Henry.
Attachment #8483179 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/196665310e4a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Based on the patch I'm going to assume that this wasn't actually Nuwa related.
No longer blocks: 977026
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #27)
> Based on the patch I'm going to assume that this wasn't actually Nuwa
> related.

Nope - this was related to the way MTP got integrated into the AutoMounter, and as you surmised had absolutely nothing to do with Nuwa.
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(dharris)
Whiteboard: [p=3][2.1-flame-test-run-1] → [p=3][2.1-flame-test-run-1][2.1-flame-test-run-2]
Triage reviewed, major functional regression -> blocking+.
blocking-b2g: 2.1? → 2.1+
Please request Aurora approval on this patch when you get a chance.
Flags: needinfo?(hchang)
Comment on attachment 8483179 [details] [diff] [review]
AutoMounter should accomodate rndis

Since the regressor 19e59c14b0aa [1] is in the mozilla-aurora tree, the patch needs to be uplifted.

http://hg.mozilla.org/releases/mozilla-aurora/rev/19e59c14b0aa

Approval Request Comment
[Feature/regressing bug #]: USB tethering
[User impact if declined]: USB tethering won't work
[Describe test coverage new/current, TBPL]: Manually testing
[Risks and why]: No risk
[String/UUID change made/needed]: No
Attachment #8483179 - Flags: approval-mozilla-aurora?
Flags: needinfo?(hchang)
Attachment #8483179 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(dharris)
Verified as fixed for the latest 2.1 Flame build:

Environmental Variables:
----------------------------------------
Device: Flame 2.1
BuildID: 20141011000201
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

When user enables USB tethering on their device, a connection is formed and internet tethering functions as expected

-

This issue is NOT fixed for the latest 2.2 Flame build:

Environmental Variables:
----------------------------------------
Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

When enabling the USB tethering, the toggle animates from "on" to "off" every time meaning it cannot be enabled.

Updating tracking flags to reflect current behaviour.
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(ktucker)
(In reply to Roland Kunkel [:RolandK] from comment #33)
> Verified as fixed for the latest 2.1 Flame build:
> This issue is NOT fixed for the latest 2.2 Flame build:

After re-flashing my device and verifying on 3 separate devices, this feature is fixed for the latest 2.2 Flame device.

Verified against all affected branches, changing bug status to "Verified Fixed"
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.