[Tracking Requested - why for this release]: Unable to Print to file on Ubuntu16.04LTS 32bit. The problem does present in Beta50.0b5 and Aurora51.0a2(10-Oct-2016). However, the problem is reproducible in Nightly52.0a1(10-Oct-2016). Steps To Reproduce: 1. Open about:home (or any simple page) 2. File > Print 3. Select "Print to file" and check pdf 4. Input file name and Click on [Print] Actual Results: The file does not created. No error appears. Expected Results; The file does should be created. Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3e9a2031152fa07b088d0cb5e168eb53a2c882c0&tochange=c838d2546cadd65bf8d5579db20a268c8b6e4b87 Regressed by: Bug 1289718

Actual Results: The file is not created. No error appears. Expected Results; The file should be created.

I can also reproduce the problem on Nightly52.0a2 Ubuntu16.04 64bit.

Sandbox: SandboxBroker: denied op=0 rflags=301 perms=3 path=/home/morbo/hg/firefox/mozilla.pdf for pid=10500 error="No such file or directory" Sandbox: Rejected errno -13 op 0 flags 0301 path /home/morbo/hg/firefox/mozilla.pdf There's also this but that's probably not fatal. Sandbox: SandboxBroker: denied op=0 rflags=302 perms=3 path=/home/morbo/.local/share/recently-used.xbel.FPH1OY for pid=10500 error="No such file or directory" Sandbox: Rejected errno -13 op 3 flags 0600 path /home/morbo/.local/share/recently-used.xbel (/home/morbo/hg/firefox/objdir-desktop/dist/bin/plugin-container:10500): Gtk-WARNING **: Attempting to set the permissions of '/home/morbo/.local/share/recently-used.xbel', but failed: Permission denied Obviously this is not something we can whitelist.

Actually, Linux still sets print.print_via_parent=false. So Linux should support printing on chrome like bug 1228022.

Tracking 52+ for the printing regression which affects Linux.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3) > Obviously this is not something we can whitelist. Disable e10s on current Nightly build will be able to fix this issue. Could you provide more pointers or suggestions on how to fix ?

As pointed out in comment 4, this needs work to do printing on Linux via the parent, similar to the work done in bug 1228022.

You can also flip the pref security.sandbox.content.level=2 back to 1.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8) > You can also flip the pref security.sandbox.content.level=2 back to 1. What's the downside of doing above change ? In comparison to this function broken.

(In reply to Astley Chen [:astley] (UTC+8) from comment #9) > (In reply to Gian-Carlo Pascutto [:gcp] from comment #8) > > You can also flip the pref security.sandbox.content.level=2 back to 1. > > What's the downside of doing above change ? In comparison to this function > broken. https://groups.google.com/forum/#!searchin/mozilla.dev.platform/linux$20sandbox%7Csort:relevance/mozilla.dev.platform/V-5G9dWJEXo/S9qxkg9EAgAJ Disabling it makes Firefox much more vulnerable to exploits. Note that the change is limited to Nightly only and does not ride the trains.

MozReview-Commit-ID: 19Ap6dXkLXp

Comment on attachment 8811391 [details] [diff] [review] Implement NativeFontResourceLinux and ScaledFontSkia This is my WIP. It looks like on Linux the native fonts are Cairo fonts/Fontconfig and Skia. I don't see any good way to serialize/get raw font data with Cairo, and we seem to be moving everything to Skia, which does have infrastructure for that, so I wrote a ScaledFontSkia. I have no idea whether I'm on the right track here. I also have no idea what the relevance of the "aIndex and aGlyphSize" arguments in the CreateScaledFont call are. Jonathan, Lee, are you willing to take a look and tell me if I'm on the right track or horribly misguided? I'm trying to get DrawTargetRecording/RecordedFontData and friends to work on Linux.

Tracking 53+ for this printing regression.

Comment on attachment 8811391 [details] [diff] [review] Implement NativeFontResourceLinux and ScaledFontSkia Review of attachment 8811391 [details] [diff] [review]: ----------------------------------------------------------------- This is a bad idea as written. Our Skia font setup is really just a wrapper around Cairo and Fontconfig and does not support serialize the resulting typefaces. So if Cairo/Fontconfig won't let you serialize, then our Skia font host will not do it either. I'm not sure there is any good way to do this at all without going through the info used to generate the fonts in the first place in gfxFcPlatformFontList.

I don't have much grasp of how we deal with fonts in skia, but in principle it seems like we should be able to serialize fonts there if that's what is needed. The cairo fonts are really just wrappers around freetype, right? And if we get the FT_Face using cairo_ft_scaled_font_lock_face(), we can then access the entire font data using FT_Load_Sfnt_Table() with tag=0.

(In reply to Jonathan Kew (:jfkthame) from comment #16) > I don't have much grasp of how we deal with fonts in skia, but in principle > it seems like we should be able to serialize fonts there if that's what is > needed. The cairo fonts are really just wrappers around freetype, right? And > if we get the FT_Face using cairo_ft_scaled_font_lock_face(), we can then > access the entire font data using FT_Load_Sfnt_Table() with tag=0. Hmm, isn't this a bit excessive as well? We don't necessarily know the actual underlying font we're using from fontconfig till basically the last minute when we render all the way down in Skia. I was imagining it would be safer to pass in the identifying info from the font list, like the font family, and initial parameters to the fontconfig pattern, etc. and just recreate the fontconfig pattern on the other side?

(In reply to Lee Salzman [:lsalzman] from comment #17) > (In reply to Jonathan Kew (:jfkthame) from comment #16) > > I don't have much grasp of how we deal with fonts in skia, but in principle > > it seems like we should be able to serialize fonts there if that's what is > > needed. The cairo fonts are really just wrappers around freetype, right? And > > if we get the FT_Face using cairo_ft_scaled_font_lock_face(), we can then > > access the entire font data using FT_Load_Sfnt_Table() with tag=0. > > Hmm, isn't this a bit excessive as well? We don't necessarily know the > actual underlying font we're using from fontconfig till basically the last > minute when we render all the way down in Skia. I was imagining it would be > safer to pass in the identifying info from the font list, like the font > family, and initial parameters to the fontconfig pattern, etc. and just > recreate the fontconfig pattern on the other side? Will that work for fonts that were loaded via @font-face in the content process, and therefore presumably are not known at all to fontconfig in the parent?

> Will that work for fonts that were loaded via @font-face in the content > process, and therefore presumably are not known at all to fontconfig in the > parent? AFAIK it won't. We currently support serializing fonts on Win and Mac exactly to handle this case, but missing Linux support is a blocker for this bug.

https://dxr.mozilla.org/mozilla-central/rev/0ddfec7126ec503b54df9c4b7c3b988906f6c882/gfx/2d/DrawTargetRecording.cpp#394 As said above, we try to serialize the font data so we are handle the web font case, but only if that fails we fall back to trying to pass a description, which I think is what Lee proposes, and works for system fonts but not web fonts.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #20) > https://dxr.mozilla.org/mozilla-central/rev/ > 0ddfec7126ec503b54df9c4b7c3b988906f6c882/gfx/2d/DrawTargetRecording.cpp#394 > > As said above, we try to serialize the font data so we are handle the web > font case, but only if that fails we fall back to trying to pass a > description, which I think is what Lee proposes, and works for system fonts > but not web fonts. That's more or less my proposal. There is also a consideration that if you're trading so much raw data over IPC for printing to a file, maybe it becomes better to just print to a temporary file within the confines of the sandbox, then let the other end rename the file to the correct destination...

(In reply to Lee Salzman [:lsalzman] from comment #21) > That's more or less my proposal. There is also a consideration that if > you're trading so much raw data over IPC for printing to a file, maybe it > becomes better to just print to a temporary file within the confines of the > sandbox, then let the other end rename the file to the correct destination... We're considering doing this (we ran into OOM errors on Windows where the described approach is used) but it's not entirely desirable long term because eventually we will want to remove file IO permissions entirely in content.

updating status for 52 as the content sandbox is only enabled in nightly on linux

This is a work-in-progress version of making print_via_parent work better on Linux. I still need to clean this up for a final patch, but everything is pretty much there and roughly seems to build and work with what little testing I've done. Just putting this up in case anyone has useful feedback. Per discussions among the graphics team, we decided to not pull gfxPlatform/FontList stuff into Moz2d, and keep clear the separation for Moz2d and thebes for now. As a result, this necessitated relying on ScaledFontFontconfig (rather than gfxFcPlatformFontList) and the information contained in FcPattern to deconstruct the font and rebuild it "equivalently" on the other side. So there's now a considerable bit of heft in ScaledFontFontconfig to do this. Also, there was never a way to associate instance data with scaled font creation in DrawTargetRecording previously, so there was no way to actually send that FcPattern info across, but now there is... yay. Also, getting an SFNT table for many valid printable fonts was seemingly not even possible on my Linux setup, so this necessitated sending across a pathname (= font descriptor) instead of sending file contents. So I had to generalize font descriptors to work with more platforms than just Windows/GDI, since there was no way to do that yet, but now there is. The other nasty issue was that we need access to the instance of the FT_Library used in thebes for creating FT_Faces, so I had to implement a way to get that information out of thebes and shuttle it into Moz2d via the Factory.

Need to be able to export FT_Library from gfxPlatform into Moz2d so that we can instantiate FT_Faces there. This just takes the existing GetFTLibrary() calls that were somewhat inconsistent and makes them virtual, and adds some machinery to Moz2d Factory to accept the result.

This adds a Fontconfig version of NativeFontResource. The basic approach is what Jonathan suggested earlier, namely that of trying to load the SFNT data for the font when we can. This doesn't always succeed for some types of fonts, so it was also necessary to fallback to shipping a pathname/index font descriptor over when SFNT data is not available. Because it is not possible to reconstruct a similar ScaledFontFontconfig with just the SFNT data, it was also necessary to add a bit of machinery for shipping over "instance data" for ScaledFonts, wherein we serialize and deserialize the FcPattern and some oddball Cairo font options that don't fit into the FcPattern. Other platforms will probably have to use this as well, but for now I merely sought to keep this issue Linux specific.

Comment on attachment 8822239 [details] [diff] [review] allow querying FT_Library from gfxPlatform so that Moz2d Factory can use it Review of attachment 8822239 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +743,5 @@ > MOZ_CRASH("Could not initialize gfxFontCache"); > } > > +#ifdef MOZ_ENABLE_FREETYPE > + Factory::SetFTLibrary(gPlatform->GetFTLibrary()); Do we need a matching Factory::SetFTLibrary(nullptr); on shutdown?

(In reply to Milan Sreckovic [:milan] from comment #29) > Comment on attachment 8822239 [details] [diff] [review] > allow querying FT_Library from gfxPlatform so that Moz2d Factory can use it > > Review of attachment 8822239 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxPlatform.cpp > @@ +743,5 @@ > > MOZ_CRASH("Could not initialize gfxFontCache"); > > } > > > > +#ifdef MOZ_ENABLE_FREETYPE > > + Factory::SetFTLibrary(gPlatform->GetFTLibrary()); > > Do we need a matching Factory::SetFTLibrary(nullptr); on shutdown? At least in the current style of doing things in the Factory, and given that FT_Library is not reference-counted, we do not.

Comment on attachment 8822239 [details] [diff] [review] allow querying FT_Library from gfxPlatform so that Moz2d Factory can use it Review of attachment 8822239 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK, but I'd suggest adding a few sanity-checking assertions (see below). r=me with these things addressed (or if you explain why they don't make sense after all...) ::: gfx/2d/Factory.cpp @@ +582,5 @@ > + > +FT_Library > +Factory::GetFTLibrary() > +{ > + return mFTLibrary; I'm guessing we shouldn't ever call this unless a library has previously been set with SetFTLibrary (right?). If so, how about adding a MOZ_ASSERT(mFTLibrary) here to help us catch any future misuse? ::: gfx/thebes/gfxFT2FontList.cpp @@ +83,5 @@ > } > > NS_ASSERTION(!aFontEntry->mFilename.IsEmpty(), > "can't use AutoFTFace for fonts without a filename"); > + FT_Library ft = gfxPlatform::GetPlatform()->GetFTLibrary(); How about adding MOZ_ASSERT(ft) here, to check that we're not misusing this code on a platform that doesn't provide it. ::: gfx/thebes/gfxPlatform.cpp @@ +743,5 @@ > MOZ_CRASH("Could not initialize gfxFontCache"); > } > > +#ifdef MOZ_ENABLE_FREETYPE > + Factory::SetFTLibrary(gPlatform->GetFTLibrary()); The ~gfxAndroidPlatform() destructor calls FT_Done_Library, so I think that before tearing down the gfxPlatform, we should call Factory::SetFTLibrary(nullptr) to disconnect it -- and the Factory should assert if it tries to use the library after it has been cleared. Should we assert here that the library is non-null? Or make the base implementation of GetFTLibrary (in gfxPlatform.h) assert instead of silently returning null? ISTM that we don't want to be mixing code that expects an FTLibrary to be available with platforms that don't provide it, so adding some assertions to catch any such case seems like it'd be a good thing.

Comment on attachment 8822244 [details] [diff] [review] provide NativeFontResourceFontconfig so that print_via_parent works on Linux Review of attachment 8822244 [details] [diff] [review]: ----------------------------------------------------------------- Marking this r- for now until the issues noted below are resolved, but I expect an updated version will be fine. ::: gfx/2d/2D.h @@ +679,5 @@ > MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(ScaledFont) > virtual ~ScaledFont() {} > > typedef void (*FontFileDataOutput)(const uint8_t *aData, uint32_t aLength, uint32_t aIndex, Float aGlyphSize, void *aBaton); > + typedef void (*FontInstanceDataOutput)(const uint8_t *aData, uint32_t aLength, void *aBaton); nit: coding style violation here (position of the *), but I see it's matching the surrounding lines. Care to fix them all together? (Sigh... it's wildly inconsistent in this file, actually, at least from the excerpts I'm seeing here, so maybe just ignore it for now. Global style cleanup would be better done separately.) ::: gfx/2d/Factory.cpp @@ +523,3 @@ > return NativeFontResourceMac::Create(aData, aSize); > +#elif defined(MOZ_WIDGET_GTK) > + return NativeFontResourceFontconfig::Create(aData, aSize); I was a bit surprised to see use of MOZ_WIDGET_GTK here, as that seems Gecko-specific and I thought moz2d was supposed to be separately buildable. Or is that no longer the case? Anyhow, I see we've had it up at the top of this file for several months, at least, so I guess it's OK. Bit of a mixture of conditionals here... WIN32 vs XP_DARWIN vs MOZ_WIDGET_GTK. It would be tidier to use all XP_* conditionals (if it's the platform that is most relevant) or all MOZ_WIDGET_* conditionals (if it's the widget toolkit). Can we harmonize things here? ::: gfx/2d/NativeFontResourceDWrite.cpp @@ +259,5 @@ > } > > already_AddRefed<ScaledFont> > +NativeFontResourceDWrite::CreateScaledFont(uint32_t aIndex, Float aGlyphSize, > + const uint8_t* aData, uint32_t aDataLength) I see you've changed aGlyphSize from integer to floating-point here. That's probably a good thing (looks suspiciously like it was simply a mistake before), but the change doesn't belong in this patch, or even in this bug. Please split it out into its own bug for separate landing. The aData parameter here is "instance data", isn't it (not font data, i.e. the actual sfnt resource, which is what I'd instinctively guess just from seeing this method signature)? Please use the more explicit name aInstanceData to make this a bit clearer. (Applies to all the declarations/definitions of this method.) ::: gfx/2d/NativeFontResourceFontconfig.cpp @@ +54,5 @@ > +} > + > +already_AddRefed<ScaledFont> > +NativeFontResourceFontconfig::CreateScaledFont(uint32_t aIndex, Float aGlyphSize, > + const uint8_t* aData, uint32_t aDataLength) aInstanceData ::: gfx/2d/NativeFontResourceGDI.cpp @@ +64,5 @@ > ::RemoveFontMemResourceEx(mFontResourceHandle); > } > > already_AddRefed<ScaledFont> > +NativeFontResourceGDI::CreateScaledFont(uint32_t aIndex, Float aGlyphSize, Here again, you're changing an integer parameter to a float; I think that should be done separately. ::: gfx/2d/NativeFontResourceMac.cpp @@ +49,5 @@ > return fontResource.forget(); > } > > already_AddRefed<ScaledFont> > +NativeFontResourceMac::CreateScaledFont(uint32_t aIndex, Float aGlyphSize, integer -> float again ::: gfx/2d/RecordedEvent.cpp @@ +1589,2 @@ > RefPtr<ScaledFont> font = > + Factory::CreateScaledFontFromFontDescriptor(mType, &mData[0], mData.size(), mFontSize); Don't use &mData[0], use mData.data() (see below). I guess the descriptor wouldn't ever be empty, but still, it seems like bad style. @@ +1646,5 @@ > } > > + RefPtr<ScaledFont> scaledFont = > + fontResource->CreateScaledFont(mIndex, mGlyphSize, > + mData.empty() ? nullptr : &mData[0], Do we need the explicit empty() check here, if you use mData.data() instead of &mData[0]? @@ +1660,5 @@ > WriteElement(aStream, mFontDataKey); > WriteElement(aStream, mIndex); > WriteElement(aStream, mGlyphSize); > + WriteElement(aStream, (size_t)mData.size()); > + aStream.write((char*)&mData[0], mData.size()); This is unsafe (&mData[0] will be undefined behavior if mData is empty). To avoid this, I think the right approach is to use mData.data() instead. (Or you could make the write conditional on size() being non-zero, but that's an extra branch we might not want.) @@ +1686,5 @@ > + > + size_t size; > + ReadElement(aStream, size); > + mData.resize(size); > + aStream.read((char*)&mData[0], size); Again, I don't think this is safe if size is zero. Use mData.data(). ::: gfx/2d/RecordedEvent.h @@ +1099,5 @@ > > class RecordedScaledFontCreation : public RecordedEvent { > public: > > + static void FontInstanceDataProc(const uint8_t *aData, uint32_t aSize, void* aBaton) nit: the * should be attached to the type, not to aData. @@ +1133,5 @@ > ReferencePtr mRefPtr; > uint64_t mFontDataKey; > Float mGlyphSize; > uint32_t mIndex; > + std::vector<uint8_t> mData; Please name this mInstanceData, to match terminology used elsewhere (and to make it less likely it'll be misunderstood as being the actual font data itself). ::: gfx/2d/ScaledFontFontconfig.h @@ +24,5 @@ > + HINT_METRICS = 1 << 5 > + }; > + > + FontconfigInstanceData() > + {} Do we actually need this constructor, or can we delete it (or at least make it private)? If we do need to keep it, should it initialize the fields to safe defaults?

