Closed Bug 1062550 Opened 10 years ago Closed 10 years ago

b2g crashes during USB tethering

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

VERIFIED FIXED
2.1 S5 (26sep)
blocking-b2g 2.0+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: tkundu, Assigned: hchang)

References

Details

(Keywords: crash, Whiteboard: [caf-crash 340][caf priority: p1][CR 717884][b2g-crash])

Crash Data

Attachments

(5 files, 1 obsolete file)

STR: 1) Run random stability test with USB tethering enabled: Logs : https://drive.google.com/file/d/0B1cSMS8_GuAEUF9zSWdLTGlyV3c/edit?usp=sharing
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Component: Stability → GonkIntegration
Keywords: crash
Whiteboard: [CR 717884] → [CR 717884][b2g-crash]
Whiteboard: [CR 717884][b2g-crash] → [caf priority: p1][CR 717884][b2g-crash]
Whiteboard: [caf priority: p1][CR 717884][b2g-crash] → [caf-crash 340][caf priority: p1][CR 717884][b2g-crash]
Observed on: Device: msm8226 Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.074 Moz BuildID: 20140826000204 B2G Version: 2.0 Gecko Version: 32.0 Gaia: http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=a51633e29a7826b6bf07cb1c5ad81b3217b9820a Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=8f121f8fa5fa1a3e82967f91e2afe337465fc593
Flags: needinfo?(khuey)
Please attach the minidump. Maybe a dup of bug 1062548 looking at the dump over here though. Both have libxul.so!NetworkUtils::setUSBTethering(NetworkParams&) [NetworkUtils.cpp : 1611 + 0x13] on the stack
Flags: needinfo?(tkundu)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #3) > Please attach the minidump. Maybe a dup of bug 1062548 looking at the dump > over here though. Both have > libxul.so!NetworkUtils::setUSBTethering(NetworkParams&) [NetworkUtils.cpp : > 1611 + 0x13] on the stack here is the decoded minidump from logs in Comment 0.
Flags: needinfo?(tkundu)
.extra file from logs in Comment 0. Please note that link in Comment 0 also has full logcat logs when this issue happens.
Bhavana -- can you please get someone to look into it asap. It's affecting out stability and high priority issue.
Flags: needinfo?(bbajaj)
I'm going to give this one to Henry since he owns bug 1061993. He can figure out if he's the right owner when he wakes up.
Assignee: nobody → hchang
Component: GonkIntegration → Wifi
Flags: needinfo?(khuey)
Inder, we'll have to wait for Taipei to wake up to move here, I'll get in touch with them.
Flags: needinfo?(bbajaj)
It doesn't seem to be the dupe of Bug 1061993 at first glance. Will update here when I've done the preliminary investigation. Thanks.
According to attachment 8483947 [details], the process which kept crashing was dnsmasq. Is it the right log? Also, is the usb tethering function totally broken? Or it just crashes from time to time?
Flags: needinfo?(tkundu)
I am wondering if the crashing of dnsmasq causes netd to return abnormal value and in turn NetworkUtils.cpp doesn't handle the error case well. Is the debug flag able to open to gather more information in the adb log? [1] http://git.mozilla.org/?p=releases/gecko.git;a=blob;f=dom/system/gonk/NetworkUtils.cpp;h=59534d31735b0390fc1351d50a6f1a8eed3c4a24;hb=8f121f8fa5fa1a3e82967f91e2afe337465fc593#l28
I debugged this issue further on our device by enabling additional logs . setUSBTethering JS functtion is called with 'true' twice consecutively which is breaking setdnmasq binary and causing b2g process to crash Could you please make sure that setUSBTethering JS functtion is called only once if USB tethering already set ?
Flags: needinfo?(tkundu)
Could you please give me quick fix for my comment in Comment 12 ? I think that this will solve this issue.
Flags: needinfo?(hchang)
@henry: One more hints : If you try to find out why we are seeing this line 09-05 00:08:18.456 194 742 D TetherController CAFLOG: Sending update msg to dnsmasq [update_ifaces:rndis0:rndis0] in logs of comment 12 then you will understand why we should not call setUSBTethering JS function again when USB tethering is already set.
Hi henry, I don't see you in #gaia/#b2g IRC. Could you please let me know your IRC channel name so that we can discuss and get a quick fix for this bug ?
(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #12) > Created attachment 8484849 [details] > more logs by enabling debug flag in gaia/gecko/dnsmasq > > I debugged this issue further on our device by enabling additional logs . > > setUSBTethering JS functtion is called with 'true' twice consecutively which > is breaking setdnmasq binary and causing b2g process to crash ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Did you also find why? > > Could you please make sure that setUSBTethering JS functtion is called only > once if USB tethering already set ? I need some time to deal with this. I could take a quick look and update here before leaving. Thanks.
Flags: needinfo?(hchang)
(In reply to Henry Chang [:henry] from comment #17) > (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from > comment #12) > > Created attachment 8484849 [details] > > more logs by enabling debug flag in gaia/gecko/dnsmasq > > > > I debugged this issue further on our device by enabling additional logs . > > > > setUSBTethering JS functtion is called with 'true' twice consecutively which > > is breaking setdnmasq binary and causing b2g process to crash > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Did you also find why? I guess it's due to NetworkUtils.cpp maintains only one command chain so if one request is received before the previous one finishes, something would be messed up. > > > > Could you please make sure that setUSBTethering JS functtion is called only > > once if USB tethering already set ? > > I need some time to deal with this. I could take a quick look > and update here before leaving. Thanks.
Attached patch Bug1062550.patch (obsolete) — Splinter Review
I found quickly toggling the usb tethering settings may screw up the internal state. In particular, the callback function [1] references the state variable which might be different from the request. Bind the action(enable or disable) to the callback to mitigate this issue. I haven't gone through all the possible transition. If this bug is easily to reproduce it's worthy of trying this patch out. I'll continue to dig deeper next week. Thanks. [1] http://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/2fae20afe1fa/dom/system/gonk/NetworkManager.js#l1119
Observed on: Device: msm8226 Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.080 Moz BuildID: 20140903000206 B2G Version: 2.0 Gecko Version: 32.0 Gaia: http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=449d8db9b3ea1f9262db822c37ef2143477172b7 Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=be914f092bb7916e1c5f33fc94aa4217cd375b65
Comment on attachment 8484904 [details] [diff] [review] Bug1062550.patch Review of attachment 8484904 [details] [diff] [review]: ----------------------------------------------------------------- I added following change on top of your patch : 52 + (this._usbTetheringAction === TETHERING_STATE_ONGOING || 53 + this._usbTetheringAction === TETHERING_STATE_ACTIVE)) { 54 + debug("Usb tethering already connecting/connected."); 55 ++ if(this._requestCount > 0) { 56 ++ this._requestCount--; 57 ++ debug("handleUSBTetheringToggle this._requestCount "+this._requestCount); 58 ++ } 59 + return; 60 + } 61 + 62 + if (!enable && 63 + this._usbTetheringAction === TETHERING_STATE_IDLE) { 64 + debug("Usb tethering already disconnected."); 65 ++ if(this._requestCount > 0) { 66 ++ this._requestCount--; 67 ++ debug("handleUSBTetheringToggle this._requestCount "+this._requestCount); 68 ++ } Then it worked for me :) . I would suggest you to do these changes and review it again before landing. Thanks for quick help :)
Attachment #8484904 - Flags: review-
Attachment #8484904 - Flags: feedback-
Flags: needinfo?(hchang)
blocking-b2g: 2.0? → 2.0+
Hi Tapas, I don't think we need to decrease |this._requestCount|. Have you verified (actual auto test) the change you made on top of mine is necessary? Thanks! (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #21) > Comment on attachment 8484904 [details] [diff] [review] > Bug1062550.patch > > Review of attachment 8484904 [details] [diff] [review]: > ----------------------------------------------------------------- > > I added following change on top of your patch : > > 52 + (this._usbTetheringAction === TETHERING_STATE_ONGOING || > 53 + this._usbTetheringAction === TETHERING_STATE_ACTIVE)) { > 54 + debug("Usb tethering already connecting/connected."); > 55 ++ if(this._requestCount > 0) { > 56 ++ this._requestCount--; > 57 ++ debug("handleUSBTetheringToggle this._requestCount > "+this._requestCount); > 58 ++ } > 59 + return; > 60 + } > 61 + > 62 + if (!enable && > 63 + this._usbTetheringAction === TETHERING_STATE_IDLE) { > 64 + debug("Usb tethering already disconnected."); > 65 ++ if(this._requestCount > 0) { > 66 ++ this._requestCount--; > 67 ++ debug("handleUSBTetheringToggle this._requestCount > "+this._requestCount); > 68 ++ } > > Then it worked for me :) . > > I would suggest you to do these changes and review it again before landing. > Thanks for quick help :)
Flags: needinfo?(hchang)
(In reply to Henry Chang [:henry] from comment #22) > Hi Tapas, > > I don't think we need to decrease |this._requestCount|. > Have you verified (actual auto test) the change you made > on top of mine is necessary? yes, I verified by rapid firing usb tethering button on/off in settings. I hit same crash if I don't apply my change on top of your change. Thanks for good work
Flags: needinfo?(hchang)
Cool! I will also have it a try tmr and proceed in review/landing the patch. Thanks! (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #23) > (In reply to Henry Chang [:henry] from comment #22) > > Hi Tapas, > > > > I don't think we need to decrease |this._requestCount|. > > Have you verified (actual auto test) the change you made > > on top of mine is necessary? > > > yes, I verified by rapid firing usb tethering button on/off in settings. I > hit same crash if I don't apply my change on top of your change. Thanks for > good work
Flags: needinfo?(hchang)
Attachment #8484904 - Attachment is obsolete: true
(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #23) > (In reply to Henry Chang [:henry] from comment #22) > > Hi Tapas, > > > > I don't think we need to decrease |this._requestCount|. > > Have you verified (actual auto test) the change you made > > on top of mine is necessary? > > > yes, I verified by rapid firing usb tethering button on/off in settings. I > hit same crash if I don't apply my change on top of your change. Thanks for > good work I still cannot hit the crash without your change but I've already realized it's necessary. Besides, another change also needs to be made to avoid interleaving usb tethering enable/disable chain. Attached the final patch for review which includes three changes.
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) Hi Vincent, I flagged you for reviewing first since you are the original author of usb tethering. This patch makes 3 significant changes regarding rapidly toggling usb tethering: 1) Bind the action (enable/disable) to the callback function |usbTetheringResultReport|. 2) Set |this._requestCount| to 1 instead of 0 to prevent from the subsequent settings change triggering another |handleUSBTetheringToggle| before the previous one finishes. 3) Reset |this._requestCount| to 0 whenever the request doesn't take effect. Thanks!
Attachment #8486964 - Flags: review?(vchang)
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) Review of attachment 8486964 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thank you.
Attachment #8486964 - Flags: review?(vchang) → review+
Hi Tapas, Given the r+'d patch, do you want to include the patch to have an final automation test before landing? Thanks!
Flags: needinfo?(tkundu)
(In reply to Henry Chang [:henry] from comment #29) > Hi Tapas, > > Given the r+'d patch, do you want to include the patch to have > an final automation test before landing? > > Thanks! Patch looks fine to me :)
Flags: needinfo?(tkundu)
Hi Henry, please land this, thanks.
Flags: needinfo?(hchang)
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): USB tethering [User impact] if declined: Rapidly switch USB tethering on and off will cause b2g crashing [Testing completed]: Yest [Risk to taking this patch] (and alternatives if risky): No [String changes made]: No
Attachment #8486964 - Flags: approval-gaia-v2.0?
Flags: needinfo?(hchang)
Keywords: checkin-needed
Attachment #8486964 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) Switching back the approval as this needs to land on master and get FIXED first. Also does this patch apply to 2.1?
Attachment #8486964 - Flags: approval-gaia-v2.0+ → approval-gaia-v2.0?
Hi Henry, Gecko patch is to request approval flag: approval‑mozilla‑b2g32 (v2.0). If the patch also apply to v2.1, please request the following approval too: approval‑mozilla‑aurora.
Flags: needinfo?(hchang)
Oops I flagged wrong one... sorry for that.
Flags: needinfo?(hchang)
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): USB tethering User impact if declined: Rapidly switching usb tethering on and off will cause b2g crash. Testing completed: Yes Risk to taking this patch (and alternatives if risky): No String or UUID changes made by this patch: No
Attachment #8486964 - Flags: approval-mozilla-b2g32?
Attachment #8486964 - Flags: approval-mozilla-b2g30?
Attachment #8486964 - Flags: approval-gaia-v2.0?
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) Approval Request Comment [Feature/regressing bug #]: USB tethering [User impact if declined]: Rapidly switch USB tethering on/off will cause b2g to crash [Describe test coverage new/current, TBPL]: [Risks and why]: No risk. [String/UUID change made/needed]: No
Attachment #8486964 - Flags: approval-mozilla-b2g32? → approval-mozilla-aurora?
Attachment #8486964 - Attachment description: Patch V2 → Patch V2 (For 2.0/2.1/master)
Comment on attachment 8486964 [details] [diff] [review] Patch V2 (For 2.0/2.1/master) b2g30 is v1.4, so I'm assuming that changing the nom from b2g32 was in error.
Attachment #8486964 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g32?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Attachment #8486964 - Flags: approval-mozilla-b2g32?
Attachment #8486964 - Flags: approval-mozilla-b2g32+
Attachment #8486964 - Flags: approval-mozilla-aurora?
Attachment #8486964 - Flags: approval-mozilla-aurora+
Verifyme to check if this is fixed on 2.1
Keywords: verifyme
Cannot verify on bug due to automation steps as we cannot run these tests at our location.
QA Whiteboard: [QAnalyst-Triage?][qanalyst-verify]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][qanalyst-verify] → [QAnalyst-Triage?][qanalyst-verify-]
QA Whiteboard: [QAnalyst-Triage?][qanalyst-verify-] → [QAnalyst-Triage?][QAnalyst-verify-]
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Verified fixed on Flame device running b2g v2.1 Test Environment Device: Flame Base image: v188 Gaia: c97463d61f45513a2123b19610386ddbfc916819 Gecko: 4f8c0c021128 Build-ID: 20141027001201 Platform Version: 34.0 OS Version: 2.1.0.0-prerelease Update Channel: nightly-b2g34 Git Commit Info: 2014-10-24 21:57:55 Steps to reproduce: 1. Connect the phone to the Internet via WiFi or cellular data connection. 2. Connect the phone to a computer via a USB cable. 3. On the phone, go to Settings > Internet Sharing. 4. Tap rapidly on the USB tethering checkbox to turn it on and off successively. Expected Results: The phone remains stable and does not crash. Actual Results: The phone remains stable and does not crash.
Status: RESOLVED → VERIFIED
Verify passed, this issue can't be repro on Woodduck 2.0;Flame2.0&2.2 Attached: Verify_Woodduck_USB.mp4 Reproducing rate: 0/5 Woodduck build: Gaia-Rev cc690f8016b672475dc186bc7fd58aef45e684b7 Gecko-Rev 03d3ab62d5b07b915434f2d1d68495ad5915ecd2 Build-ID 20141118184148 Version 32.0 Flame2.0 build: Gaia-Rev 1ede2666f1e6c1b3fd3b282011caf0cbc59544b0 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/faa64077b0c2 Build-ID 20141119000207 Version 32.0 FLame2.2 build: Gaia-Rev e64428c5b2dce5db90b75a5055077a04f4bd4819 Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/aa72ddfe9f93 Build-ID 20141119160202 Version 36.0a1
Attached video Verify_Woodduck_USB.MP4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: