1187724 - Flash hangs on BBC iPlayer after changing system volume through keyboard

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-723e84db-2f46-407e-b148-a81332150726.
=============================================================

i can reproduce the plugin hang in a new profile and the current version of flash 18.0.0.209 with the following steps (due to the geoblocking on the bbc site you'd need to be in the uk or have a uk vpn):
- load a video on bbc iplayer like http://www.bbc.co.uk/iplayer/episode/p01rk00j/architecture-at-the-crossroads-1-doubt-and-reassessment
- click on the play button on the video (to start the video & put focus on the flash content)
- hit volume up/down keys on the keyboard (i have a logitech k260): this is causing the video to stop, the sound continues in the background and you will the plugin hanging notification dialog after a few secons. a crash report with the mentioned signature is triggered when you stop the plugin
(- when you try to play the video after reloading the page without clearing the cache, the site gives the error "This content doesn't seem to be working. Please try again later.")

i don't get the hang on a windows 32bit os or when e10s is turned on.

the regression range for this problem is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=67872ce17918&tochange=88037f94b7d7 so it appears to be caused by the asynchronous plugin initialisation work in bug 998863, which landed in firefox 39 release for the first time.

Comment 1

[Robert Kaiser]

(In reply to philipp from comment #0)
> the regression range for this problem is
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=67872ce17918&tochange=88037f94b7d7 so it appears to
> be caused by the asynchronous plugin initialisation work in bug 998863,
> which landed in firefox 39 release for the first time.

It was deactivated in 39 release. Can you confirm that it does not affect 39 release but does affect the latest 40 Beta?

If so, please nominate for tracking 40.

Comment 2

[[:philipp]]

sorry for mentioning 39 before, i was just looking at the target milestone for bug 1116806...

so i tested it again, but the issue looks less conclusive now & i cannot reproduce the issue reliably like before (so maybe other factors like connection speed play a part in it as well, and asyncplugininit may only aid to that condition).

some more findings though - the issue is occurring on windows 32bit as well; i was mistaken when i said otherwise before: https://crash-stats.mozilla.com/report/index/febbec99-d5f6-4f06-9c5a-353142150726
and there's a different signature on beta: https://crash-stats.mozilla.com/report/index/c105ef34-18cc-4a92-be4e-7fffc2150726
so the signatures are similar as in bug 676361, needinfoing masayuki who has fixed it before...

Comment 3

[Robert Kaiser]

As philipp seems to think there is some connection to when async plugin init landed, aklotz, can you take a look?

Comment 4

[Robert Kaiser]

[Tracking Requested - why for this release]:
Also, if this happens with high enough probability, it should at least be on the radar of release management.

Comment 5

[jen strickland :jenstrickland]

Tracked for 40, since this is a crash happening sufficiently enough that relman should keep an eye on it.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

This is not specifically related to async init. It's a typical deadlock that we've seen due to Windows messaging.

Comment 7

[Benjamin Smedberg]

Is it reproducible in such a way that we can collect full dumps and really fix it? Why does the regression range point to async init?

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

We're at the end of the 40 cycle. This bug is now wontfix for 40.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> - hit volume up/down keys on the keyboard (i have a logitech k260): this is causing the video to stop,
> the sound continues in the background and you will the plugin hanging notification dialog after a few
> secons. a crash report with the mentioned signature is triggered when you stop the plugin

Hmm, could you check if the volume down/up causes key event in following page?
https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html

If key events are fired, might be a regression of bug 865561. Although, WM_APPCOMMAND shouldn't be sent to us when plugin has focus...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35880aec58ee

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you check if my test patch fixes this bug with following test build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b6e688f6fa8a/try-win32/

Comment 12

[[:philipp]]

Attachment: Keyboard Events - Firefox Developer Edition.png

thank you, on the key event test page i got a keyCode 183/'VolumeUp'

unfortunately with the test build the issue was still present:
https://crash-stats.mozilla.com/report/index/7925206e-fb6e-4b68-8060-48d092150803

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aebfa024ef1

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you. But I found a bug in the my patch. Could you test with this again?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f3e31e65e686/try-win32/

Comment 15

[[:philipp]]

i could still reproduce the hang in that try build: https://crash-stats.mozilla.com/report/index/b54015e5-2c92-4a55-8191-b9a452150804

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to philipp from comment #15)
> i could still reproduce the hang in that try build:
> https://crash-stats.mozilla.com/report/index/b54015e5-2c92-4a55-8191-
> b9a452150804

Thanks. Then, my bug (bug 865561) might not be the cause...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=944cb45c33da

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the page tries to communicate the plugin at a keydown or a keyup event fired, this patched build might fix this bug. Could you try this?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d01299c47e4/try-win32/

Comment 19

[[:philipp]]

hi & thank you again. this last try build looks good - i could no longer reproduce the plugin hang while testing with that one!

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Looks like Masayuki has a fix for this.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Don't dispatch KeyboardEvents when the target of WM_APPCOMMAND is a windowed plug-in for preventing deadlock

So, it seems that when a windowed plug-in has focus, WM_APPCOMMAND is sent to the plug-in window first. Then, it calls DefWindowProc(), its parent window (i.e., our window) receives WM_APPCOMMAND again. In this case, dispatching keyboard event may cause deadlock if JS tries to access the plug-in synchronously.

So, we shouldn't dispatch key events even if we receive WM_APPCOMMAND but its target is not our process's window.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/06b7b3ee15095221cdd31417c6d2f3d329f18d24
changeset:  06b7b3ee15095221cdd31417c6d2f3d329f18d24
user:       Masayuki Nakano <[e-mail]>
date:       Mon Aug 10 23:54:18 2015 +0900
description:
Bug 1187724 Don't dispatch KeyboardEvents when the target of WM_APPCOMMAND is a windowed plug-in for preventing deadlock r=jimm

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06b7b3ee1509

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8644132 [details] [diff] [review]
Don't dispatch KeyboardEvents when the target of WM_APPCOMMAND is a windowed plug-in for preventing deadlock

This was tracked for 40, but we missed. I think that it's worthwhile to retry because this patch enough simple and low risk.

Approval Request Comment
[Feature/regressing bug #]: bug 1187724
[User impact if declined]: users may meet this bug when they press volume key of multimedia keyboard when a flash player has focus and the page tries to access it from keydown or keyup event listener.
[Describe test coverage new/current, TreeHerder]: landed on m-c a couple of weeks ago (I forgot to request to uplift).
[Risks and why]: Low. We haven't dispatched key events when multimedia keys are pressed before bug1187724. This patch stops it when the keys are pressed on plugin window which belongs to different process.
[String/UUID change made/needed]: no.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8644132 [details] [diff] [review]
Don't dispatch KeyboardEvents when the target of WM_APPCOMMAND is a windowed plug-in for preventing deadlock

Patch has been in Nightly for two weeks, let's uplift to Aurora42 and Beta41.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6ed87e5d641

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5cdd80a3cfd