Bob Owen (:bobowen) Assignee
	Description • 9 years ago

When printing with e10s enabled, two splwow64.exe (Print driver host for 32bit applications) processes are spawned.

One as a child of the broker/chrome process and one as a child of the content process.

If you try to print using the Microsoft XPS Document Writer, the splwow64 that is the child of the content process attempts to write the resulting file.
As this is a child of the content process, it is also low integrity and so can't write the file anywhere apart from low integrity directories (e.g. %USERPROFILE%\AppData\LocalLow\).

Tried using PDFCreator, which is a print driver that prints to PDF, and this worked.
This seems to be because the PDFCreator process is created by the Print Spooler service and this is what saves the file.

The "Print to file" option seems to be failing for a different reason, so I might break that out into a different bug.

	psyvenrix
	Comment 4 • 9 years ago

I am not a dev but i have been able to reproduce this. Mainly as i was hit with this and spent some time trying to make XPS printing work.
My hack work around was to nerf the MIC level on C:\users\%USERNAME%\appdata\local\temp to allow low MIC processes to write to that folder. I used icacls.exe for the record. Not secure but it worked, was acceptable as it meant i could print off an important (to me) invoice with out having to close nightly and mess about with other ugly ideas.

I can only suggest a fix of getting the main nightly.exe process when it spawns plugin-container.exe to change the two environment varibles of %TMP% and %TEMP% to point to C:\users\%USERNAME%\appdata\LocalLow\Temp\, this folder having the FS MIC permission to allow low integrity processes to write/trash files in that folder as they please.

HTH

	Bob Owen (:bobowen) Assignee
	Comment 5 • 9 years ago

