1662038 - nsIPrintSettings::SetPrinterName is broken on macOS

Status: VERIFIED FIXED

(Core :: Printing: Setup, defect, P1)

Description

[Jonathan Watt [:jwatt]]

nsPrintSettingsX is a mess - right now we try to wrap an NSPrintInfo obect but fail in some places. One are this manifests is in setting the printer name.

Currently there is no nsPrintSettingsX::Get/SetPrinterName override. Therefore we're just relying on the base class methods, which simply sit mPrinterName in the base class, leaving the NSPrintInfo untouched. This is an issue since nsPrintSettingsServiceX::SerializeToPrintDataParent gets the printer name from the NSPrintInfo when it serializes it for transfering between processes.

I've tried creating nsPrintSettingsX::Get/SetPrinterName overrides to fix this in two different ways:

By setting the NSPrintPrinterName dictionary key:

NS_IMETHODIMP nsPrintSettingsX::GetPrinterName(nsAString& aName) {
  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

  if (XRE_IsParentProcess()) {
    NSDictionary* dict = [mPrintInfo dictionary];
    NSString* printerName = [dict objectForKey:NSPrintPrinterName];
    if (printerName) {
      nsCocoaUtils::GetStringForNSString(printerName, aName);
    } else {
      aName.Truncate();
    }
  } else {
    nsPrintSettings::GetPrinterName(aName);
  }
  return NS_OK;

  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
}

NS_IMETHODIMP nsPrintSettingsX::SetPrinterName(const nsAString& aName) {
  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

  if (XRE_IsParentProcess()) {
    NSMutableDictionary* dict = [mPrintInfo printSettings];
    [dict setObject:[NSString stringWithString:nsCocoaUtils::ToNSString(aName)]
          forKey:NSPrintPrinterName];
  } else {
    nsPrintSettings::SetPrinterName(aName);
  }
  return NS_OK;

  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
}

This does not appear to work, so I tried at alternative method to replace [mPrintInfo printer]:

NS_IMETHODIMP nsPrintSettingsX::GetPrinterName(nsAString& aName) {
  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

  if (XRE_IsParentProcess()) {
    NSString* name = [[mPrintInfo printer] name];
    if (name) {
      nsCocoaUtils::GetStringForNSString(name, aName);
    } else {
      aName.Truncate();
    }
  } else {
    nsPrintSettings::GetPrinterName(aName);
  }
  return NS_OK;

  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
}

NS_IMETHODIMP nsPrintSettingsX::SetPrinterName(const nsAString& aName) {
  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

  if (XRE_IsParentProcess()) {
    NSString* name = nsCocoaUtils::ToNSString(aName);
    [mPrintInfo setPrinter:[NSPrinter printerWithName:name]];
  } else {
    nsPrintSettings::SetPrinterName(aName);
  }
  return NS_OK;

  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
}

That does appear to work, but there's one outstanding issue. Currently the frontend code creates nsIPrintSettings objects for a pseudo-printer called "Mozilla Save as PDF". That will currently be silently rejected and the new NSPrinter will be for the last successfully created NSPrinter (or something like that). The result is that after serializing the settings in the parent, sending them to the child for nsPrintJob, and then deserializing them, the printer name in the child is for the last selected physical printer.

Comment 1

[Jonathan Watt [:jwatt]]

In other words we don't have a direct way to detect that the selected printer is our pseudo-"Mozilla Save as PDF" printer in the content process on macOS.

Another more serious consequence of this bug (I think, but can't test) is that on macOS the printer associated with NSPrintInfo objects can only be changed via the system dialog. The new printing UI may look like it's allowing the name of the printer to be changed, but if the NSPrintInfo is stuck pointing to a different printer, then the actual print output is going to be sent to the wrong printer. This requires two physical printers to confirm.

Comment 2

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Watt [:jwatt] from comment #1)

In other words we don't have a direct way to detect that the selected printer is our pseudo-"Mozilla Save as PDF" printer in the content process on macOS.

Another more serious consequence of this bug (I think, but can't test) is that on macOS the printer associated with NSPrintInfo objects can only be changed via the system dialog. The new printing UI may look like it's allowing the name of the printer to be changed, but if the NSPrintInfo is stuck pointing to a different printer, then the actual print output is going to be sent to the wrong printer. This requires two physical printers to confirm.

I can confirm this: I rounded up two physical printers, but my output always goes to the one that is selected as default in the system settings, ignoring my attempt to choose the other in our Destination popup.

Comment 3

[Jonathan Kew [:jfkthame]]

I can also confirm that with the "alternative method" impl of Get/SetPrinterName from comment 0, I can successfully print to either of my printers from the new UI, by just selecting in the Destination popup and clicking Print.

However, I can no longer select the Save to PDF destination at all.

Comment 4

[Jonathan Kew [:jfkthame]]

In brief testing, something like this appears to work for me (although it's a painfully ugly hack):

+NS_IMETHODIMP nsPrintSettingsX::GetPrinterName(nsAString& aName) {
+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
+
+  nsPrintSettings::GetPrinterName(aName);
+  if (XRE_IsParentProcess()) {
+    if (!aName.Equals(u"Mozilla Save to PDF"_ns)) {
+      NSString* name = [[mPrintInfo printer] name];
+      if (name) {
+        nsCocoaUtils::GetStringForNSString(name, aName);
+      } else {
+        aName.Truncate();
+      }
+    }
+  }
+  return NS_OK;
+
+  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
+}
+
+NS_IMETHODIMP nsPrintSettingsX::SetPrinterName(const nsAString& aName) {
+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
+
+  nsPrintSettings::SetPrinterName(aName);
+  if (XRE_IsParentProcess() && !aName.Equals(u"Mozilla Save to PDF"_ns)) {
+    NSString* name = nsCocoaUtils::ToNSString(aName);
+    [mPrintInfo setPrinter:[NSPrinter printerWithName:name]];
+  }
+  return NS_OK;
+
+  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
+}

Comment 5

[Jonathan Kew [:jfkthame]]

Stealing this from jwatt, as I'm better placed to test it. :)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1662038 - Fix printer-name handling on macOS so it doesn't fail on the fake "Save to PDF" destination. r=jwatt

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3b1f72de9dd
Fix printer-name handling on macOS so it doesn't fail on the fake "Save to PDF" destination. r=jwatt

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 9173171 [details]
Bug 1662038 - Fix printer-name handling on macOS so it doesn't fail on the fake "Save to PDF" destination. r=jwatt

Beta/Release Uplift Approval Request

• User impact if declined: macOS users cannot control which printer is used (the Destination control is ignored)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: On a macOS machine with multiple printers installed, try printing to different destinations from the Print UI (without changing the default printer in the OS settings).
  Also test the same thing via the "Print using the system dialog" route.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Provide missing SetPrinterName override so that we properly update the macos printinfo record.
• String changes made/needed:

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d3b1f72de9dd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173171 [details]
Bug 1662038 - Fix printer-name handling on macOS so it doesn't fail on the fake "Save to PDF" destination. r=jwatt

Approved for 81.0b5.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5812ebd9ca62

Comment 12

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed on macOS 10.15.6. Verified using 82.0a1 (2020-09-09) and 81.0b8.