Closed
Bug 1185529
Opened 9 years ago
Closed 9 years ago
Flash AS2 Key.isDown recently broken
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
VERIFIED
FIXED
mozilla42
People
(Reporter: csamborski, Assigned: bobowen)
References
Details
(Keywords: regression, testcase)
Attachments
(2 files)
15.09 KB,
application/x-shockwave-flash
|
Details | |
40 bytes,
text/x-review-board-request
|
bobowen
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 Build ID: 20150719004007 Steps to reproduce: I encountered this bug when trying to play some old games flash games. (ie: http://www.hfest.net/try.html) but I made a quick swf file to demonstrate the issue. When opening the swf using the embed Flash Player in Firefox, I expect Key.isDown(keyCode) to return a boolean indicating whether or not the corresponding key is pressed. Actual results: When opened with Firefox Dev Edition, Key.isDown(keyCode) always returns false (even when the key is actually pressed). Expected results: I expect Key.isDown to reflect the real state of the key (pressed or not) as when opened in Firefox Stable or the standalone Flash Player (on Windows).
Reporter | ||
Comment 1•9 years ago
|
||
I noticed this bug 2 or 3 weeks ago. Maybe it's related to the new way to handle plug-ins because of e10s but it just a guess. PS: the swf file I made is in the attachments.
It regressed: good=2015-06-18 bad=2015-06-19 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a3f280b6f8d5&tochange=2694ff2ace6a Maybe bug Bug 1165903
Status: UNCONFIRMED → NEW
status-firefox40:
--- → unaffected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
tracking-firefox41:
--- → ?
tracking-firefox42:
--- → ?
Component: Untriaged → Plug-ins
Ever confirmed: true
Keywords: regression,
testcase
Product: Firefox → Core
Comment 3•9 years ago
|
||
FYI, It works as expected if you disabled Adobe Flash protected mode.
Updated•9 years ago
|
OS: Unspecified → Windows
Alice, can you explain me how I add an environment variable in FF to disable the sandbox, like MOZ_DISABLE_CONTENT_SANDBOX=1 or MOZ_DISABLE_NPAPI_SANDBOX?
Flags: needinfo?(alice0775)
Comment 5•9 years ago
|
||
about:addons > Plugns> Shockwave Flash Option > uncheck " Adobe Flash protected mode" OR append "ProtectedMode=0" to mms.cfg file.
Flags: needinfo?(alice0775)
I know that, I speak about Firefox, not Flash, like: "* turning off the content sandbox altogether by setting an environment variable MOZ_DISABLE_CONTENT_SANDBOX to 1 before starting Firefox."
Comment 7•9 years ago
|
||
(In reply to Loic from comment #6) > I know that, I speak about Firefox, not Flash, like: > "* turning off the content sandbox altogether by setting an environment > variable MOZ_DISABLE_CONTENT_SANDBOX to 1 before starting Firefox." Sorry, I do not know.
Component: Plug-ins → Security: Process Sandboxing
Flags: needinfo?(bobowen.code)
Assignee | ||
Comment 8•9 years ago
|
||
Loic - you can use an environment variable MOZ_DISABLE_NPAPI_SANDBOX=1 to disable our NPAPI sandbox (it's not enabled by default). This is not Flash's protected mode. You can set environment variables for all processes for your user in "Control Panel"->System->"Advanced system settings". But you would need to remember to remove them afterwards. I think that a better option is to open a command prompt and use the "set" command, e.g.: set MOZ_DISABLE_NPAPI_SANDBOX=1 and then run Firefox from the command prompt, that way the environment variable is only set for that process (and any it creates).
Flags: needinfo?(bobowen.code)
Assignee | ||
Comment 9•9 years ago
|
||
csamborski - have you got our NPAPI sandbox turned on and at level 2, that is have you got the following pref set: dom.ipc.plugins.sandbox-level.flash=2
Flags: needinfo?(csamborski)
Reporter | ||
Comment 10•9 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #9) > csamborski - have you got our NPAPI sandbox turned on and at level 2, that > is have you got the following pref set: > dom.ipc.plugins.sandbox-level.flash=2 I have dom.ipc.plugins.sandbox-level.flash=0 (apparently it's the default value) - When(In reply to Alice0775 White from comment #5) > about:addons > Plugns> Shockwave Flash Option > uncheck " Adobe Flash > protected mode" > OR append "ProtectedMode=0" to mms.cfg file. When I disable protected mode in about:addons, Key.isDown works fine. (But it should work even if protected mode is enabled, right ?) Firefox stable (39) does not have this option (& Key.isDown works) so it seems related to this protected mode.
Flags: needinfo?(csamborski)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to csamborski from comment #10) > (In reply to Bob Owen (:bobowen) from comment #9) > > csamborski - have you got our NPAPI sandbox turned on and at level 2, that > > is have you got the following pref set: > > dom.ipc.plugins.sandbox-level.flash=2 > > I have dom.ipc.plugins.sandbox-level.flash=0 (apparently it's the default > value) OK, my previous changes for our own sandbox must have broken this for protected mode as well as our low integrity sandbox (bug 1180684). Looks like I'll have to back all this out if I can't find a fix. csamborski - thanks for spotting this!
Reporter | ||
Comment 12•9 years ago
|
||
Ok, just sorry I took me so long to report it. (I did not checked lots of websites when I first seen this problem so I simply assumed that it was affecting every Flash movie using the keyboard and that it will be already reported)
Recent regression. Tracked.
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75342bb8565
Assignee | ||
Comment 15•9 years ago
|
||
Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm
Attachment #8639879 -
Flags: review?(jmathies)
Assignee | ||
Comment 16•9 years ago
|
||
Flash depends on our child window having the correct parent before being passed to setwindow. Flash appears to use AttachThreadInput to get the keyboard input to work, so my guess is that it uses the parent of the child window to get to the correct thread for that call. This patch undoes some of my changes from bug 1165903 and instead adds a new call to create (and parent) the child NPAPI window upfront. I think that I might be able to expand the use of CreateChildPluginWindow to create the surrogate for windowless plugins, to improve that call chain that you were worried about in my patch for bug 1182411. I thought that this change fixed the same issue for our sandbox (bug 1180684), but unfortunately it does not. Although it gives me some hope of fixing it.
Assignee: nobody → bobowen.code
Comment 17•9 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #16) > Flash depends on our child window having the correct parent before being > passed to setwindow. Not surprising, they love to walk up our window chain and muck around with the browser window handle. > Flash appears to use AttachThreadInput to get the keyboard input to work, so > my guess is that it uses the parent of the child window to get to the > correct thread for that call. What makes you think this? Also, I'm a bit surprised since the two threads are already bound thanks to win32 ux related calls that manipulate windows. (Actually I'm not that surprised flash would pull a stunt like this, I just don't understand why it fixes anything since, the two thread queues should already be bound.)
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #17) > (In reply to Bob Owen (:bobowen) from comment #16) > > Flash appears to use AttachThreadInput to get the keyboard input to work, so > > my guess is that it uses the parent of the child window to get to the > > correct thread for that call. > > What makes you think this? I can see it being hit in debug in their broker process. Attaching the input from our main firefox UI thread to a thread in that process. It seems to be on plugin focus. Then another call to reset when focus is lost.
Comment 19•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm https://reviewboard.mozilla.org/r/14255/#review13275 sorry this took so long, I needed some time to study it. Generally I think this will be ok, I wish that create call was async, but doesn't look like that can happen. We should also cc in aaron klotz since this might impact async plugin init.
Attachment #8639879 -
Flags: review?(jmathies) → review+
Comment 20•9 years ago
|
||
A landing that might impact async plugin work.
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm
Assignee | ||
Comment 23•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=079e5c7466b9
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm Making this obsolete because MozReview didn't seem to update the patch. See latest MozReview request for the latest patch.
Attachment #8639879 -
Attachment is obsolete: true
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm Ah looks like I can't do that.
Attachment #8639879 -
Attachment is obsolete: false
Attachment #8639879 -
Flags: review+
Assignee | ||
Comment 26•9 years ago
|
||
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce43b991306ff30a8f3c62430fd285e7b69a131c changeset: ce43b991306ff30a8f3c62430fd285e7b69a131c user: Bob Owen <bobowencode@gmail.com> date: Fri Aug 07 08:48:16 2015 +0100 description: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r=jimm
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm Approval Request Comment [Feature/regressing bug #]: Patch for bug 1165903 regressed this. [User impact if declined]: Flash content using Action Script 1 or 2 and that needs keyboard interaction (mainly games) will not work . [Describe test coverage new/current, TreeHerder]: There are some NPAPI plugin tests on TreeHerder, but they don't seem to catch many plugin specific issues. I have applied this patch to Aurora (it applies cleanly) and checked the attached test. I've tested a few flash games and video on BBC and Amazon. I've also tested Amazon and Netflix using Silverlight. [Risks and why]: Medium - I would say low, but the NPAPI code seems to be fairly fragile and can affect the various plugins in different ways. However, this patch now puts the order of the various calls back to how they were before bug 1165903, just with the parenting of the child window being done in the main process. So, this is a safer change than the one we already have. If we don't take this, we should back out bugs 1165903 and 1179972, which was landed on top of it. But this would mean that we would be unable to use the NPAPI sandbox until Firefox 42. [String/UUID change made/needed]: None Also re-instated the r+ from jimm in comment 22, which I accidentally removed.
Attachment #8639879 -
Flags: review+
Attachment #8639879 -
Flags: approval-mozilla-aurora?
Comment 28•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce43b991306f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm I missed the merge, so this is now a request for Beta. See comment 27 for the original uplift request.
Attachment #8639879 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Hello there, could you please verify the fix works as expected on the latest Nightly? Thanks.
Flags: needinfo?(csamborski)
Comment on attachment 8639879 [details] MozReview Request: Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm Patch has been on Nightly for a week, seems safe to uplift to Beta.
Attachment #8639879 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 33•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #30) > Hello there, could you please verify the fix works as expected on the latest > Nightly? Thanks. Sorry for the delay but it is still broken on the latest Nightly: Application Basics ------------------ Name: Firefox Version: 43.0a1 Build ID: 20150828030205 Update Channel: nightly User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0 Multiprocess Windows: 1/1 (default: true) Safe Mode: false
Flags: needinfo?(csamborski)
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to csamborski from comment #33) > (In reply to Ritu Kothari (:ritu) from comment #30) > > Hello there, could you please verify the fix works as expected on the latest > > Nightly? Thanks. > > Sorry for the delay but it is still broken on the latest Nightly: > > Application Basics > ------------------ > Name: Firefox > Version: 43.0a1 > Build ID: 20150828030205 > Update Channel: nightly > User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 > Firefox/43.0 > Multiprocess Windows: 1/1 (default: true) > Safe Mode: false It looks like you're using the 64-bit version of Nightly there. This has our own sandbox turned on, because there is no protected mode. The Key.isDown problem is still a known regression with that. Would you mind trying 32-bit Nightly with protected mode enabled (which is the default). Thanks.
Flags: needinfo?(csamborski)
Reporter | ||
Comment 35•9 years ago
|
||
(In reply to Bob Owen (:bobowen) (on PTO) from comment #34) > It looks like you're using the 64-bit version of Nightly there. > > This has our own sandbox turned on, because there is no protected mode. > The Key.isDown problem is still a known regression with that. > > Would you mind trying 32-bit Nightly with protected mode enabled (which is > the default). > > Thanks. I just downloaded the 32-bit Nightly and it works fine for me. Sorry for the mistake (I didn't know that the 64-bit version had a different protection)
Flags: needinfo?(csamborski)
Updated•9 years ago
|
status-firefox43:
--- → verified
Comment 36•9 years ago
|
||
Tested with dom.ipc.plugins.sandbox-level.flash=0 (default value). I could reproduce the issue with Firefox 41 beta 1 under Win 7 64-bit with both the testcase and the game from comment 0. The issue is no longer reproducible using Firefox 41 beta 8 and latest Dev Edition 42.0a2 2015-09-09 under Win 7 64-bit, but the game / demo must be clicked before. This also happens on Firefox 40RC, so I'm marking this versions as verified.
You need to log in
before you can comment on or make changes to this bug.
Description
•