1329216 - crash on "print" in print dialog

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Michael Bonfils]

Attachment: GDB backtrace

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170106030204

Steps to reproduce:

Load a simple webpage
Try to print webpage with Control-P


Actual results:

Firefox child crash 


Expected results:

The dialog printer

Comment 1

[Michael Bonfils]

Seen with Firefox 53.0a1 (2017-01-06) (64-bit) with Debian GNU/Linux 8.6 (jessie) 
On same computer, with Firefox 45.6.0, no issue with printer dialog

Comment 2

[Loic]

WFm with FF53 on Win 7.

Could you use the tool mozregression to narrow down a regression range.
Run the command "mozregression --good=45" then copy here the final pushlog.
http://mozilla.github.io/mozregression/

Comment 3

[Michael Bonfils]

Attachment: moz_regression.log

Result of mozregression --good=45

Comment 4

[Michael Bonfils]

I confirm that following https://wiki.mozilla.org/Security/Sandbox/Seccomp with 

export MOZ_DISABLE_CONTENT_SANDBOX=1

my firefox doesn't crash anymore while displaying Print Dialog

And launching firefox from console, I got:

(/home/bonfils/Personnel/apps/firefox/firefox:832): GLib-GObject-CRITICAL **: g_object_unref: assertion 'object->ref_count > 0' failed
Sandbox: seccomp sandbox violation: pid 832, syscall 307, args 36 140723904620672 2 16384 0 0.  Killing process.
Sandbox: JS frame 0: getPrintSettings chrome://global/content/browser-content.js line 482
Sandbox: JS frame 1: print chrome://global/content/browser-content.js line 666
Sandbox: JS frame 2: receiveMessage chrome://global/content/browser-content.js line 469
[Parent 736] WARNING: pipe error (48): Connection reset by peer: file /builds/slave/m-cen-l64-ntly-000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346

Comment 5

[Loic]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c676d55b6b006a2edb37c7c29c64e69f7cb8012a&tochange=23140396a80eb27ff586c41fdc1cad62c875c9b1

 8:35.55 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=742434


Michael, do you crash with FF50 too?

Comment 6

[Julian Hector [:tedd] [:jhector]]

:gcp, could this be related to Bug 1309205?

according to the log message in Comment 4, syscall 307 on 64 bit is sendmmsg() (notice the extra 'm'), which would be the first crash I see for this syscall.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

