Closed Bug 1185529 Opened 9 years ago Closed 9 years ago

Flash AS2 Key.isDown recently broken

Categories

(Core :: Security: Process Sandboxing, defect)

41 Branch
Unspecified
Windows
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + verified
firefox42 + verified
firefox43 --- verified

People

(Reporter: csamborski, Assigned: bobowen)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

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).
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
Component: Untriaged → Plug-ins
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
FYI, It works as expected if you disabled Adobe Flash protected mode.
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)
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."
(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)
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)
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)
(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)
(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!
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)
Blocks: 1185532
Bug 1185529: On Windows, ensure that NPAPI child window has the correct parent before setwindow is called. r?jimm
Attachment #8639879 - Flags: review?(jmathies)
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
(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.)
(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 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+
A landing that might impact async plugin work.
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
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
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+
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
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?
Blocks: 1165903
https://hg.mozilla.org/mozilla-central/rev/ce43b991306f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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+
Flags: qe-verify+
(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)
(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)
(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)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: