Closed Bug 1156742 Opened 5 years ago Closed 4 years ago

[e10s] Low integrity content sandbox blocks Printing with "Microsoft XPS Document Writer" or "Print to file"

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox47 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

(Depends on 2 open bugs)

Details

Attachments

(27 files, 37 obsolete files)

7.94 KB, patch
bas.schouten
: review+
glandium
: review+
Details | Diff | Splinter Review
4.04 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
16.53 KB, patch
mconley
: review+
Details | Diff | Splinter Review
11.76 KB, patch
roc
: review+
mconley
: review+
Details | Diff | Splinter Review
10.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
29.29 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.74 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
10.05 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.84 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.43 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
7.56 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
10.69 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.87 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.18 KB, patch
mconley
: review+
Details | Diff | Splinter Review
11.62 KB, patch
mconley
: review+
Details | Diff | Splinter Review
29.03 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
30.96 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.65 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
15.42 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
2.28 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
61.73 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.21 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
442.36 KB, application/x-zip-compressed
Details
304.69 KB, application/zip
Details
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.
See Also: → 1090454
Duplicate of this bug: 1172398
Duplicate of this bug: 1173946
Duplicate of this bug: 1177884
tracking-e10s: --- → ?
Summary: Low integrity content sandbox blocks Printing with "Microsoft XPS Document Writer" or "Print to file" → [e10s] Low integrity content sandbox blocks Printing with "Microsoft XPS Document Writer" or "Print to file"
Blocks: e10s, 927188
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
(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.
Assignee: nobody → bobowen.code
See Also: → 1189846
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"
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. :-) )
Attachment #8651104 - Flags: feedback?(roc)
Attachment #8651104 - Flags: feedback?(jmathies)
Wow, really wish I'd found this before assuming it was an issue with my profile and formatting.
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.
Attachment #8651104 - Flags: feedback?(roc)
Attachment #8651104 - Flags: feedback?(bas)
Attachment #8651104 - Flags: feedback-
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.
(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.
(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.
Flags: needinfo?(bas)
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.
(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
Flags: needinfo?(bas) → needinfo?(bobowen.code)
Attachment #8651104 - Flags: feedback?(bas)
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.)
Attachment #8651104 - Flags: feedback?(jmathies)
(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.
Flags: needinfo?(bobowen.code)
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.
Attachment #8651104 - Attachment is obsolete: true
Duplicate of this bug: 1212619
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
Status: NEW → ASSIGNED
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.
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.
Attachment #8688979 - Flags: review?(bas)
Bug 1156742 Part 2: Make gfx thebes/gl/2d work with UNICODE defined. r?bas, r?glandium
Attachment #8688980 - Flags: review?(mh+mozilla)
Attachment #8688980 - Flags: review?(bas)
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.
Attachment #8688981 - Flags: review?(bas)
Bug 1156742 Part 4: Add an in memory DrawEventRecorder. r?bas
Attachment #8688982 - Flags: review?(bas)
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.
Attachment #8688983 - Flags: review?(mconley)
Bug 1156742 Part 6: Add RemotePrintJob to PrintSession and PrintData. r?roc, r?mconley

Someone knew that nsIPrintSession would come in handy one day.
Attachment #8688984 - Flags: review?(roc)
Attachment #8688984 - Flags: review?(mconley)
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.
Attachment #8688985 - Flags: review?(roc)
Bug 1156742 Part 8: Change gfxWindowsSurface, so that a non-printing surface can be used when recording a print. r?roc
Attachment #8688986 - Flags: review?(roc)
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.
Attachment #8688987 - Flags: review?(roc)
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.
Attachment #8688988 - Flags: review?(roc)
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.
Attachment #8688989 - Flags: review?(roc)
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.
Attachment #8688990 - Flags: review?(bas)
Bug 1156742 Part 13: Create a Moz2D PrintTranslator. r?bas

A better solution to the ScaledFonts issue comes in Part 24.
Attachment #8688991 - Flags: review?(bas)
Bug 1156742 Part 14: Complete RemotePrintJob using PrintTranslator. r?mconley
Attachment #8688992 - Flags: review?(mconley)
Bug 1156742 Part 15: Add pref for turning on printing via the parent process. r?mconley
Attachment #8688993 - Flags: review?(mconley)
Bug 1156742 Part 16: Add recording and forwarding of Matrix attribute set for Moz2D recording. r?bas
Attachment #8688994 - Flags: review?(bas)
Bug 1156742 Part 17: Add virtual destructor to RecorededEvent and fix subsequent crash with DWrite playback fonts. r?bas
Attachment #8688995 - Flags: review?(bas)
Bug 1156742 Part 18: Fix the way we hold custom font data so that they can be recorded with Moz2D. r?bas
Attachment #8688996 - Flags: review?(bas)
Bug 1156742 Part 19: Implement GetFontFileData for ScaledFontWin. r?bas
Attachment #8688997 - Flags: review?(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.
Attachment #8688998 - Flags: review?(bas)
Bug 1156742 Part 21: Add and use Moz2D preference access to layers.acceleration.disabled. r?bas
Attachment #8688999 - Flags: review?(bas)
Bug 1156742 Part 22: Change ScaledFontDWrite to support creation from TrueType Collection data. r?bas
Attachment #8689000 - Flags: review?(bas)
Bug 1156742 Part 23: Add null checks to lookups in Moz2D RecordedEvent playback. r?bas
Attachment #8689001 - Flags: review?(bas)
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.
Attachment #8689002 - Flags: review?(bas)
Bug 1156742 Part 25: Flip the big switch and wait for the lightning. r?mconley
Attachment #8689003 - Flags: review?(mconley)
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/
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.
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
Attachment #8688979 - Attachment is obsolete: true
Attachment #8688979 - Flags: review?(bas)
Attachment #8688980 - Attachment is obsolete: true
Attachment #8688980 - Flags: review?(mh+mozilla)
Attachment #8688980 - Flags: review?(bas)
Attachment #8688981 - Attachment is obsolete: true
Attachment #8688981 - Flags: review?(bas)
Attachment #8688982 - Attachment is obsolete: true
Attachment #8688982 - Flags: review?(bas)
Attachment #8688983 - Attachment is obsolete: true
Attachment #8688983 - Flags: review?(mconley)
Attachment #8688984 - Attachment is obsolete: true
Attachment #8688984 - Flags: review?(roc)
Attachment #8688984 - Flags: review?(mconley)
Attachment #8688985 - Attachment is obsolete: true
Attachment #8688985 - Flags: review?(roc)
Attachment #8688986 - Attachment is obsolete: true
Attachment #8688986 - Flags: review?(roc)
Attachment #8688987 - Attachment is obsolete: true
Attachment #8688987 - Flags: review?(roc)
Attachment #8688988 - Attachment is obsolete: true
Attachment #8688988 - Flags: review?(roc)
Attachment #8688989 - Attachment is obsolete: true
Attachment #8688989 - Flags: review?(roc)
Attachment #8688990 - Attachment is obsolete: true
Attachment #8688990 - Flags: review?(bas)
Attachment #8688991 - Attachment is obsolete: true
Attachment #8688991 - Flags: review?(bas)
Attachment #8688992 - Attachment is obsolete: true
Attachment #8688992 - Flags: review?(mconley)
Hmm ... looks like I broke Review Board.

I think I'm going to have to upload them all again manually.
Sorry for the spam.
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.
Attachment #8661706 - Attachment is obsolete: true
Attachment #8689014 - Flags: review?(bas)
Parts of this change and related code get moved around in Part 24.
Attachment #8689016 - Flags: review?(bas)
The methods will get filled out by later patches, this sets up the IPDL and lifetime management.
Attachment #8689018 - Flags: review?(mconley)
Someone knew that nsIPrintSession would come in handy one day.
Attachment #8689019 - Flags: review?(roc)
Attachment #8689019 - Flags: review?(mconley)
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.
Attachment #8689021 - Flags: review?(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.
Attachment #8689026 - Flags: review?(roc)
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.
Attachment #8689027 - Flags: review?(roc)
This is so the RemotePrintJobParent can abort the printing in the child when something goes wrong.
Attachment #8689028 - Flags: review?(roc)
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.
Attachment #8689029 - Flags: review?(bas)
A better solution to the ScaledFonts issue comes in Part 24.
Attachment #8689030 - Flags: review?(bas)
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. :-(
Even the remaining ones are pretty broken on review board, so I'm going to upload manually.
Attachment #8688993 - Attachment is obsolete: true
Attachment #8688994 - Attachment is obsolete: true
Attachment #8688995 - Attachment is obsolete: true
Attachment #8688996 - Attachment is obsolete: true
Attachment #8688997 - Attachment is obsolete: true
Attachment #8688998 - Attachment is obsolete: true
Attachment #8688999 - Attachment is obsolete: true
Attachment #8689000 - Attachment is obsolete: true
Attachment #8689001 - Attachment is obsolete: true
Attachment #8689002 - Attachment is obsolete: true
Attachment #8689003 - Attachment is obsolete: true
Attachment #8688993 - Flags: review?(mconley)
Attachment #8688994 - Flags: review?(bas)
Attachment #8688995 - Flags: review?(bas)
Attachment #8688996 - Flags: review?(bas)
Attachment #8688997 - Flags: review?(bas)
Attachment #8688998 - Flags: review?(bas)
Attachment #8688999 - Flags: review?(bas)
Attachment #8689000 - Flags: review?(bas)
Attachment #8689001 - Flags: review?(bas)
Attachment #8689002 - Flags: review?(bas)
Attachment #8689003 - Flags: review?(mconley)
Attachment #8689055 - Flags: review?(mconley)
This is so we can add a new preference in Part 21, which has nothing to do with logging.
Attachment #8689066 - Flags: review?(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.
Attachment #8689071 - Flags: review?(bas)
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
Attachment #8689018 - Flags: review?(mconley) → review+
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.
Attachment #8689019 - Flags: review?(mconley) → review+
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. :)
Attachment #8689032 - Flags: review?(mconley) → review+
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.
Attachment #8689055 - Flags: review?(mconley) → review+
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?
Attachment #8689073 - Flags: review?(mconley) → review+
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.
Attachment #8689032 - Attachment is obsolete: true
Attachment #8689596 - Flags: review?(mconley)
Attachment #8689015 - Flags: review?(mh+mozilla) → review+
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
Attachment #8689019 - Flags: review?(roc) → review+
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.
Attachment #8689026 - Flags: review?(roc)
(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?
Flags: needinfo?(roc)
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.
Attachment #8689026 - Flags: review+
Flags: needinfo?(roc)
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.
Attachment #8689596 - Flags: review?(mconley) → review+
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.
(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.
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.)
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.
Attachment #8689014 - Flags: review?(bas) → review+
Attachment #8689015 - Flags: review?(bas) → review+
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?
Attachment #8689016 - Flags: review?(bas) → review+
Attachment #8689017 - Flags: review?(bas) → review+
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
Attachment #8689029 - Flags: review?(bas) → review+
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.
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.
Attachment #8689030 - Flags: review?(bas) → review+
Attachment #8689060 - Flags: review?(bas) → review+
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?
Attachment #8689061 - Flags: review?(bas) → review+
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?
Attachment #8689063 - Flags: review?(bas) → review+
Attachment #8689064 - Flags: review?(bas) → review+
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?
Attachment #8689066 - Flags: review?(bas) → review+
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..
Attachment #8689067 - Flags: review?(bas) → review-
Attachment #8689069 - Flags: review?(bas) → review+
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.
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.
(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 :).
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.
Attachment #8689014 - Attachment is obsolete: true
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.
Attachment #8693639 - Flags: review+
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.
Attachment #8689016 - Attachment is obsolete: true
Attachment #8689061 - Attachment is obsolete: true
Attachment #8693640 - Flags: review+
r=bas - from comment 105 - again moved to destructor as previous.
Attachment #8689063 - Attachment is obsolete: true
Attachment #8693641 - Flags: review+
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.
Attachment #8689070 - Attachment is obsolete: true
Attachment #8689070 - Flags: review?(bas)
Attachment #8693642 - Flags: review?(bas)
Bring part 24 into line with changes in part 23.
Attachment #8689071 - Attachment is obsolete: true
Attachment #8689071 - Flags: review?(bas)
Attachment #8693644 - Flags: review?(bas)
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.
Attachment #8689067 - Attachment is obsolete: true
Attachment #8694330 - Flags: review?(bas)
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.
Flags: needinfo?(jfkthame)
Attachment #8694330 - Flags: review?(bas) → review+
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?
Attachment #8693644 - Flags: review?(bas) → review+
Attachment #8693642 - Flags: review?(bas) → review+
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.
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
renom'ing because we won't block on sandboxing
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
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Attached image 捕获.PNG (obsolete) —
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.
Attached file testinfo.zip (obsolete) —
Reproduced with a clean profile. Here are my screenshot and machine info.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.)
Flags: needinfo?(leichixian)
Flags: needinfo?(bobowen.code)
Flags: needinfo?(alice0775)
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 ...
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.
No longer blocks: 1093774
Changed  the pref. Now the process will not hang, but can't create file to transfer pages want to print into.
Flags: needinfo?(leichixian)
(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...
Flags: needinfo?(alice0775)
(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?
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
(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.
Is this Bug related to Bug 1240370?

Bug 1240370 - Nightly crashes in an attempt to print out a page with Microsoft XPS Document Writer
Duplicate of this bug: 1240370
Hi all,
As you have all had problems when I turned on printing via the parent last time, would you mind retesting now that I have landed fixes for the two outstanding problems.

You will need to update to the latest Nightly and also set the pref:
print.print_via_parent=true

Thanks.
Flags: needinfo?(leichixian)
Flags: needinfo?(foobar094)
Flags: needinfo?(alice0775)
Tested on my machine (Windows 10 14251 x64 build, Nightly 47.0 20160128, Intel Core i5-6200U, AMD Radeon R5 M335 but browser was forced to use Intel graphics 520) with a fresh profile. Now I can just print, but the printed file don't like the page shown in browser. Does this relates to my graphic setting?
Attachment #8704667 - Attachment is obsolete: true
Attachment #8704674 - Attachment is obsolete: true
Flags: needinfo?(leichixian) → needinfo?(bobowen.code)
My platform: Nightly (2016-01-28) on x86_32 Windows 7

Nightly doesn't crash anymore. Thanks! ;)
Here is the result of printing.
Flags: needinfo?(foobar094)
(In reply to Ryo Kato from comment #141)
> Created attachment 8713426 [details]
> Print test on x86_32 Windows 7
> 
> My platform: Nightly (2016-01-28) on x86_32 Windows 7
> 
> Nightly doesn't crash anymore. Thanks! ;)
> Here is the result of printing.

(In reply to leichixian from comment #140)
> Created attachment 8713409 [details]
> Print test on my machine
> 
> Tested on my machine (Windows 10 14251 x64 build, Nightly 47.0 20160128,
> Intel Core i5-6200U, AMD Radeon R5 M335 but browser was forced to use Intel
> graphics 520) with a fresh profile. Now I can just print, but the printed
> file don't like the page shown in browser. Does this relates to my graphic
> setting?

Thanks both. :-)

leichixian - when I print those two pages to pdf in Release I get very similar prints, so it must just be the way the pages are set to print. I don't think it is your graphics set-up.

I notice that our headers and footers differ slightly, but they are set to different formats, so that probably explains that.

Thanks again for testing and please check any problems you find against Release or Nightly with e10s turned off.
If you find e10s / sandbox specific ones, please CC me if you file them as bugs.
Flags: needinfo?(bobowen.code)
(In reply to Bob Owen (:bobowen) from comment #139)
> Hi all,
> As you have all had problems when I turned on printing via the parent last
> time, would you mind retesting now that I have landed fixes for the two
> outstanding problems.
> 
> You will need to update to the latest Nightly and also set the pref:
> print.print_via_parent=true
> 
> Thanks.
"Microsoft XPS Document Writer" forks forme.
Flags: needinfo?(alice0775)
s/forks/works/
(In reply to Alice0775 White from comment #143)

> "Microsoft XPS Document Writer" forks forme.

Thanks Alice, I'll get this turned back on by default.
https://hg.mozilla.org/mozilla-central/rev/69e6782758af
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1184936
This isn't marked with a milestone for e10s, but on this page, https://wiki.mozilla.org/E10s/Status/m8#m8_Tracker it's listed as an m8 blocker not yet uplifted to 46.   Is this an m8 blocker?
Flags: needinfo?(bobowen.code)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #149)
> This isn't marked with a milestone for e10s, but on this page,
> https://wiki.mozilla.org/E10s/Status/m8#m8_Tracker it's listed as an m8
> blocker not yet uplifted to 46.   Is this an m8 blocker?

This was only blocking the sandbox and we decided not to block e10s on that.

So, I think it should be removed from that tracker.
tracking-e10s: ? → ---
Flags: needinfo?(bobowen.code)
Thanks Bob, that clears some things up for me!
Depends on: 1257124
Depends on: 1258609
See Also: → 1228022
Depends on: 1308394
Depends on: 1316783
See Also: → 1318845
Depends on: 1328072
Depends on: 1315212
Depends on: 1329274
Tidying up some of my needinfos.
Flags: needinfo?(jfkthame)
Depends on: 1263503
No longer depends on: 1263503
You need to log in before you can comment on or make changes to this bug.