Added assertions as requested and made mFTLibrary reset to null when Factory/Platform is shut down.

... v2 Left the style issues aside for the moment. Probably best to leave those for separate cleanup bugs, since the platform macros especially open up a big can of worms in all of Moz2d that needs to be decided on, not just for Factory. After talking with Jeff, the MOZ_WIDGET_GTK macro for now makes the most sense as these Fontconfig bits are meant to target our GTK platforms. As for Float aGlyphSize, separated that out as bug 1328337 and made this bug depend on that one. In process of landing that. Renamed aData/mData stuff to aInstanceData/mInstanceData. Changed &mData[0] to mData.data() Fixed the * style issue in some of the mentioned places Removed the FontconfigInstanceData default constructor and moved the struct inside ScaledFontFontconfig so that it is private

Comment on attachment 8823385 [details] [diff] [review] provide NativeFontResourceFontconfig so that print_via_parent works on Linux Review of attachment 8823385 [details] [diff] [review]: ----------------------------------------------------------------- Looks better, thanks; one smart-pointer usage nit we could fix up, if I'm reading this right. ::: gfx/2d/NativeFontResourceFontconfig.cpp @@ +43,5 @@ > + return nullptr; > + } > + > + RefPtr<NativeFontResourceFontconfig> resource = > + new NativeFontResourceFontconfig(fontData.release(), face); Rather than using fontData.release(), I think we should make the NativeFontResourceFontconfig constructor explicitly take a UniquePtr, and then do Move(fontData) here.

Marking it as blocking as we would not release 53 with this bug.

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c25a123203a part 1 - allow querying FT_Library from gfxPlatform so that Moz2d Factory can use it. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/2797f193a147 part 2 - provide NativeFontResourceFontconfig so that print_via_parent works on Linux. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9702d8fe4e part 3 - enable print_via_parent on Linux. r=bobowen

https://hg.mozilla.org/mozilla-central/rev/5c25a123203a https://hg.mozilla.org/mozilla-central/rev/2797f193a147 https://hg.mozilla.org/mozilla-central/rev/5b9702d8fe4e

Reproduced on Nightly 2016-10-10, Ubuntu 14.04 x64. Verified fixed Fx 53b1.

Note that printing to file doesn't work on Windows too. "An error occurred while printing" message is displayed. Is this expected?

(In reply to Paul Silaghi, QA [:pauly] from comment #40) > Note that printing to file doesn't work on Windows too. > "An error occurred while printing" message is displayed. > Is this expected? I did not make changes to Windows printing so I can't offer any insight on why that might be.