(In reply to psyvenrix from comment #4)
> I am not a dev but i have been able to reproduce this. Mainly as i was hit
> with this and spent some time trying to make XPS printing work.
> My hack work around was to nerf the MIC level on
> C:\users\%USERNAME%\appdata\local\temp to allow low MIC processes to write
> to that folder. I used icacls.exe for the record. Not secure but it worked,
> was acceptable as it meant i could print off an important (to me) invoice
> with out having to close nightly and mess about with other ugly ideas.

Did you reset the integrity level on temp ... probably wise if you haven't.

I'll be looking at a proper fix to this soon, sorry that it has caused you pain.
One alternative, for the moment, is to use "New Non-e10s Window" from the hamburger menu when you want to print to XPS.
(Although, I know it's sometimes not easy to get back to some pages after the fact.)

> I can only suggest a fix of getting the main nightly.exe process when it
> spawns plugin-container.exe to change the two environment varibles of %TMP%
> and %TEMP% to point to C:\users\%USERNAME%\appdata\LocalLow\Temp\, this
> folder having the FS MIC permission to allow low integrity processes to
> write/trash files in that folder as they please.

We used to do something like this, but had some problems, so worked around things slightly differently.
I'll probably put something similar back in for a different reason, but it would still only allow you to print to low integrity directories.
Anyway, as I say, I'll be working on a proper fix soon.

	leichixian
	Comment 6 • 9 years ago

The bug also happens if you try to print with Windows 10's "Microsoft Print to PDF". The reason may be same as "print to XPS"

	Bob Owen (:bobowen) Assignee
	Comment 7 • 9 years ago

This is still WIP, but generally seems to work OK.
As I'm away for a bit it would be great to get some feedback.

I've very much just dropped the new code where it is most convenient.

We may want to think about putting in some sort of abstractions in order to re-use some of this cross-platform. Although I think it will only be the basic flow that could be cross-platform.
The token to store the print settings could be used easily.

I need to think about how to handle silent printing, because it doesn't call the parent for the settings, we won't get a printing token.

I also need some more error handling in places.

roc - you come top of the reviewer list with Jim, not sure if you are the best person from the graphics side to look at this. Please pass on to someone else if not.
(Hopefully I haven't abused cairo too much in getting this to work. :-) )

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 9 years ago

Wow, really wish I'd found this before assuming it was an issue with my profile and formatting.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 9 years ago

Comment on attachment 8651104 [details] [diff] [review]
WIP: Print from the parent process using EMF.

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

I think we should try taking a cross-platform approach to forwarding the printed pages, using Moz2D's CreateRecordingDrawTarget. Bas, does that sound reasonable?

I suspect that using N shmems simultaneously for N pages is going to be a problem. Some printed documents are very large and are likely to exhaust shmem limits. Also, I think it's quite important to users printing huge documents to be able to print incrementally, i.e. start printing before all pages have been rendered. Maybe we could solve both problems by dispatching one call to PrintingParent per page, so we print incrementally and can release page shmems incrementally? Might need to throttle page production in case PrintingParent is getting behind --- we could make the call to PrintingParent sync, though it would probably be better to have a separate async reply message and make the content process wait explicitly.

	Bob Owen (:bobowen) Assignee
	Comment 10 • 9 years ago

Thanks roc.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8651104 [details] [diff] [review]
> WIP: Print from the parent process using EMF.
> 
> Review of attachment 8651104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should try taking a cross-platform approach to forwarding the
> printed pages, using Moz2D's CreateRecordingDrawTarget. Bas, does that sound
> reasonable?

That was one of the original plans.
I should have mentioned (sorry), I spoke to Jeff at Whistler and he steered me away from that towards EMF.
I agree though, if that could be made to work it would be good and at least the content and transport parts could be cross-platform.

> I suspect that using N shmems simultaneously for N pages is going to be a
> problem. Some printed documents are very large and are likely to exhaust
> shmem limits. Also, I think it's quite important to users printing huge
> documents to be able to print incrementally, i.e. start printing before all
> pages have been rendered. Maybe we could solve both problems by dispatching
> one call to PrintingParent per page, so we print incrementally and can
> release page shmems incrementally? Might need to throttle page production in
> case PrintingParent is getting behind --- we could make the call to
> PrintingParent sync, though it would probably be better to have a separate
> async reply message and make the content process wait explicitly.

Yes, I was thinking about something like that for a follow-up, but maybe we should have that from the start.
I'll have to look into how I hold the state on both sides.
I suppose I could just hold it with the print settings on the parent, things would be trickier on the child.

	Bas Schouten (:bas.schouten)
	Comment 11 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8651104 [details] [diff] [review]
> WIP: Print from the parent process using EMF.
> 
> Review of attachment 8651104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should try taking a cross-platform approach to forwarding the
> printed pages, using Moz2D's CreateRecordingDrawTarget. Bas, does that sound
> reasonable?

In theory Moz2D's Recording works both to and from a stream so theoretically this should be perfectly possible.

	Bob Owen (:bobowen) Assignee
	Comment 12 • 9 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > Comment on attachment 8651104 [details] [diff] [review]
> > WIP: Print from the parent process using EMF.
> > 
> > Review of attachment 8651104 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think we should try taking a cross-platform approach to forwarding the
> > printed pages, using Moz2D's CreateRecordingDrawTarget. Bas, does that sound
> > reasonable?
> 
> In theory Moz2D's Recording works both to and from a stream so theoretically
> this should be perfectly possible.

Hi Bas ... so I think I see how I could use CreateRecordingDrawTarget to record things to a stream.
Although it appears that you have to give it a real DrawTarget as well, which also gets written to, which I'm not sure we'd want.

What I can't find at the moment is how I would then use that stream (or the data from it) to replay the recording to a print surface or device in the parent process.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 13 • 9 years ago

FYI, the “print to PDF” features on both OS X and Linux open and write directly to files from the content process, so we're going to need to deal with this in a cross-platform way eventually.

	Bas Schouten (:bas.schouten)
	Comment 14 • 9 years ago

(In reply to Bob Owen (:bobowen) from comment #12)
> (In reply to Bas Schouten (:bas.schouten) from comment #11)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > > Comment on attachment 8651104 [details] [diff] [review]
> > > WIP: Print from the parent process using EMF.
> > > 
> > > Review of attachment 8651104 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I think we should try taking a cross-platform approach to forwarding the
> > > printed pages, using Moz2D's CreateRecordingDrawTarget. Bas, does that sound
> > > reasonable?
> > 
> > In theory Moz2D's Recording works both to and from a stream so theoretically
> > this should be perfectly possible.
> 
> Hi Bas ... so I think I see how I could use CreateRecordingDrawTarget to
> record things to a stream.
> Although it appears that you have to give it a real DrawTarget as well,
> which also gets written to, which I'm not sure we'd want.

This is sort of required in order to 'get data out' that some drawing functions may expect I believe, just any offscreen DrawTarget of the right size will do.

> What I can't find at the moment is how I would then use that stream (or the
> data from it) to replay the recording to a print surface or device in the
> parent process.

Note how recorded events can be replayed to a certain translator (after you create them from a stream). You would want to do something like this, and then you'll need to recognize your first DrawTarget creation and create a DrawTarget of the printing type for that. Recorded events then will tell your translator about objects they're creating and asking your translator for the right pointers in the current process.

For the base translator class see: http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/annotate/46e20d96e909/RecordedEvent.h

For a very basic implementation of a translator see: http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/file/46e20d96e909/recordbench/RawTranslator.cpp

	Jim Mathies [:jimm]
	Comment 15 • 9 years ago

What's the level of effort here, do we need to get the gfx team involved to get this recording api working? If so the emf approach might be a good work around while they work on resourcing this. (At mozilla though temporary work arounds have a bad habit of becoming permanent features, especially on Windows. So we should avoid that if we can.)

	Bob Owen (:bobowen) Assignee
	Comment 16 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #15)
> What's the level of effort here, do we need to get the gfx team involved to
> get this recording api working? If so the emf approach might be a good work
> around while they work on resourcing this. (At mozilla though temporary work
> arounds have a bad habit of becoming permanent features, especially on
> Windows. So we should avoid that if we can.)

I chatted to Bas on IRC about this on Friday and he doesn't think it should be a big job.

I'm working on getting the per page remote printing working (as roc also suggested), because we want that either way.
I pretty much got a rough version of this working on Friday, but had a problem with the lifetime of the nsPagePrintTimer, so I'm just working on a bit of a re-design.

Once, I'm done with that I'm going to try and work out the Moz2D recording API stuff, as we now all seem agreed that that is the best long term approach.
I think short term work arounds becoming permanent is a universal thing in software development. :-)
Bas said he'd help me with the details, so hopefully I can work it out.

Having said all that, if it's going to take me too long, I'll try and persuade people to fall back to the EMF solution on Windows for the moment, as I'm keen to get the blockers for the sandbox moving to Aurora removed.
I'd then look to come back to the printing again, once those were sorted.

	Bob Owen (:bobowen) Assignee
	Comment 17 • 9 years ago

Another WIP patch that now prints a page at a time over IPC, so that we don't use loads of shared memory on large prints.

This replaces the UUID with a new PRemotePrintJob protocol to hold the state and functionality on both sides. The protocol is initiated from the parent, so performs the same job as the UUID.

This still needs work, but I'm going to see if I can get my head around the Moz2D recording API first, as that will make some of that work unnecessary.

	Bob Owen (:bobowen) Assignee
	Comment 19 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6569c3ebee6

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f3eca3aacdb

	Bob Owen (:bobowen) Assignee
	Comment 20 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fbbbc0056e3

	Bob Owen (:bobowen) Assignee
	Comment 21 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17f44861995f

	Bob Owen (:bobowen) Assignee
	Comment 22 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e5e7ca17c6d

	Bob Owen (:bobowen) Assignee
	Comment 23 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22cc590c42ac

	Bob Owen (:bobowen) Assignee
	Comment 24 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b832b8bf7aa2

	Bob Owen (:bobowen) Assignee
	Comment 25 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e4e6244a84

	Bob Owen (:bobowen) Assignee
	Comment 26 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6ee0485c872

	Bob Owen (:bobowen) Assignee
	Comment 27 • 9 years ago

Looking green, although this probably doesn't mean much for printing.
It at least looks like I haven't broken anything else.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=272ad514515c

Here's a run with e10s turned on for mochitests, there will be failures here anyway:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c5d8f7012a

	Bob Owen (:bobowen) Assignee
	Comment 28 • 9 years ago

Apologies in advance for the number of review requests coming up.

I've tried to split it up as best I can.
Most of the patches towards the end are to address issues I found in testing, where they made sense as a separate patch and weren't just a small amendment to a previous one.

	Bob Owen (:bobowen) Assignee
	Comment 29 • 9 years ago

Bug 1156742 Part 1: Change Moz2D recording, so that it can be used in isolation. r?bas

These are mainly changes to make sure we have recorded relevant dependencies to each draw operation.
Where we can't record them on the fly like this, it makes sure the object has originated from our DrawTarget.

	Bob Owen (:bobowen) Assignee
	Comment 30 • 9 years ago

Bug 1156742 Part 2: Make gfx thebes/gl/2d work with UNICODE defined. r?bas, r?glandium

	Bob Owen (:bobowen) Assignee
	Comment 31 • 9 years ago

Bug 1156742 Part 3: Add support for FontType::CAIRO to CreateScaledFontForTrueTypeData on Windows. r?bas

Parts of this change and related code get moved around in Part 24.

	Bob Owen (:bobowen) Assignee
	Comment 32 • 9 years ago

Bug 1156742 Part 4: Add an in memory DrawEventRecorder. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 33 • 9 years ago

Bug 1156742 Part 5: Add a skeleton RemotePrintJob. r?mconley

The methods will get filled out by later patches, this sets up the IPDL and lifetime management.

	Bob Owen (:bobowen) Assignee
	Comment 34 • 9 years ago

Bug 1156742 Part 6: Add RemotePrintJob to PrintSession and PrintData. r?roc, r?mconley

Someone knew that nsIPrintSession would come in handy one day.

	Bob Owen (:bobowen) Assignee
	Comment 35 • 9 years ago

Bug 1156742 Part 7: Refactor nsDeviceContext.cpp to use printing surface for size and nsIDeviceContextSpec for DPI and scale. r?roc

These changes are to make using an off screen surface behind our DrawTarget in the child easier.
It still creates the real printing surface for some of the calculations,
removing this will be required for future tightening of the sandbox.

	Bob Owen (:bobowen) Assignee
	Comment 36 • 9 years ago

Bug 1156742 Part 8: Change gfxWindowsSurface, so that a non-printing surface can be used when recording a print. r?roc

	Bob Owen (:bobowen) Assignee
	Comment 37 • 9 years ago

Bug 1156742 Part 9: Add a new nsIDeviceContextSpec for proxied printing. r?roc

This also changes aPrintToFileName parameter for BeginDocument to an nsAString& from char16_t*.
Having a char16_t* caused a pain with VS2105 where wchar_t != char16_t (as on VS2103), after it had been sent over IPDL.
This could have been worked around with casting, but this seemed like the tidier solution.

	Bob Owen (:bobowen) Assignee
	Comment 38 • 9 years ago

Bug 1156742 Part 10: Allow RemotePrintJob to influence nsPagePrintTimer. r?roc

Adds a new timer for the RemotePrintJob to notify the nsPagePrintTimer when the last page has finished printing in the parent.
Changed the page delay timer to reset the watch dog count on every page to prevent timeouts due to remote printing.

	Bob Owen (:bobowen) Assignee
	Comment 39 • 9 years ago

Bug 1156742 Part 11: Allow RemotePrintJobChild to abort the print. r?roc

This is so the RemotePrintJobParent can abort the printing in the child when something goes wrong.

	Bob Owen (:bobowen) Assignee
	Comment 40 • 9 years ago

Bug 1156742 Part 12: Record CreateSimilarDrawTarget separately for Moz2D. r?bas

This is so we can create the real print DrawTarget from our nsDeviceContext at the beginning of each page.
Default behaviour for other Translators is still to always use CreateSimilarDrawTarget.

	Bob Owen (:bobowen) Assignee
	Comment 41 • 9 years ago

Bug 1156742 Part 13: Create a Moz2D PrintTranslator. r?bas

A better solution to the ScaledFonts issue comes in Part 24.

	Bob Owen (:bobowen) Assignee
	Comment 42 • 9 years ago

Bug 1156742 Part 14: Complete RemotePrintJob using PrintTranslator. r?mconley

	Bob Owen (:bobowen) Assignee
	Comment 43 • 9 years ago

Bug 1156742 Part 15: Add pref for turning on printing via the parent process. r?mconley

	Bob Owen (:bobowen) Assignee
	Comment 44 • 9 years ago

Bug 1156742 Part 16: Add recording and forwarding of Matrix attribute set for Moz2D recording. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 45 • 9 years ago

Bug 1156742 Part 17: Add virtual destructor to RecorededEvent and fix subsequent crash with DWrite playback fonts. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 46 • 9 years ago

Bug 1156742 Part 18: Fix the way we hold custom font data so that they can be recorded with Moz2D. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 47 • 9 years ago

Bug 1156742 Part 19: Implement GetFontFileData for ScaledFontWin. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 48 • 9 years ago

Bug 1156742 Part 20: Move Moz2D PreferenceAccess into its own header. r?bas

This is so we can add a new preference in Part 21, which has nothing to do with logging.

	Bob Owen (:bobowen) Assignee
	Comment 49 • 9 years ago

Bug 1156742 Part 21: Add and use Moz2D preference access to layers.acceleration.disabled. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 50 • 9 years ago

Bug 1156742 Part 22: Change ScaledFontDWrite to support creation from TrueType Collection data. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 51 • 9 years ago

Bug 1156742 Part 23: Add null checks to lookups in Moz2D RecordedEvent playback. r?bas

	Bob Owen (:bobowen) Assignee
	Comment 52 • 9 years ago

Bug 1156742 Part 24: Add new Recorded event to record font data. r?bas

We create and destroy ScaledFonts for every piece of text we write.
That causes a huge amount of duplicated data within the recording.
This splits out the recording of the font data itself from the ScaledFont.
The key generated to determine uniqueness could probably be fairly easily faked, but for our purposes that doesn't matter.

	Bob Owen (:bobowen) Assignee
	Comment 53 • 9 years ago

Bug 1156742 Part 25: Flip the big switch and wait for the lightning. r?mconley

	Bob Owen (:bobowen) Assignee
	Comment 54 • 9 years ago

Comment on attachment 8688993 [details]
MozReview Request: Bug 1156742 Part 15: Add pref for turning on printing via the parent process. r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25513/diff/1-2/

	Bob Owen (:bobowen) Assignee
	Comment 55 • 9 years ago

Comment on attachment 8688998 [details]
MozReview Request: Bug 1156742 Part 20: Move Moz2D PreferenceAccess into its own header. r?bas

Bug 1156742 Part 20: Move Moz2D PreferenceAccess into its own header. r?bas

This is so we can add a new preference in Part 21, which has nothing to do with logging.

	Bob Owen (:bobowen) Assignee
	Comment 56 • 9 years ago

Comment on attachment 8689003 [details]
MozReview Request: Bug 1156742 Part 25: Flip the big switch and wait for the lightning. r?mconley

Bug 1156742 Part 25: Flip the big switch and wait for the lightning. r?mconley

	Bob Owen (:bobowen) Assignee
	Comment 57 • 9 years ago

Hmm ... looks like I broke Review Board.

I think I'm going to have to upload them all again manually.
Sorry for the spam.

	Bob Owen (:bobowen) Assignee
	Comment 58 • 9 years ago

These are mainly changes to make sure we have recorded relevant dependencies to each draw operation.
Where we can't record them on the fly like this, it makes sure the object has originated from our DrawTarget.

	Bob Owen (:bobowen) Assignee
	Comment 60 • 9 years ago

Parts of this change and related code get moved around in Part 24.

	Bob Owen (:bobowen) Assignee
	Comment 62 • 9 years ago

The methods will get filled out by later patches, this sets up the IPDL and lifetime management.

	Bob Owen (:bobowen) Assignee
	Comment 63 • 9 years ago

Someone knew that nsIPrintSession would come in handy one day.

	Bob Owen (:bobowen) Assignee
	Comment 64 • 9 years ago

These changes are to make using an off screen surface behind our DrawTarget in the child easier.
It still creates the real printing surface for some of the calculations,
removing this will be required for future tightening of the sandbox.

	Bob Owen (:bobowen) Assignee
	Comment 66 • 9 years ago

This also changes aPrintToFileName parameter for BeginDocument to an nsAString& from char16_t*.
Having a char16_t* caused a pain with VS2105 where wchar_t != char16_t (as on VS2103), after it had been sent over IPDL.
This could have been worked around with casting, but this seemed like the tidier solution.

	Bob Owen (:bobowen) Assignee
	Comment 67 • 9 years ago

Adds a new timer for the RemotePrintJob to notify the nsPagePrintTimer when the previous page has finished printing in the parent.
Changed the page delay timer to reset the watch dog count on every page to prevent timeouts due to remote printing.

	Bob Owen (:bobowen) Assignee
	Comment 68 • 9 years ago

This is so the RemotePrintJobParent can abort the printing in the child when something goes wrong.

	Bob Owen (:bobowen) Assignee
	Comment 69 • 9 years ago

This is so we can create the real print DrawTarget from our nsDeviceContext at the beginning of each page.
Default behaviour for other Translators is still to always use CreateSimilarDrawTarget.

	Bob Owen (:bobowen) Assignee
	Comment 70 • 9 years ago

A better solution to the ScaledFonts issue comes in Part 24.

	Bob Owen (:bobowen) Assignee
	Comment 72 • 9 years ago

It looks like 15-25 can still be used for review.
The others said they were discarded, so I've uploaded manually.
When I tried to re-push to review board it still said they were discarded. :-(

	Bob Owen (:bobowen) Assignee
	Comment 73 • 9 years ago

Even the remaining ones are pretty broken on review board, so I'm going to upload manually.

	Bob Owen (:bobowen) Assignee
	Comment 79 • 9 years ago

This is so we can add a new preference in Part 21, which has nothing to do with logging.

	Bob Owen (:bobowen) Assignee
	Comment 83 • 9 years ago

We create and destroy ScaledFonts for every piece of text we write.
That causes a huge amount of duplicated data within the recording.
This splits out the recording of the font data itself from the ScaledFont.
The key generated to determine uniqueness could probably be fairly easily faked, but for our purposes that doesn't matter.

	Mike Conley (:mconley) (:⚙️)
	Comment 85 • 9 years ago

Comment on attachment 8689018 [details] [diff] [review]
Part 5: Add a skeleton RemotePrintJob.

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

LGTM, thanks!

::: embedding/components/printingui/ipc/PrintingParent.h
@@ +49,5 @@
>      AllocPPrintSettingsDialogParent();
>  
>      virtual bool
>      DeallocPPrintSettingsDialogParent(PPrintSettingsDialogParent* aActor);
> +    

Nit - whitespace

	Mike Conley (:mconley) (:⚙️)
	Comment 86 • 9 years ago

Comment on attachment 8689019 [details] [diff] [review]
Part 6: Add RemotePrintJob to PrintSession and PrintData.

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

::: embedding/components/printingui/ipc/PPrintSettingsDialog.ipdl
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  include PPrintingTypes;
>  include protocol PPrinting;
> +include protocol PRemotePrintJob;

Does the IPDL parser require this to be in here? We already include it in PPrintingTypes.ipdlh, and I don't see it being referred to in here.

::: embedding/components/printingui/ipc/PrintingParent.cpp
@@ +23,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::dom;
>  
> +typedef mozilla::layout::RemotePrintJobParent RemotePrintJobParent;

I would consider myself still reasonably newbish to C++, so I have to ask - does this give us some kind of advantage over doing something like, using

`namespace mozilla::layout`

?

@@ +106,5 @@
>    // And send it back.
>    rv = po->SerializeToPrintData(settings, nullptr, aResult);
> +
> +  PRemotePrintJobParent* remotePrintJob = new RemotePrintJobParent(settings);
> +  aResult->remotePrintJobParent() = SendPRemotePrintJobConstructor(remotePrintJob);

So we'll construct this thing if we show the dialog.

I found out in all of this that we have a hidden pref, print.always_print_silent, that will skip the dialog, and just start printing straight away (which is about as dangerous as it sounds).

If we want to support that, we'd have to find an opportunity to construct the PRemotePrintJob in the child, and send it up to the parent somehow.

If we don't want to support that (and we might not - it's a hidden pref, and it sounds dangerous given that web content could just call window.print() on all sorts of things), then we should probably explicitly tear out the pref support in a follow-up.

::: layout/printing/ipc/RemotePrintJobChild.cpp
@@ +31,5 @@
>  }
>  void
>  RemotePrintJobChild::ProcessPage(Shmem& aStoredPage)
>  {
> +  Unused << SendProcessPage(aStoredPage);

TIL about bug 1219392.

	Mike Conley (:mconley) (:⚙️)
	Comment 87 • 9 years ago

Comment on attachment 8689032 [details] [diff] [review]
Part 14: Complete RemotePrintJob using PrintTranslator.

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

::: layout/printing/ipc/RemotePrintJobParent.cpp
@@ +79,5 @@
> +  {
> +    // Make sure the memory gets deallocated, this is scoped so that we always
> +    // deallocate before we tell the child we've finished via SendPageProcessed.
> +    AutoShmemDeallocator autoShmemDeallocator{aStoredPage, this};
> +    Unused << autoShmemDeallocator;

What does this line do?

@@ +107,5 @@
> +
> +    mPrintTranslator->ClearSavedFonts();
> +  }
> +
> +  Unused << SendPageProcessed();

This whole method is really straight-forward and easy to understand. Thanks for that. :)

	Mike Conley (:mconley) (:⚙️)
	Comment 88 • 9 years ago

Comment on attachment 8689055 [details] [diff] [review]
Part 15: Add pref for turning on printing via the parent process.

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

::: browser/app/profile/firefox.js
@@ +1149,5 @@
>  pref("browser.tabs.remote.autostart", false);
>  pref("browser.tabs.remote.desktopbehavior", true);
>  
> +// Print via the parent process.
> +pref("print.print-via-parent", false);

This might make more sense in modules/libpref/init/all.js with the rest of the printing prefs.

	Mike Conley (:mconley) (:⚙️)
	Comment 89 • 9 years ago

Comment on attachment 8689073 [details] [diff] [review]
Part 25: Flip the big switch and wait for the lightning.

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

::: browser/app/profile/firefox.js
@@ +1149,5 @@
>  pref("browser.tabs.remote.autostart", false);
>  pref("browser.tabs.remote.desktopbehavior", true);
>  
>  // Print via the parent process.
> +#if defined(XP_WIN)

Assuming this gets moved into modules/libpref/init/all.js, this is probably fine.

So we're only doing this for Windows - out of curiosity, what is preventing us from supporting the other platforms right now?

	Bob Owen (:bobowen) Assignee
	Comment 90 • 9 years ago

Thanks for the reviews!

Really sorry to do this to you, but I spotted a problem with InitializePrint in the parent.

Can't believe I hadn't realised before, but the BeginDocument call, which causes StartDoc to be called on the Windows device, can pop up a dialog to choose the file.
We need to wait for that and the result.

So, I've had to rework this patch, although I'm generally happier with it anyway.
It's meant I could get rid of that kludge where I was holding the nsresult and I've improved the way I'm ensuring the Shmem is de-allocated I think.


Replies to comments for this and other patches:

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #85)

> >      DeallocPPrintSettingsDialogParent(PPrintSettingsDialogParent* aActor);
> > +    
> 
> Nit - whitespace

Weird this wasn't in my local file, I must have accidentally slipped it into the exported patch file.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #86)

> ::: embedding/components/printingui/ipc/PPrintSettingsDialog.ipdl
> @@ +4,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  include PPrintingTypes;
> >  include protocol PPrinting;
> > +include protocol PRemotePrintJob;
> 
> Does the IPDL parser require this to be in here? We already include it in
> PPrintingTypes.ipdlh, and I don't see it being referred to in here.

I get compile errors without this, I was surprised as well.
 
> > +typedef mozilla::layout::RemotePrintJobParent RemotePrintJobParent;
> 
> I would consider myself still reasonably newbish to C++, so I have to ask -
> does this give us some kind of advantage over doing something like, using
> 
> `namespace mozilla::layout`

That would mean anything in mozilla::layout wouldn't need an explicit namespace and so would be more likely to cause conflicts.

Fairly new to C++ myself, while I used it occasionally at a previous job it wasn't much and only C really.
So, I've learnt most of my C++ working on Gecko, so don't take anything I say as Gospel. :-)

As I understand it: |typedef foo::bar bar;| is the same as |using bar = foo::bar;|, although the second helps for some reason when used with Templates.

> @@ +106,5 @@
> >    // And send it back.
> >    rv = po->SerializeToPrintData(settings, nullptr, aResult);
> > +
> > +  PRemotePrintJobParent* remotePrintJob = new RemotePrintJobParent(settings);
> > +  aResult->remotePrintJobParent() = SendPRemotePrintJobConstructor(remotePrintJob);
> 
> So we'll construct this thing if we show the dialog.
> 
> I found out in all of this that we have a hidden pref,
> print.always_print_silent, that will skip the dialog, and just start
> printing straight away (which is about as dangerous as it sounds).
> 
> If we want to support that, we'd have to find an opportunity to construct
> the PRemotePrintJob in the child, and send it up to the parent somehow.
> 
> If we don't want to support that (and we might not - it's a hidden pref, and
> it sounds dangerous given that web content could just call window.print() on
> all sorts of things), then we should probably explicitly tear out the pref
> support in a follow-up.