(In reply to Julian Hector [:tedd] [:jhector] from comment #6)
> :gcp, could this be related to Bug 1309205?

That should, if anything, fix this, no? (Aside from bug 1329835 but that won't cause a seccomp violation).

A full crash stack and/or info on the Linux distro etc would be handy.

Comment 8

[Michael Bonfils]

(In reply to Loic from comment #5)
> 
> Michael, do you crash with FF50 too?

With FF50, crash doesn't occur

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Does about:crashes have any relevant reports? Can you link some?

Comment 10

[Michael Bonfils]

Attachment: Crash Report

I hope that it can help you

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: stack_dump.txt

This is the backtrace of that minidump. It's showing the crash happening inside the file broker? I didn't notice the original report included a GDB backtrace, oops.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

The original stack trace is this:

Program received signal SIGSYS, Bad system call.
0x00007fb74d37a4c5 in __GI___xstat (vers=<optimized out>, name=0x7fb733c95640 "/etc/fonts/fonts.conf", buf=0x7fff44d94c50) at ../sysdeps/unix/sysv/linux/wordsize-64/xstat.c:35
35	../sysdeps/unix/sysv/linux/wordsize-64/xstat.c: No such file or directory.
(gdb) bt
#0  0x00007fb74d37a4c5 in __GI___xstat (vers=<optimized out>, name=0x7fb733c95640 "/etc/fonts/fonts.conf", buf=0x7fff44d94c50) at ../sysdeps/unix/sysv/linux/wordsize-64/xstat.c:35
#1  0x00007fb748b9f830 in ?? () from /usr/lib/x86_64-linux-gnu/libfontconfig.so.1
#2  0x00007fb748ba018d in FcConfigUptoDate () from /usr/lib/x86_64-linux-gnu/libfontconfig.so.1
#3  0x00007fb748bac771 in FcInitBringUptoDate () from /usr/lib/x86_64-linux-gnu/libfontconfig.so.1
#4  0x00007fb7429e001e in gfxFcPlatformFontList::CheckFontUpdates(nsITimer*, void*) () from /home/bonfils/Personnel/apps/firefox/libxul.so

Which could also reasonably have the file broker involved. However, why on earth would that be calling sendmmsg? The broker itself does use sendmsg (single m) to send the file paths and handles across.

I checked and sendmmsg was added in 2.6.19. This is 3.16.0. So it's not an issue of a "real" SIGSYS (because the syscall doesn't exist) interfering with our brokering.

Comment 13

[Marcia Knous [:marcia]]

Tracking 53+ for this crash regression as it seems we have good info to use to debug.

Comment 14

[Jed Davis [:jld] (on leave until 08-31)]

From the minidump stack walk:

 1  libresolv-2.19.so + 0xa390
    rsp = 0x00007ffdb114f580   rip = 0x00007f6c53e2b390
    Found by: stack scanning

"Stack scanning" means that it searches stack memory for anything that looks like a valid return address; it's what breakpad falls back on when it doesn't have unwind info and there doesn't seem to be a frame pointer, which is what happened here.  The references to the file broker might not be part of the actual current stack; they could be left over from calls that returned, if the stack later descended past that point and didn't happen to overwrite that word.

However, libresolv seems reasonable for sendmmsg(), and the libnss_dns frames above that would also make sense… and then the stack ends after that.  So that doesn't explain why we'd be trying to make DNS requests.

For comment #12, was that gdb backtrace the result of using md2core?  If so, is that the exact same bit-identical version of all the system libraries involed as on the system that crashed?  If not, that would explain seeing xstat instead of getmmsg.

Comment 15

[Jed Davis [:jld] (on leave until 08-31)]

Attachment: The crash site as an ELF file

I used the same approach as bug 1290896 comment #11 to turn the chunk of libc around the crash into an ELF file with the right virtual addresses.  Disassembled, it certainly looks like 64-bit code for invoking syscall 0x133 == 307.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #14)

> For comment #12, was that gdb backtrace the result of using md2core?  If so,
> is that the exact same bit-identical version of all the system libraries
> involed as on the system that crashed?  If not, that would explain seeing
> xstat instead of getmmsg.

It's from the bug reported himself, on the actual binary.

Now that I write this, I realize that he hit SIGSYS there not because of a crash but simply because that's how the file broker works and what GCC will do if you don't add our scripts/settings. So that backtrace should be discarded, it's simply showing the first sandbox intervention, not a real crash.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

I meant to say "bug reporter" and "what GDB will do", of course.

Comment 18

[Julian Hector [:tedd] [:jhector]]

I looked into this and couldn't reproduce the bug, I set up an Debian virtual box using the same Debian as stated in Comment 1. I will have a look at the get getPrintSettings code and see if I can find anything there.

Comment 19

[Michael Bonfils]

On my Debian, there is few printers installed (via cup), I hope it can help:

 - Name   | Device URI           | Model
 - Xerox1 | socket://xerox1:9100 | Xerox WorkCentre 7328/7335/7345
 - Xerox2 | socket://xerox2:9100 | Xerox WorkCentre 7345 Foomatic/hpijs-pcl5c
 - Dell   | socket://dell:9100   | Dell Laser Printer 3100cn v3015.102 PS

Comment 20

[Julian Hector [:tedd] [:jhector]]

After reading some code, I believe I found the issue, it appears to be related to CUPS. Anyways, starting at

> getPrintSettings

in browser-content.js line 482 as seen in the JS frames from Comment 4.
Which, judging from the name, tries to get the default printer.

It boils down to the function:

> GlobalPrinters::GetDefaultPrinterName

which is defined in ./widget/gtk/nsDeviceContextSpecG.cpp
Assuming we start a fresh initialization and first have to somehow get a default printer, we will end up in the following function:

> GlobalPrinters::InitializeGlobalPrinters

also defined in widget/gtk/nsDeviceContextSpecG.cpp.
Now here comes the interesting part, InitializeGlobalPrinters will call

> nsPSPrinterList::GetPrinterList

in widget/gtk/nsPSPrinters.cpp, which then goes into libcups.so with this line of code:

>  int num_dests = (gCupsShim.mCupsGetDests)(&dests);

mCupsGetDests is a library function from libcups.so (the library is loaded on demand with dlopen() and the symbols retrieved using dlsym()). Now Diving into the libcups code (starting with the 'cupsGetDests' function) one can see that it tries to connect to the default CUPS server and the code eventually leads to the following call:

>     myaddrlist = httpAddrGetList(host, family, service);

Doing a dumb search on the web, I found an attachment from a bug report [1] which showed an interesting backtrace:

> ==13361==    at 0x78AD29F: sendmmsg (in /usr/lib/libc-2.17.so)
> ==13361==    by 0xCB2934F: ??? (in /usr/lib/libresolv-2.17.so)
> ==13361==    by 0xCB26B74: __libc_res_nquery (in /usr/lib/libresolv-2.17.so)
> ==13361==    by 0xCB2715E: ??? (in /usr/lib/libresolv-2.17.so)
> ==13361==    by 0xCB275D2: __libc_res_nsearch (in /usr/lib/libresolv-2.17.so)
> ==13361==    by 0x27CCDA8A: _nss_dns_gethostbyname4_r (in /usr/lib/libnss_dns-2.17.so)
> ==13361==    by 0x7896BEC: gaih_inet (in /usr/lib/libc-2.17.so)
> ==13361==    by 0x789906C: getaddrinfo (in /usr/lib/libc-2.17.so)
> ==13361==    by 0x2568EE01: httpAddrGetList (in /usr/lib/libcups.so.2)

looking at the crash dump, it shows that libcups.so.2 was loaded, so I assume this is likely what causes the crash.
This would also explain why libnss_dns is shown in the stack trace (although the stack trace from the crash dump might not be 100 percent accurate).

This is all code reading, so no debugging, I had a chat with :gcp on IRC and the question is, why is CUPS running in the content process, if print-via-parent is enabled (which it should be on nightly).

[1] https://bugs.freedesktop.org/attachment.cgi?id=83608

Comment 21

[Julian Hector [:tedd] [:jhector]]

Given Comment 19, I think I am on the right track.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

I guess print-via-parent doesn't avoid the problem that the list of printers is queried in content instead of chrome. Nice work.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

I'm going to make this block the sandboxing rollout, as I assume it's going to crash everyone who has actual printers or at least CUPS on their Linux system.

Comment 24

[Julian Hector [:tedd] [:jhector]]

I will give it a shot at fixing this.

Comment 25

[Julian Hector [:tedd] [:jhector]]

Attachment: Move default printer name query to parent [v1]

Probably not the prettiest of a patch, I tried a couple of things, this I got to work. But :bobowen mentioned that he wanted to do a refactor of the printing code which could clean this up.

gcp: What do you think? Since this is blocking sandbox rollout, this patch my be acceptable (given that the problem with CUPS is fixed)

Michael: I don't know if you build from source, but if you do, could you compile with this patch and try to print?

Comment 26

[Michael Bonfils]

(In reply to Julian Hector [:tedd] [:jhector] from comment #25)
> Created attachment 8830715 [details] [diff] [review]
en that the problem with CUPS is fixed)
> 
> Michael: I don't know if you build from source, but if you do, could you
> compile with this patch and try to print?

I'll try to do it but I'm quite busy this week

Comment 27

[Michael Bonfils]

Just tested with 71224049c0b5
- without attached patch, it crashes
- with attached patch, crash doesn't occurs and I've successfully printed http://perdu.com with my cups Printer.

Comment 28

[Julian Hector [:tedd] [:jhector]]

Awesome Michael, thank you very much for testing this, I really do appreciate it.

This fix is not complete yet, :bobowen pointed out that the same problem can occur when window.print() [1] is invoked in javascript.

Some investigation shows that for GTK the printer name is not used, so it may be possible to just ifdef out the query of the printer name, I am currently setting up CUPS to test this further.

[1] http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/dom/base/nsGlobalWindow.cpp#7484

Comment 29

[Julian Hector [:tedd] [:jhector]]

Attachment: window.print() gdb backtrace

I don't have an actual CUPS printer setup up yet, but apparently it is already enough if one sets the following in the /etc/cups/client.conf

> ServerName <some domain>

to trigger this bug, at least this allows to test this to some extend.

Attached is the backtrace when window.print() is called from the developer console.

Comment 30

[Julian Hector [:tedd] [:jhector]]

Frame #19 shows that the code is triggered which was noted by :bobowen

Comment 31

[Julian Hector [:tedd] [:jhector]]

Attachment: Fix window.print to query printer name in parent on Linux [v1]

This patch should fix window.print(), unfortunately I can't get my network printer to work with my CUPS setup, but running window.print() in the web console on some url (I chose icanhazip.com) showed me the print dialog without crash and I could print to file.

@Michael, you have been so kind to test the previous patch with your printer, would you be so kind to test this as well? I would really appreciate it since I am having trouble setting up CUPS. It is sufficient to open some web page, and type window.print() into the web console (which can be found under Tools -> Web Developer -> Web console) and report back if you were able to print a page with your printer.

Comment 32

[Julian Hector [:tedd] [:jhector]]

Never mind the testing, I got my setup locally. Thanks anyways though.

Comment 33

[Julian Hector [:tedd] [:jhector]]

Attachment: Fix window.print to query printer name in parent on Linux [v2]

The CheckForPrinters() function leads to a crash as well.

Comment 34

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8830715 [details] [diff] [review]
Move default printer name query to parent [v1]

Requesting feedback from :bobowen, since he seems more familiar with the code.

:bobowen, who should I request a review from for this? Do you know?

Comment 35

[Julian Hector [:tedd] [:jhector]]

I tested both patches with my setup, I verified that the sendmmsg() call is now happening in the parent (for both code paths)

I was also able to print, using my network printer, however, my CUPS daemon is running locally on my machine, so it doesn't need to resolve the name. But I verified that the name resolving happens in the parent when I replace my ServerName configuration in clients.conf with something that would trigger the crash.

It is probably not the best patch, but that entire printing code could use a refactor and we need to uplift this to aurora, because the Linux sandbox is riding the trains. So hopefully this is acceptable and will be changed in a future refactor.

Comment 36

[Bob Owen (:bobowen)]

Comment on attachment 8830715 [details] [diff] [review]
Move default printer name query to parent [v1]

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

This looks good to me.
mconley is probably the best person to review the JS printing code.

::: toolkit/components/printing/content/printUtils.js
@@ +128,4 @@
>      mm.sendAsyncMessage("Printing:Print", {
>        windowID: aWindowID,
>        simplifiedMode: this._shouldSimplify,
> +      defaultPrinterName: defaultPrinterName,

nit: you can have |defaultPrinterName,| and not duplicate the name now.

Same for preview.

@@ +505,4 @@
>      // listener.
>      let ppBrowser = this._listener.getPrintPreviewBrowser();
>      let mm = ppBrowser.messageManager;
> +    let defaultPrinterName = this.getDefaultPrinterName();

I think in the normal case this might mean we end up getting the default printer name twice, but that probably doesn't matter much.

Comment 37

[Julian Hector [:tedd] [:jhector]]

Thank you very much, should probably also remove the console.log()...

I don't know all the possible code paths, is printWindow and enterPrintPreview always called when either one is called?

Comment 38

[Bob Owen (:bobowen)]

Comment on attachment 8832705 [details] [diff] [review]
Fix window.print to query printer name in parent on Linux [v2]

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

::: dom/base/nsGlobalWindow.cpp
@@ +7490,3 @@
>            printSettings->SetPrinterName(printerName);
>          }
>          printSettingsService->InitPrintSettingsFromPrinter(printerName, printSettings);

Don't we still need to call InitPrintSettingsFromPrinter here for the non-e10s and non-print via parent case?

Actually, maybe the call to GetPrinterName needs to be called if non-e10s and this shouldn't be a #if.

::: layout/printing/nsPrintEngine.cpp
@@ +980,3 @@
>    // Mac doesn't support retrieving a printer list.
> +  // Ignore this on Linux, default printer name should be
> +  // requested on the parent side.

Again, what about the non-e10s case?

::: toolkit/components/printingui/ipc/PrintingParent.cpp
@@ +134,5 @@
> +  // at this point, use the print settings service to get the default printer name.
> +#ifdef MOZ_X11
> +  nsXPIDLString printerName;
> +  settings->GetPrinterName(getter_Copies(printerName));
> +  if (printerName.IsEmpty()) {

My only concern here is whether any add-ons trigger printing on linux and deliberately pass an empty name for some reason, but I don't have a better suggestion.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8830715 [details] [diff] [review]
Move default printer name query to parent [v1]

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

Thanks for the patch! r-'ing for debug logging, but with that removed, I'm fine with this.

::: toolkit/components/printing/content/printUtils.js
@@ +123,5 @@
>     */
>    printWindow(aWindowID, aBrowser) {
>      let mm = aBrowser.messageManager;
> +    let defaultPrinterName = this.getDefaultPrinterName();
> +    console.log(defaultPrinterName);

Need to remove this debug logging.

Comment 40

[Julian Hector [:tedd] [:jhector]]

Attachment: Move default printer name querying

(In reply to Bob Owen (:bobowen) from comment #38)
> Comment on attachment 8832705 [details] [diff] [review]
> Fix window.print to query printer name in parent on Linux [v2]
> 
> Review of attachment 8832705 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/printingui/ipc/PrintingParent.cpp
> @@ +134,5 @@
> > +  // at this point, use the print settings service to get the default printer name.
> > +#ifdef MOZ_X11
> > +  nsXPIDLString printerName;
> > +  settings->GetPrinterName(getter_Copies(printerName));
> > +  if (printerName.IsEmpty()) {
> 
> My only concern here is whether any add-ons trigger printing on linux and
> deliberately pass an empty name for some reason, but I don't have a better
> suggestion.

Thank you for the feedback, I addressed your previously mentioned issues with non-e10s mode, regarding this comment, I don't know a better way either, but the reviewer might so I left it as is.

Could you give this new patch a quick look again, I think it should cover the non-e10s case.

Comment 41

[Julian Hector [:tedd] [:jhector]]

Attachment: Pass default printer name to child

(In reply to Mike Conley (:mconley) from comment #39)
> Comment on attachment 8830715 [details] [diff] [review]
> Move default printer name query to parent [v1]
> 
> Review of attachment 8830715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! r-'ing for debug logging, but with that removed, I'm
> fine with this.
> 
> ::: toolkit/components/printing/content/printUtils.js
> @@ +123,5 @@
> >     */
> >    printWindow(aWindowID, aBrowser) {
> >      let mm = aBrowser.messageManager;
> > +    let defaultPrinterName = this.getDefaultPrinterName();
> > +    console.log(defaultPrinterName);
> 
> Need to remove this debug logging.

Thank you Mike, I removed the debug print and addressed the issues mentioned by Bob in Comment 36, I am not sure if I could just set the r+, so please could you take a look again, thank you.

Comment 42

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8835852 [details] [diff] [review]
Move default printer name querying

Hey David, (since you are currently not accepting review requests) do you know someone who could review this patch?

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Julian Hector [:tedd] [:jhector] from comment #42)
> Hey David, (since you are currently not accepting review requests) do you
> know someone who could review this patch?

:bobowen seems like a good reviewer for this, and is already involved here.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8835856 [details] [diff] [review]
Pass default printer name to child

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

Thanks!

Comment 45

[Bob Owen (:bobowen)]

Comment on attachment 8835852 [details] [diff] [review]
Move default printer name querying

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

/me puts on his (hopefully temporary) printing-peer hat.

r+ with those minor changes, which I think make it a bit more readable.

(This all adds more incentive for me to re-write all this in a more async/e10s/sandboxing friendly way.)

> (In reply to Bob Owen (:bobowen) from comment #38)

> > > +  if (printerName.IsEmpty()) {
> > 
> > My only concern here is whether any add-ons trigger printing on linux and
> > deliberately pass an empty name for some reason, but I don't have a better
> > suggestion.

Actually I think we'll be OK, because from looking at your changes we would have done this in the child in CheckForPrinters anyway.

::: dom/base/nsGlobalWindow.cpp
@@ +7778,5 @@
> +        // The printer name is not needed anywhere else on Linux before it gets to the parent.
> +        // In the parent, we will then query the default printer name if no name is set.
> +        // Unless we are in the parent, we will skip this part.
> +        if (XRE_IsParentProcess()) {
> +#endif

How about:

        bool shouldGetDefaultPrinterName = printerName.IsEmpty();
#ifdef MOZ_X11
        // <commment>
        if (!XRE_IsParentProcess()) {
	  shouldGetDefaultPrinterName = false;
        }
#endif
        if (shouldGetDefaultPrinterName) {

::: layout/printing/nsPrintEngine.cpp
@@ +982,5 @@
>  #else
> +#if defined(MOZ_X11)
> +  // On Linux, default printer name should be requested on the parent side.
> +  // Unless we are in the parent, we ignore this function
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {

if (!XRE_IsParentProcess()) {

::: toolkit/components/printingui/ipc/PrintingParent.cpp
@@ +132,5 @@
> +  // Requesting the default printer name on Linux has been removed in the child,
> +  // because it was causing a sandbox violation (see Bug 1329216).
> +  // If no printer name is set at this point, use the print settings service
> +  // to get the default printer name.
> +#ifdef MOZ_X11

Let's move this and down to before the if (and put the comment inside the #ifdef).
Then remove the GetPrinterName completely below instead of the #ifndef.

Comment 46

[Julian Hector [:tedd] [:jhector]]

Attachment: Move default printer name querying

Carry over r+ from :bobowen, with the additional changes he recommended (also checked with him on irc, for the new patch)

Comment 47

[Julian Hector [:tedd] [:jhector]]

Attachment: Pass default printer name to child

Carry over r+

Created a HG changeset patch.

Comment 48

[Julian Hector [:tedd] [:jhector]]

Attachment: Move default printer name querying

Carry over r+

Created a HG changeset patch.

Comment 49

[Julian Hector [:tedd] [:jhector]]

Try push for build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=121a4936dc3b288521e87cd6008f16c74c0ebcaa

Running test wouldn't make much sense, since they don't trigger the code path. Otherwise we would have found this issue when we enabled seccomp.

Comment 50

[Julian Hector [:tedd] [:jhector]]

Try run in Comment 49

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfc74b156dc
Pass default printer name to child. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f3edf30501
Move default printer name querying. r=bobowen

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3cfc74b156dc
https://hg.mozilla.org/mozilla-central/rev/32f3edf30501

Comment 53

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8836686 [details] [diff] [review]
Pass default printer name to child

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 742434

[User impact if declined]:
Users on Linux won't be able to print pages when using a CUPS server reachable through a hostname. I.e. if the server is running on some.domain and Firefox would need to resolve that hostname.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes, see Comment 27, but only on the platform of the user that reported the bug.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes. The file /etc/cups/client.conf needs to have an entry with "ServerName <some.domain>", <some.domain> needs to be a hostname that needs resolving. This CUPS server then needs to have a default printer configured. For this patch, the print needs to be initiated by pressing ctrl+p (or the equivalent of the platform being tested on) and the default printer name should be given in the print dialog.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
This change has only been tested on one platform (Linux), although it should only affect this platform it may affect others as well. Second, I could only test this to some extend, because I couldn't get a CUPS server setup on my network (due to missing printer). However, I tested it with manual debugging and stepping through to code to see which process is resolving a hostname given in the client.conf, without the patch, the resolving happens in the child and the child process crashes. With the patch, the resolving happens in the parent.

[String changes made/needed]:
None.

Comment 54

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8836687 [details] [diff] [review]
Move default printer name querying

Approval Request Comment
See Comment 53

With a slight difference for the testing path, for this patch the printing needs to be initiated with window.print() in javascript. This can be done using the web console.

Comment 55

[Gerry Chang [:gchang]]

Hi Brindusa, 
Could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 56

[ovidiu boca[:Ovidiu]]

Tested on Mac 10.10, Ubuntu 16.04 and Windows 10 and I can't reproduce this issue on FF Nighlty 54.0a1. Please note that I tried to test this issue with an old FF Nighlty build but I wasn't able to reproduce it. Based on the fact that with the current version of Nighlty 2017-02-15 I can't reproduce it and comment 27 from the reporter, I will mark this as Verified.

Comment 57

[Gerry Chang [:gchang]]

Comment on attachment 8836686 [details] [diff] [review]
Pass default printer name to child

Fix a crash related to printing and was verified. Aurora53+.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd79dc6fb4c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b52b0dcd4b1

Comment 60

[Julian Hector [:tedd] [:jhector]]

Clear 8 month old ni? flag, bug already landed