64-bit Firefox on Windows - NPN_GetValue() passes an invalid HWND to PrintDlg()

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jeclark, Assigned: handyman)

Tracking

(Blocks: 1 bug, {64bit, flashplayer, regression})

51 Branch
mozilla57
64bit, flashplayer, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8895575 [details]
PrintTest.zip

Method: 
Unzip the the attached project.
With Flash Player installed and enabled, load bin-debug/PrintTest.html in Firefox
Click anywhere on the stage (the block of text)

Result: A print dialogue does not appear.

Expected: A print dialogue should appear.


Developer Notes: 

Last known working build from firefox is 
https://download-origin.cdn.mozilla.net/pub/firefox/nightly/2017/01/2017-01-16-13-31-20-mozilla-release-l10n/firefox-51.0.en-GB.win64.installer.exe

It is 64bit only and regression in FF side. The reason is that the PrintDlg() (Win32 API) fails with error (0xFFFF), which means an "Invalid Handle" is passed to PrintDlg().

The player in 64bit FF passes a parent HWND, which it got from FF through NPN_GetValue(), to PrintDlg() as it is running in windowless mode.

We need help from Mozilla to figure out why PrintDlg fails with the HWND supplied by FF.
Component: Untriaged → Plug-ins
Keywords: 64bit, flashplayer, regression
Product: Firefox → Core

Comment 1

a year ago
Probably sandboxing, although we have been touching printing recently as well.
Flags: needinfo?(jmathies)
Hey David, please investigate. Looks like this might be flash sandbox related.
Flags: needinfo?(jmathies) → needinfo?(davidp99)
(Assignee)

Updated

a year ago
Assignee: nobody → davidp99
(Assignee)

Comment 3

a year ago
The issue is indeed the PrintDlg attempt to parent to the browser window.  The sandbox won't allow that.  Options:

* Brokering PrintDlg in another process.  I've done this locally using the brokering mechanism I'm putting together for bug 1382251.  The principle issue here is that PrintDlg takes a callback that obviously can't be marshaled.  I've run things without the callback and it seems to work ok.  The callback is probably just used for styling.  We could ask Adobe to confirm.  We can lose styling if needed.
* "Unparenting" the window by hijacking PrintDlg with the DLL interceptor and zeroing out the HWND Adobe gave us.  This actually also works.  We'll go this way short-term.  Obviously, the print dialog can end up behind the browser window but that's manageable.  This solution will fall down as we harden the NPAPI sandbox -- for example, it will never make it past Alternative Desktop.

With the code in bug 1382251, both solutions are pretty simple.  They result in the ability to print.  However, a separate bug should be made for print-to-file if it isn't remedied by this bug, which it looks like it wont be.  I'm not sure what API it uses for file access (apparently not GetSaveFileName, which would work even with file access sandboxed).

I'll hack up a version (sans the intercepting code in bug 1382251) soon.  The plan is to watch it for regressions for a week or so and then uplift.
Flags: needinfo?(davidp99)
(Assignee)

Updated

a year ago
Blocks: 1395321
(Assignee)

Comment 4

a year ago
Created attachment 8902892 [details] [diff] [review]
1. Adding PrintDlgW to x64 DLL Interceptor Test

This is the test for the DLL interceptor.  No new opcodes were needed but I'll feel better about this when we've been in nightly for a bit without another version of the DLL inspiring crashes (not that this has happened before...).  The next patch has the functionality.

The architecture is limited by the quirks, which are set in 64-bit.

This patch leaves an issue with Print to File.  I'm not clear on why that is but I have confirmed that Print to file uses GetSaveFileNameW, which normally does permit file writing.  I've filed that as bug 1395321.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9e763c81a51de0b7c8d091ff699932484968e21
Attachment #8902892 - Flags: review?(jmathies)
(Assignee)

Comment 5

a year ago
Created attachment 8902895 [details] [diff] [review]
2. Remove PrintDlg Window Parentage in Sandboxed NPAPI Process
Attachment #8902895 - Flags: review?(jmathies)

Updated

a year ago
Attachment #8902892 - Flags: review?(jmathies) → review+

Updated

a year ago
Attachment #8902895 - Flags: review?(jmathies) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 6

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c962b8c853
Part 1: Add PrintDlgW to x64 DLL Interceptor Test. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7fe0c5020c
Part 2: Remove PrintDlg Window Parentage in NPAPI Process. r=jimm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56c962b8c853
https://hg.mozilla.org/mozilla-central/rev/ad7fe0c5020c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you're comfortable doing so.
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(davidp99)
Flags: in-testsuite+
(Assignee)

Comment 9

a year ago
Comment on attachment 8902892 [details] [diff] [review]
1. Adding PrintDlgW to x64 DLL Interceptor Test

Approval Request Comment
[Feature/Bug causing the regression]:
Turning on NPAPI sandbox

[User impact if declined]:
Printing from plugins is completely broken.

[Is this code covered by automated tests?]:
The DLLInterceptor hook of PringDlgW is tested in Part 1.  The actual dialog window and printing are not tested.

[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?]:
The change is very unlikely to lead to a crash or any inconsistency beyond the inability to print, which is already this bug.
DLL Interceptor failures are always a possibility but they should lead to doing nothing.  Also, the DLL interceptor portion is covered by tests.

[String changes made/needed]:
N/A
Flags: needinfo?(davidp99)
Attachment #8902892 - Flags: approval-mozilla-beta?
(Assignee)

Comment 10

a year ago
Comment on attachment 8902895 [details] [diff] [review]
2. Remove PrintDlg Window Parentage in Sandboxed NPAPI Process

Approval Request Comment
[Feature/Bug causing the regression]:
Turning on NPAPI sandbox

[User impact if declined]:
Printing from plugins is completely broken.

[Is this code covered by automated tests?]:
The DLLInterceptor hook of PringDlgW is tested in Part 1.  The actual dialog window and printing are not tested.

[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?]:
The change is very unlikely to lead to a crash or any inconsistency beyond the inability to print, which is already this bug.
DLL Interceptor failures are always a possibility but they should lead to doing nothing.  Also, the DLL interceptor portion is covered by tests.

[String changes made/needed]:
N/A
Attachment #8902895 - Flags: approval-mozilla-beta?
Comment on attachment 8902892 [details] [diff] [review]
1. Adding PrintDlgW to x64 DLL Interceptor Test

Fix a regression and include a test. Beta56+.
Attachment #8902892 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8902895 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.