Right, that will stop working, I need to fix that for the Print Edit add-on.
I think the best solution is to add a PrintWithSettings or something, to the parent so an addon can call it in the main process.
Currently, Print Edit is sending a message from the parent and then constructing the settings in the child (with print silently), so giving addons something in the parent should work.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #87)

> > +    Unused << autoShmemDeallocator;
> 
> What does this line do?

Static analysis was complaining that autoShmemDeallocator wasn't used, but I've got rid of this anyway in the new patch.
 
> This whole method is really straight-forward and easy to understand. Thanks
> for that. :)

Glad to hear it ... hopefully the new patch is as well. :-)

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #88)

> > +// Print via the parent process.
> > +pref("print.print-via-parent", false);
> 
> This might make more sense in modules/libpref/init/all.js with the rest of
> the printing prefs.

Yes, I was thinking of putting it there, but wasn't sure if it applied to all platforms.
I should maybe ask Jed about this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 91 • 9 years ago

Comment on attachment 8689019 [details] [diff] [review]
Part 6: Add RemotePrintJob to PrintSession and PrintData.

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

::: embedding/components/printingui/ipc/PrintingParent.cpp
@@ +23,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::dom;
>  
> +typedef mozilla::layout::RemotePrintJobParent RemotePrintJobParent;

