Closed Bug 1394024 Opened 3 years ago Closed 3 years ago

[Windows only] Firefox crashes on unplugging microphone

Categories

(Core :: Plug-ins, defect)

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- fixed
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: vilee, Assigned: handyman)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

Steps to reproduce:

1. Make sure that:
   (a) Flash Player is installed. If not, please install it from here: https://get.adobe.com/flashplayer
   (b) The Windows system should only have one microphone attached. The bug will not reproduce if more than one mic is present.

2. Start Firefox and navigate to any Flash content like this one: https://helpx.adobe.com/content/dam/help/en/flash-player/assets/flash_tree.swf
3. Right-click to bring up the Flash Player context menu and select "Settings"
4. Click the microphone icon (4th from the left) in the "Adobe Flash Player Settings" dialog
5. Now unplug the microphone


Actual results:

Firefox crashes at mozilla::plugins::PluginUtilsWin::AudioNotification::OnDefaultDeviceChanged

* FAIL: Firefox 55.0.2 (32-bit & 64-bit) on Win7 x64 physical machine
* FAIL: Firefox 55.0.2 (32-bit & 64-bit) on Win8.1 x64 physical machine
* FAIL: Firefox 55.0.2 (32-bit & 64-bit) on Win10 x64 physical machine
  PASS: Chrome 60 on Win7 x64 physical machine
  PASS: Firefox 55.0.2 on Mac 10.12

Crash reports:
(1) Firefox 32-bit: https://crash-stats.mozilla.com/report/index/84ba55b7-bdaf-4616-a012-eb5d61170825#allthreads
(2) Firefox 64-bit: https://crash-stats.mozilla.com/report/index/19740e0d-64ab-429c-b118-a67e81170825#allthreads


Expected results:

There should be no crash on unplugging the microphone (as in step #5).
Crash Signature: [@ std::char_traits<T>::length]
Component: Untriaged → Audio/Video
Keywords: crash
Product: Firefox → Core
Version: 52 Branch → 55 Branch
It looks more related to plugin. 
Lets have plug-ins team to check first.
Component: Audio/Video → Plug-ins
Depends on: 1339259
Hey David, thisd looks like a crash associated with those mic changes we made. Can you take a look? Might be related to bug 1339259 although that bug is win7 only.
Flags: needinfo?(davidp99)
Patch is pretty obvious.  I'd add to the STR in comment 0 that an internal mic counts as an available mic.  If you want to test this on a machine with an internal mic then you need to disable the hardware from the Sound control panel.  

---

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b31f4d9d1dbabc289d5a69aea8fce3a1098789e
Flags: needinfo?(davidp99)
Attachment #8907289 - Flags: review?(jmathies)
Assignee: nobody → davidp99
Attachment #8907289 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f81c3cd701a
Handle null string when alerting plugin about Windows microphone change. r=jimm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f81c3cd701a
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I guess it might be a bit late to get this into 56.  Please request uplift if you want to get this into the release candidate anyway, or set to wontfix?
Flags: needinfo?(davidp99)
Comment on attachment 8907289 [details] [diff] [review]
Handle null string as name of device when no mic is available

This fix is trivial.  Lets try for the uplift.

Approval Request Comment
[Feature/Bug causing the regression]:
Flash player when removing the active microphone

[User impact if declined]:
Browser crash every time the last mic is removed

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Its one very simple line of code

[String changes made/needed]:
None
Flags: needinfo?(davidp99)
Attachment #8907289 - Flags: approval-mozilla-beta?
Comment on attachment 8907289 [details] [diff] [review]
Handle null string as name of device when no mic is available

56 is on m-r now.
Attachment #8907289 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Comment on attachment 8907289 [details] [diff] [review]
Handle null string as name of device when no mic is available

This is pretty frequent on ESR52 too. Given that it's a one-liner, we should probably take it there too.
Attachment #8907289 - Flags: approval-mozilla-esr52?
Comment on attachment 8907289 [details] [diff] [review]
Handle null string as name of device when no mic is available

Moderate crash rate but since the fix looks simple let's uplift for the 56 RC and for ESR.
Attachment #8907289 - Flags: approval-mozilla-release?
Attachment #8907289 - Flags: approval-mozilla-release+
Attachment #8907289 - Flags: approval-mozilla-esr52?
Attachment #8907289 - Flags: approval-mozilla-esr52+
(In reply to David Parks (dparks) [:handyman] from comment #7)
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No
Updating the qe-verify flag, per David's assesment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.