Closed Bug 879392 Opened 7 years ago Closed 7 years ago

Change - Remove support for sending input to plugins

Categories

(Firefox for Metro Graveyard :: Input, defect, P3)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

References

Details

(Whiteboard: feature=change u=metro_firefox_user c=Content_features p=1)

Attachments

(1 file, 1 obsolete file)

In bug 812351, we added code that forwards input information to windowless plugins.  Windows 8 Style Firefox will not support plugins, so we should remove the input forwarding code.
Summary: Remove support for sending input to plugins → Change - Remove support for sending input to plugins
Whiteboard: feature=change c=tbd u=tbd p=0
Is there any harm in keeping this just in case?
This is code that we intend never to execute and never to test.  I think the harm in keeping this code around is two-fold:

  1) It adds maintenance cost; this code makes MetroInput.cpp significantly more cluttered, and anyone who wants to make changes to MetroInput.cpp will have to read through it and figure out what it's doing.  If someone makes changes to MetroInput.cpp that break this code, we'll never know because we don't plan to ever test it.

  2) This code could have security vulnerabilities (and even if it doesn't have them now, it could have them introduced later). A malicious party could discover a vulnerability and trick users into enabling plugins (which is possible through a pref, and probably not very difficult to convince a user to do since it makes Flash kind of work), then exploit the vulnerability.

We can always add this code back later if we decide to support plugins.
Attached patch Patch v1 (obsolete) — Splinter Review
Since I'm messing with widget code for bug 879562 and bug 837293, I figured I'd just go ahead and do this too.

This patch should basically be the reverse of the patch in bug 812351.
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #762924 - Flags: review?(netzen)
Attached patch Patch v2Splinter Review
This patch also gets rid of the plugin input tests (which were disabled anyway)
Attachment #762924 - Attachment is obsolete: true
Attachment #762924 - Flags: review?(netzen)
Attachment #762929 - Flags: review?(netzen)
Blocks: metrov1it9
No longer blocks: metrov1defect&change
QA Contact: jbecerra
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=operations u=operations p=1
Priority: -- → P3
Comment on attachment 762929 [details] [diff] [review]
Patch v2

Review of attachment 762929 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this but I'd like jimm to give his OK since he expressed some concern in Comment 1.
Attachment #762929 - Flags: review?(netzen) → review?(jmathies)
Attachment #762929 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/14503e9ec043
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043
> 
> The patch incorrectly references bug 879382.  Oops :(

If this happens again please can you backout and reland as soon as possible, as one push, with DONTBUILD in the topmost commit message (to save build resources). The bug marking script used after a merge will handle that fine as long as the original landing and backout are in the same merge.
Target Milestone: --- → Firefox 24
(In reply to Ed Morley [:edmorley UTC+1] from comment #8)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043
> > 
> > The patch incorrectly references bug 879382.  Oops :(
> 
> If this happens again please can you backout and reland as soon as possible,
> as one push, with DONTBUILD in the topmost commit message (to save build
> resources). The bug marking script used after a merge will handle that fine
> as long as the original landing and backout are in the same merge.

Absolutely; thanks for the tip!
No problem :-)
Blocks: 892054
I tried testing using the information in bug 812351#c4, by going to about:config and changing plugin.disable to false then going to hulu.com and playing some content using touch controls. That worked.

Did this land correctly?
Hey Tim, please see Juan's question in Comment #11.
Flags: needinfo?(tabraldes)
There's code somewhere that generates plugin messages for mouse input; I think it's probably this [1] but I haven't tested to be sure.  When working on bug 812351, I must have missed the fact that mouse input was working with plugins even before applying the patch.

For verifying this bug, it would have been sufficient to ensure that keyboard input was not reaching plugins.  However, since bug 843236 landed, we use the KeyboardLayout class [2] to process all our keyboard input, and it sends input to plugins.

Basically there's no way to verify this bug because it's a lie; we always send input to plugins now (and it probably works better now than it did with the implementation that I ripped out as part of this bug).

[1] https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp?rev=5d38d2a6e4c1#2095
[2] https://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp
Flags: needinfo?(tabraldes)
Whiteboard: feature=change c=operations u=operations p=1 → feature=change u=metro_firefox_user c=Content_features p=1
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.