Yeah I think "using namespace mozilla:;layout" would be better here

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 92 • 9 years ago

Comment on attachment 8689026 [details] [diff] [review]
Part 9: Add a new nsIDeviceContextSpec for proxied printing.

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

::: gfx/src/nsDeviceContext.cpp
@@ +402,5 @@
>  
> +    RefPtr<DrawEventRecorder> recorder;
> +    nsresult rv = mDeviceContextSpec->GetDrawEventRecorder(getter_AddRefs(recorder));
> +    if (NS_SUCCEEDED(rv) && recorder) {
> +      dt = gfx::Factory::CreateRecordingDrawTarget(recorder, dt);

Confusing. Are we creating a DrawTarget via CreateDrawTargetForSurface and then throwing it away here? Or is CreateDrawTargetForSurface guaranteed to fail when there's a recorder. Seems to me it would be better to call GetDrawEventRecorder first and only call CreateDrawTargetForsurface if that fails.

	Bob Owen (:bobowen) Assignee
	Comment 93 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #92)
> Comment on attachment 8689026 [details] [diff] [review]
> Part 9: Add a new nsIDeviceContextSpec for proxied printing.
> 
> Review of attachment 8689026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/nsDeviceContext.cpp
> @@ +402,5 @@
> >  
> > +    RefPtr<DrawEventRecorder> recorder;
> > +    nsresult rv = mDeviceContextSpec->GetDrawEventRecorder(getter_AddRefs(recorder));
> > +    if (NS_SUCCEEDED(rv) && recorder) {
> > +      dt = gfx::Factory::CreateRecordingDrawTarget(recorder, dt);
> 
> Confusing. Are we creating a DrawTarget via CreateDrawTargetForSurface and
> then throwing it away here? Or is CreateDrawTargetForSurface guaranteed to
> fail when there's a recorder. Seems to me it would be better to call
> GetDrawEventRecorder first and only call CreateDrawTargetForsurface if that
> fails.

We call CreateDrawTargetForSurface to get the "real" DrawTarget and then if there is a DrawEventRecorder then we wrap the DrawTarget from CreateDrawTargetForSurface in a DrawTargetRecording, which uses the recorder.
We don't throw it away.
Shall I add a comment to make this clearer?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 94 • 9 years ago

Comment on attachment 8689026 [details] [diff] [review]
Part 9: Add a new nsIDeviceContextSpec for proxied printing.

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

No need for a comment, I was just being daft.

	Mike Conley (:mconley) (:⚙️)
	Comment 95 • 9 years ago

Comment on attachment 8689596 [details] [diff] [review]
Part 14: Complete RemotePrintJob using PrintTranslator.

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

::: layout/printing/ipc/RemotePrintJobChild.cpp
@@ +28,5 @@
> +  // need to spin a nested event loop until initialization completes.
> +  Unused << SendInitializePrint(aDocumentTitle, aPrintToFile, aStartPage,
> +                                aEndPage);
> +  while (!mPrintInitialized) {
> +    Unused << NS_ProcessNextEvent();

:( I hate these things.

::: layout/printing/ipc/RemotePrintJobParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "RemotePrintJobParent.h"
>  
> +#include <istream>

Just noticed this - are we cleared to use istream? I ask because I recently read https://developer.mozilla.org/en-US/docs/Mozilla/C++_Portability_Guide#Don%27t_use_the_C.2B.2B_standard_library_%28including_iostream.2C_locale.2C_and_the_STL%29.

	Bob Owen (:bobowen) Assignee
	Comment 96 • 9 years ago

Thanks Mike.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #95)

> > +    Unused << NS_ProcessNextEvent();
> 
> :( I hate these things.

I'm not keen, but I suppose it shows you've deliberately dropped the return value.
 
> ::: layout/printing/ipc/RemotePrintJobParent.cpp

> > +#include <istream>
> 
> Just noticed this - are we cleared to use istream? I ask because I recently
> read
> https://developer.mozilla.org/en-US/docs/Mozilla/
> C++_Portability_Guide#Don%27t_use_the_C.2B.
> 2B_standard_library_%28including_iostream.2C_locale.2C_and_the_STL%29.

The Moz2D recording stuff needs/uses it, so I guess so.
The Moz2D stuff has been written to only rely on MFBT and I don't think that it has much stream support, so I imagine that's the reason.

	Mike Conley (:mconley) (:⚙️)
	Comment 97 • 9 years ago

(In reply to Bob Owen (:bobowen) from comment #96)
> Thanks Mike.
> 
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #95)
> 
> > > +    Unused << NS_ProcessNextEvent();
> > 
> > :( I hate these things.
> 
> I'm not keen, but I suppose it shows you've deliberately dropped the return
> value.
>  

To be clear, I meant the nested event loop. :)

> > ::: layout/printing/ipc/RemotePrintJobParent.cpp
> 
> > > +#include <istream>
> > 
> > Just noticed this - are we cleared to use istream? I ask because I recently
> > read
> > https://developer.mozilla.org/en-US/docs/Mozilla/
> > C++_Portability_Guide#Don%27t_use_the_C.2B.
> > 2B_standard_library_%28including_iostream.2C_locale.2C_and_the_STL%29.
> 
> The Moz2D recording stuff needs/uses it, so I guess so.
> The Moz2D stuff has been written to only rely on MFBT and I don't think that
> it has much stream support, so I imagine that's the reason.

Alrighty, sounds good.

	Bob Owen (:bobowen) Assignee
	Comment 98 • 9 years ago

Thanks for the reviews roc.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)

> > +typedef mozilla::layout::RemotePrintJobParent RemotePrintJobParent;
> 
> Yeah I think "using namespace mozilla:;layout" would be better here

Changed locally.
(You see Mike, I told you not to listen to me.)

	Bas Schouten (:bas.schouten)
	Comment 99 • 9 years ago

Comment on attachment 8689014 [details] [diff] [review]
Part 1: Change Moz2D recording, so that it can be used in isolation.

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

::: gfx/2d/DrawTargetRecording.cpp
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: set ts=8 sts=2 et sw=2 tw=80:

nit: modeline slipped in

@@ +40,5 @@
> +  if (!aDataSurf) {
> +    gfxWarning() << "Recording failed to record SourceSurface for " << reason;
> +    // Insert a bogus source surface.
> +    int32_t stride = aSurface->GetSize().width * BytesPerPixel(aSurface->GetFormat());
> +    uint8_t *sourceData = new uint8_t[stride * aSurface->GetSize().height]();

Use either STL vector here or an mfbt RAII class. I realize this code was copied but now that we have this we should use it while we're moving it around :-).

@@ +48,5 @@
> +    delete[] sourceData;
> +  }
> +  else {
> +    aRecorder->RecordEvent(
> +      RecordedSourceSurfaceCreation(aSurface, aDataSurf->GetData(), aDataSurf->Stride(),

GetData() and Stride() are deprecated, use the mapping helper class.

::: gfx/2d/DrawTargetRecording.h
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: set ts=8 sts=2 et sw=2 tw=80:

nit: Unwanted modeline slipped in.

	Bas Schouten (:bas.schouten)
	Comment 100 • 9 years ago

Comment on attachment 8689016 [details] [diff] [review]
Part 3: Add support for FontType::CAIRO to CreateScaledFontForTrueTypeData on Windows.

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

Load of vim modelines here that need removing.

::: gfx/2d/BigEndianInts.h
@@ +19,5 @@
> +  {
> +    value = other.value;
> +  }
> +
> +  BigEndianUint16& operator=(const BigEndianUint16& other)

Any reason you're explicitly putting in the assignment operator and constructor?

	Bas Schouten (:bas.schouten)
	Comment 101 • 9 years ago

Comment on attachment 8689029 [details] [diff] [review]
Part 12: Record CreateSimilarDrawTarget separately for Moz2D.

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

::: gfx/2d/DrawTargetRecording.h
@@ +20,5 @@
>    DrawTargetRecording(DrawEventRecorder *aRecorder, DrawTarget *aDT, bool aHasData = false);
> +
> +  /**
> +   * Used for creating a DrawTargetRecording for a CreateSimilarDrawTarget call.
> +   * 

nit: whitespace

	Bas Schouten (:bas.schouten)
	Comment 102 • 9 years ago

Comment on attachment 8689030 [details] [diff] [review]
Part 13: Create a Moz2D PrintTranslator.

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

This all looks good, we need to be very careful though, this all seems like an ideal attach vector for malicious content.

	Bas Schouten (:bas.schouten)
	Comment 103 • 9 years ago

Comment on attachment 8689030 [details] [diff] [review]
Part 13: Create a Moz2D PrintTranslator.

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

This all looks good, we need to be very careful though, this all seems like an ideal attach vector for malicious content.

	Bas Schouten (:bas.schouten)
	Comment 104 • 9 years ago

Comment on attachment 8689061 [details] [diff] [review]
Part 17: Add virtual destructor to RecorededEvent and fix subsequent crash with DWrite playback fonts.

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

::: gfx/2d/ScaledFontDWrite.cpp
@@ +118,5 @@
>    IFACEMETHOD_(ULONG, Release)()
>    {
>      --mRefCnt;
>      if (mRefCnt == 0) {
> +      sFontFileStreams.erase(mFontFileKey);

Is there a reason not to just do this on the destructor?

	Bas Schouten (:bas.schouten)
	Comment 105 • 9 years ago

Comment on attachment 8689063 [details] [diff] [review]
Part 18: Fix the way we hold custom font data so that they can be recorded with Moz2D.

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

::: gfx/thebes/gfxDWriteCommon.cpp
@@ +8,5 @@
> +#include <unordered_map>
> +
> +#include "mozilla/Atomics.h"
> +
> +static mozilla::Atomic<uint64_t> sNextFontFileKey;

I feel a little nauseous.. but I suppose a session getting more fonts than UINT64_MAX is pretty unrealistic ;-).

@@ +56,5 @@
> +  {
> +    NS_PRECONDITION(0 != mRefCnt, "dup release");
> +    --mRefCnt;
> +    if (mRefCnt == 0) {
> +      sFontFileStreams.erase(mFontFileKey);

Also, why not in destructor?

	Bas Schouten (:bas.schouten)
	Comment 106 • 9 years ago

Comment on attachment 8689066 [details] [diff] [review]
Part 20: Move Moz2D PreferenceAccess into its own header.

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

::: gfx/2d/Preferences.cpp
@@ +21,5 @@
> +};
> +
> +static Vector<Int32Pref>& Int32Prefs()
> +{
> +  static Vector<Int32Pref>* sInt32Prefs = new Vector<Int32Pref>();

I dislike never cleaning this up. What's wrong with just a std::vector<Int32Pref> none-pointer?

	Bas Schouten (:bas.schouten)
	Comment 107 • 9 years ago

Comment on attachment 8689067 [details] [diff] [review]
Part 21: Add and use Moz2D preference access to layers.acceleration.disabled.

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

Hrm, I don't think this pref will properly convey the information you need when we're blacklisting..

	Bas Schouten (:bas.schouten)
	Comment 108 • 9 years ago

Comment on attachment 8689070 [details] [diff] [review]
Part 23: Add null checks to lookups in Moz2D RecordedEvent playback.

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

Hrm, under what conditions would this happen? I'm thinking I'd rather just crash in a bunch of these situations and not complicate the code with this. But I may be naive.

	Bob Owen (:bobowen) Assignee
	Comment 109 • 9 years ago

Thanks for all the reviews, sorry there were so many.

A few extra question.

(In reply to Bas Schouten (:bas.schouten) from comment #99)

> > + * vim: set ts=8 sts=2 et sw=2 tw=80:
> 
> nit: modeline slipped in

Why don't we want vim modelines?

> > +    uint8_t *sourceData = new uint8_t[stride * aSurface->GetSize().height]();
> 
> Use either STL vector here or an mfbt RAII class. I realize this code was
> copied but now that we have this we should use it while we're moving it
> around :-).

Ah, yes I'll use UniquePtr like I have elsewhere, I'd forgotten about this one, thanks.

> > +      RecordedSourceSurfaceCreation(aSurface, aDataSurf->GetData(), aDataSurf->Stride(),
> 
> GetData() and Stride() are deprecated, use the mapping helper class.

So I create a DataSourceSurface::ScopedMap and use that?

(In reply to Bas Schouten (:bas.schouten) from comment #100)

> > +  BigEndianUint16& operator=(const BigEndianUint16& other)
> 
> Any reason you're explicitly putting in the assignment operator and
> constructor?

For the simple reason that I didn't realise that the compiler would add them for me, I'll remove them thanks.

(In reply to Bas Schouten (:bas.schouten) from comment #104)

> >      if (mRefCnt == 0) {
> > +      sFontFileStreams.erase(mFontFileKey);
> 
> Is there a reason not to just do this on the destructor?

Because I didn't think of it. :)
I'll move it and the other one, thanks.

(In reply to Bas Schouten (:bas.schouten) from comment #106)

> > +static Vector<Int32Pref>& Int32Prefs()
> > +{
> > +  static Vector<Int32Pref>* sInt32Prefs = new Vector<Int32Pref>();
> 
> I dislike never cleaning this up. What's wrong with just a
> std::vector<Int32Pref> none-pointer?

I think this is the usual pattern because of static de-initialisation ordering.
That shouldn't be an issue for us here, but given that it is static, does it really matter?

(In reply to Bas Schouten (:bas.schouten) from comment #107)
> Comment on attachment 8689067 [details] [diff] [review]
> Part 21: Add and use Moz2D preference access to layers.acceleration.disabled.
> 
> Review of attachment 8689067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hrm, I don't think this pref will properly convey the information you need
> when we're blacklisting..

I'll probably need to keep this code because otherwise I found that when you set the pref, GDI gets used in the child, but DWrite was being used in the parent.
What do I need to do to take account of the blacklisting?

(In reply to Bas Schouten (:bas.schouten) from comment #108)
> Comment on attachment 8689070 [details] [diff] [review]
> Part 23: Add null checks to lookups in Moz2D RecordedEvent playback.
> 
> Review of attachment 8689070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hrm, under what conditions would this happen? I'm thinking I'd rather just
> crash in a bunch of these situations and not complicate the code with this.
> But I may be naive.

At the very least (assuming that they can get the user to initiate a print), a compromised content process would be able to crash the main process.

	Bas Schouten (:bas.schouten)
	Comment 110 • 9 years ago

(In reply to Bob Owen (:bobowen) from comment #109)
> Thanks for all the reviews, sorry there were so many.
> 
> A few extra question.
> 
> (In reply to Bas Schouten (:bas.schouten) from comment #99)
> 
> > > + * vim: set ts=8 sts=2 et sw=2 tw=80:
> > 
> > nit: modeline slipped in
> 
> Why don't we want vim modelines?

As far as I know we have a fixed header style that we're not supposed to deviate from. At least we don't anywhere else in the tree so I feel bad if we'd be the first people do randomly put it in a couple of places :)

> > > +      RecordedSourceSurfaceCreation(aSurface, aDataSurf->GetData(), aDataSurf->Stride(),
> > 
> > GetData() and Stride() are deprecated, use the mapping helper class.
> 
> So I create a DataSourceSurface::ScopedMap and use that?

Exactly!

> (In reply to Bas Schouten (:bas.schouten) from comment #106)
> 
> > > +static Vector<Int32Pref>& Int32Prefs()
> > > +{
> > > +  static Vector<Int32Pref>* sInt32Prefs = new Vector<Int32Pref>();
> > 
> > I dislike never cleaning this up. What's wrong with just a
> > std::vector<Int32Pref> none-pointer?
> 
> I think this is the usual pattern because of static de-initialisation
> ordering.
> That shouldn't be an issue for us here, but given that it is static, does it
> really matter?

If you're using leak detection tools these kind of things cause false positives which I kind of dislike, but it's not a dealbreaker for me.

> (In reply to Bas Schouten (:bas.schouten) from comment #107)
> > Comment on attachment 8689067 [details] [diff] [review]
> > Part 21: Add and use Moz2D preference access to layers.acceleration.disabled.
> > 
> > Review of attachment 8689067 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hrm, I don't think this pref will properly convey the information you need
> > when we're blacklisting..
> 
> I'll probably need to keep this code because otherwise I found that when you
> set the pref, GDI gets used in the child, but DWrite was being used in the
> parent.
> What do I need to do to take account of the blacklisting?

Hrm, I need to think about that a little more.

> (In reply to Bas Schouten (:bas.schouten) from comment #108)
> > Comment on attachment 8689070 [details] [diff] [review]
> > Part 23: Add null checks to lookups in Moz2D RecordedEvent playback.
> > 
> > Review of attachment 8689070 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hrm, under what conditions would this happen? I'm thinking I'd rather just
> > crash in a bunch of these situations and not complicate the code with this.
> > But I may be naive.
> 
> At the very least (assuming that they can get the user to initiate a print),
> a compromised content process would be able to crash the main process.

Right, I'm not -super- concerned about that particular one, once you'd at a state where you can compromise the content process like that a crash would be the least of my worries I suppose :).

	Bob Owen (:bobowen) Assignee
	Comment 111 • 9 years ago

r=bas - from comment 99.

(In reply to Bas Schouten (:bas.schouten) from comment #99)

> > +    uint8_t *sourceData = new uint8_t[stride * aSurface->GetSize().height]();
> 
> Use either STL vector here or an mfbt RAII class. I realize this code was
> copied but now that we have this we should use it while we're moving it
> around :-).

Now uses UniquePtr.
 
> > +      RecordedSourceSurfaceCreation(aSurface, aDataSurf->GetData(), aDataSurf->Stride(),
> 
> GetData() and Stride() are deprecated, use the mapping helper class.

Now uses DataSourceSurface::ScopedMap.

	Bob Owen (:bobowen) Assignee
	Comment 112 • 9 years ago

r=bas - from comment 100.

(In reply to Bas Schouten (:bas.schouten) from comment #100)

> > +  BigEndianUint16& operator=(const BigEndianUint16& other)
> 
> Any reason you're explicitly putting in the assignment operator and
> constructor?

Removed.

	Bob Owen (:bobowen) Assignee
	Comment 113 • 9 years ago

r=bas - from comment 104.

(In reply to Bas Schouten (:bas.schouten) from comment #104)

> Is there a reason not to just do this on the destructor?

Moved.

	Bob Owen (:bobowen) Assignee
	Comment 114 • 9 years ago

r=bas - from comment 105 - again moved to destructor as previous.

	Bob Owen (:bobowen) Assignee
	Comment 115 • 9 years ago

As discussed on IRC.

In theory these lookups should never fail.
The plan is to move them to release asserts when any bugs have been fixed, as failed lookups may be an indication of data being sent from a compromised renderer.

	Bob Owen (:bobowen) Assignee
	Comment 116 • 9 years ago

Bring part 24 into line with changes in part 23.

	Bob Owen (:bobowen) Assignee
	Comment 117 • 9 years ago

This method will allow for acceleration being disabled through prefs and blacklisting.

Bas - I've just updated Part 21 even though this is a tiny change as the previous bugs have already been r+ed.

	Bob Owen (:bobowen) Assignee
	Comment 118 • 9 years ago

Jonathan - as we discussed. Patch 24 is the particular one that I was talking about, but you might want to take a look at 3, 17, 18, 19 and 22 as well, as they are the other main ones dealing with fonts.
Shout if you spot something terrible. :-)

Bas - Jonathan didn't feel he needed to review any of this as well, but just asked me to needinfo him, so he would be aware of the changes.

	Bas Schouten (:bas.schouten)
	Comment 119 • 9 years ago

Comment on attachment 8693644 [details] [diff] [review]
Part 24: Add new Recorded event to record font data.

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

::: gfx/2d/NativeFontResourceDWrite.cpp
@@ +61,5 @@
> +
> +  /**
> +    * Gets the singleton loader instance. Note that when using this font
> +    * loader, the key must be a pointer to an FallibleTArray<uint8_t>. This
> +    * array will be empty when the function returns.

Add a note this isn't threadsafe.

::: gfx/2d/NativeFontResourceWin.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "NativeFontResourceWin.h"

Let's call this one GDI and not 'Win' that's confusing :).

::: gfx/2d/ScaledFontDWrite.cpp
@@ -299,5 @@
> -
> -  uint64_t fontFileKey = sNextFontFileKey++;
> -  sFontFileStreams[fontFileKey] =
> -    new DWriteFontFileStream(aData, aSize, fontFileKey);
> -  RefPtr<IDWriteFontFileStream> ffsRef = sFontFileStreams[fontFileKey];

nit: I don't like this pattern, the object briefly exists here with a 0 refcount, can you new it to the RefPtr and assign that to the map?

	Bob Owen (:bobowen) Assignee
	Comment 120 • 9 years ago

Thanks for the reviews and all you help with this, Bas.

Changes made locally, here's a final try push:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=1bc4837503c5

(In reply to Bas Schouten (:bas.schouten) from comment #119)
> Comment on attachment 8693644 [details] [diff] [review]
> Part 24: Add new Recorded event to record font data.
> 
> Review of attachment 8693644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/NativeFontResourceDWrite.cpp
> @@ +61,5 @@
> > +
> > +  /**
> > +    * Gets the singleton loader instance. Note that when using this font
> > +    * loader, the key must be a pointer to an FallibleTArray<uint8_t>. This
> > +    * array will be empty when the function returns.
> 
> Add a note this isn't threadsafe.

Added and fixed the comments to reflect the new key.

> ::: gfx/2d/NativeFontResourceWin.cpp
> @@ +3,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "NativeFontResourceWin.h"
> 
> Let's call this one GDI and not 'Win' that's confusing :).

Done, I was keeping it consistent with other similar GDI files, but I agree Win is confusing, so maybe the others should change to match this.
I'll file a follow-up.

> ::: gfx/2d/ScaledFontDWrite.cpp
> @@ -299,5 @@
> > -
> > -  uint64_t fontFileKey = sNextFontFileKey++;
> > -  sFontFileStreams[fontFileKey] =
> > -    new DWriteFontFileStream(aData, aSize, fontFileKey);
> > -  RefPtr<IDWriteFontFileStream> ffsRef = sFontFileStreams[fontFileKey];
> 
> nit: I don't like this pattern, the object briefly exists here with a 0
> refcount, can you new it to the RefPtr and assign that to the map?

Done and I also changed gfxDWriteCommon.cpp from part 18 where I'd done the same thing.

	Pulsebot
	Comment 121 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/79733166f05e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4934e88b2d7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a8859afcdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a40eea914519
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4da453572c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b90d302c57f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8e59588518
https://hg.mozilla.org/integration/mozilla-inbound/rev/d748b1354f92
https://hg.mozilla.org/integration/mozilla-inbound/rev/dedca8fb19b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe429ee880c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd52947f73c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/48c6ce65a5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd34ea230c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/de352000aae1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac1c7e15a11
https://hg.mozilla.org/integration/mozilla-inbound/rev/99df86591613
https://hg.mozilla.org/integration/mozilla-inbound/rev/18844d26b873
https://hg.mozilla.org/integration/mozilla-inbound/rev/5355c636a81a
https://hg.mozilla.org/integration/mozilla-inbound/rev/16360fe94d54
https://hg.mozilla.org/integration/mozilla-inbound/rev/363829accc09
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d86b0471c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6d14d80238
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c026d5dcb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f08df57ff700
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c0aadae8e7

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 122 • 9 years ago

Backed out for build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdfae920430
https://treeherder.mozilla.org/logviewer.html#?job_id=18855644&repo=mozilla-inbound

	Brad Lassey [:blassey] (use needinfo?)
	Comment 123 • 9 years ago

renom'ing because we won't block on sandboxing

	Pulsebot
	Comment 124 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3834d920c4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9837ebf5d48
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3078a358d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/00b3ae3147bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d265787bad29
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0cb2ca72fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/d765b7934fae
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced236c18d9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f91b619a87
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42b8b6de581
https://hg.mozilla.org/integration/mozilla-inbound/rev/5167c1c18e40
https://hg.mozilla.org/integration/mozilla-inbound/rev/57d758c8ac22
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c13b9729336
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17ecf3189bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda221fc2e37
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7f18985869
https://hg.mozilla.org/integration/mozilla-inbound/rev/318888301328
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c51f0d7cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14e6cfb20dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6b129da39a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e490856b991
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67ed3a6ee9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ce8e300960
https://hg.mozilla.org/integration/mozilla-inbound/rev/cafdab832378
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e805e80b96

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 125 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e3834d920c4a
https://hg.mozilla.org/mozilla-central/rev/e9837ebf5d48
https://hg.mozilla.org/mozilla-central/rev/ef3078a358d8
https://hg.mozilla.org/mozilla-central/rev/00b3ae3147bc
https://hg.mozilla.org/mozilla-central/rev/d265787bad29
https://hg.mozilla.org/mozilla-central/rev/cf0cb2ca72fe
https://hg.mozilla.org/mozilla-central/rev/d765b7934fae
https://hg.mozilla.org/mozilla-central/rev/ced236c18d9d
https://hg.mozilla.org/mozilla-central/rev/f9f91b619a87
https://hg.mozilla.org/mozilla-central/rev/e42b8b6de581
https://hg.mozilla.org/mozilla-central/rev/5167c1c18e40
https://hg.mozilla.org/mozilla-central/rev/57d758c8ac22
https://hg.mozilla.org/mozilla-central/rev/6c13b9729336
https://hg.mozilla.org/mozilla-central/rev/f17ecf3189bd
https://hg.mozilla.org/mozilla-central/rev/dda221fc2e37
https://hg.mozilla.org/mozilla-central/rev/0b7f18985869
https://hg.mozilla.org/mozilla-central/rev/318888301328
https://hg.mozilla.org/mozilla-central/rev/f8c51f0d7cd4
https://hg.mozilla.org/mozilla-central/rev/c14e6cfb20dd
https://hg.mozilla.org/mozilla-central/rev/fa6b129da39a
https://hg.mozilla.org/mozilla-central/rev/6e490856b991
https://hg.mozilla.org/mozilla-central/rev/f67ed3a6ee9a
https://hg.mozilla.org/mozilla-central/rev/f2ce8e300960
https://hg.mozilla.org/mozilla-central/rev/cafdab832378
https://hg.mozilla.org/mozilla-central/rev/33e805e80b96

	leichixian
	Comment 126 • 8 years ago

Issue not fixed. When try to print some page (like this), plugin-container process will get stacked. I'll try with a clean profile later.

	leichixian
	Comment 127 • 8 years ago

Reproduced with a clean profile. Here are my screenshot and machine info.

	Alice0775 White
	Comment 128 • 8 years ago

I can reproduce the hang at "Progress: Preparing...".

https://hg.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20160106030225

	Bob Owen (:bobowen) Assignee
	Comment 129 • 8 years ago

(In reply to leichixian from comment #126)
> Created attachment 8704667 [details]
> 捕获.PNG
> 
> Issue not fixed. When try to print some page (like this), plugin-container
> process will get stacked. I'll try with a clean profile later.

Thanks for testing this.
Presumably this has broken things for more than just XPS.
If you set pref print.print_via_parent=false, does this fix it again for everything else?
(I really don't want to have all of this backed out.)

	Bob Owen (:bobowen) Assignee
	Comment 130 • 8 years ago

I've backed out the pref change, which fixes things for me on a clean profile.
(XPS problem is still there of course.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2f776d40c5

Now I just need to work out why the profile I was using for testing is working ...

	Bob Owen (:bobowen) Assignee
	Comment 131 • 8 years ago

It appears to be the paper size, if you print with letter it works.
I must have changed the profile prefs I was using to A4 for some reason.

It means the off screen surface is totally the wrong size, which then causes the reflow to hang.

Possibly some other existing hidden Windows printing bug I'm hitting.
I know vaguely where it might be, hopefully won't take long to track down.

	leichixian
	Comment 132 • 8 years ago

Changed  the pref. Now the process will not hang, but can't create file to transfer pages want to print into.

	Alice0775 White
	Comment 133 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #129)
> (In reply to leichixian from comment #126)
> > Created attachment 8704667 [details]
> > 捕获.PNG
> > 
> > Issue not fixed. When try to print some page (like this), plugin-container
> > process will get stacked. I'll try with a clean profile later.
> 
> Thanks for testing this.
> Presumably this has broken things for more than just XPS.
> If you set pref print.print_via_parent=false, does this fix it again for
> everything else?
> (I really don't want to have all of this backed out.)

NOT FIXED.
Save as dialog pops up and then I select a folder, but "Access denied" dialog pops up...

	Bob Owen (:bobowen) Assignee
	Comment 134 • 8 years ago

(In reply to Alice0775 White from comment #133)

> NOT FIXED.
> Save as dialog pops up and then I select a folder, but "Access denied"
> dialog pops up...

Right, the print to XPS (and Adobe PDF print and possibly print to file) will still be broken, but does other printing now work again?

	Alice0775 White
	Comment 135 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #134)
> (In reply to Alice0775 White from comment #133)
> 
> > NOT FIXED.
> > Save as dialog pops up and then I select a folder, but "Access denied"
> > dialog pops up...
> 
> does other printing now work again?

The native printer was working before Bug 1156742 landing.
And It is also working now.

	Bob Owen (:bobowen) Assignee
	Comment 136 • 8 years ago

(In reply to Alice0775 White from comment #135)

> The native printer was working before Bug 1156742 landing.
> And It is also working now.

Thanks Alice.

The pref disable has just been merged to m-c, so not sure if it will make today's Nightly.

I've found the paper size problem in the code, so should have a patch soonish.