Closed Bug 1388903 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

51 Branch
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: jeclark, Assigned: handyman)

References

Details

(Keywords: 64bit, flashplayer, regression)

Attachments

(3 files)

Attached file 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
Product: Firefox → Core
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: nobody → davidp99
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)
Blocks: 1395321
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)
Attachment #8902892 - Flags: review?(jmathies) → review+
Attachment #8902895 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
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
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(davidp99)
Flags: in-testsuite+
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?
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+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.