Closed Bug 1662038 Opened 1 year ago Closed 1 year ago

nsIPrintSettings::SetPrinterName is broken on macOS

Categories

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

defect

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox81 --- verified
firefox82 --- verified

People

(Reporter: jwatt, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Whiteboard: [print2020_v81][old-ui-])

Attachments

(1 file)

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.

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.

Status: NEW → ASSIGNED

(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.

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.

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;
+}

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

Assignee: jwatt → jfkthame
Pushed by jkew@mozilla.com:
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 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:
Attachment #9173171 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

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.

Attachment #9173171 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1740421
You need to log in before you can comment on or make changes to this bug.