diff --git a/widget/nsPrinterCUPS.cpp b/widget/nsPrinterCUPS.cpp index 9b5e91485305..6e89face9d5d 100644 --- a/widget/nsPrinterCUPS.cpp +++ b/widget/nsPrinterCUPS.cpp @@ -87,9 +87,8 @@ nsPrinterCUPS::~nsPrinterCUPS() { if (lock->mPrinterInfo) { mShim.cupsFreeDestInfo(lock->mPrinterInfo); } - if (mPrinter) { - mShim.cupsFreeDests(1, mPrinter); - mPrinter = nullptr; + if (lock->mPrinter) { + mShim.cupsFreeDests(1, lock->mPrinter); } } @@ -101,14 +100,16 @@ nsPrinterCUPS::GetName(nsAString& aName) { NS_IMETHODIMP nsPrinterCUPS::GetSystemName(nsAString& aName) { - CopyUTF8toUTF16(MakeStringSpan(mPrinter->name), aName); + PrinterInfoLock lock = mPrinterInfoMutex.Lock(); + CopyUTF8toUTF16(MakeStringSpan(lock->mPrinter->name), aName); return NS_OK; } void nsPrinterCUPS::GetPrinterName(nsAString& aName) const { if (mDisplayName.IsEmpty()) { aName.Truncate(); - CopyUTF8toUTF16(MakeStringSpan(mPrinter->name), aName); + PrinterInfoLock lock = mPrinterInfoMutex.Lock(); + CopyUTF8toUTF16(MakeStringSpan(lock->mPrinter->name), aName); } else { aName = mDisplayName; } @@ -122,7 +123,8 @@ const char* nsPrinterCUPS::LocalizeMediaName(http_t& aConnection, return aMedia.media; } PrinterInfoLock lock = TryEnsurePrinterInfo(); - return mShim.cupsLocalizeDestMedia(&aConnection, mPrinter, lock->mPrinterInfo, + return mShim.cupsLocalizeDestMedia(&aConnection, lock->mPrinter, + lock->mPrinterInfo, CUPS_MEDIA_FLAGS_DEFAULT, &aMedia); } @@ -151,7 +153,8 @@ bool nsPrinterCUPS::SupportsColor() const { bool nsPrinterCUPS::SupportsCollation() const { // We can't depend on cupsGetIntegerOption existing. - const char* const value = FindCUPSOption("printer-type"); + PrinterInfoLock lock = mPrinterInfoMutex.Lock(); + const char* const value = FindCUPSOption(lock, "printer-type"); if (!value) { return false; } @@ -168,7 +171,7 @@ nsPrinterBase::PrinterInfo nsPrinterCUPS::CreatePrinterInfo() const { bool nsPrinterCUPS::Supports(const char* aOption, const char* aValue) const { PrinterInfoLock lock = TryEnsurePrinterInfo(); - return mShim.cupsCheckDestSupported(CUPS_HTTP_DEFAULT, mPrinter, + return mShim.cupsCheckDestSupported(CUPS_HTTP_DEFAULT, lock->mPrinter, lock->mPrinterInfo, aOption, aValue); } @@ -235,13 +238,13 @@ PrintSettingsInitializer nsPrinterCUPS::DefaultSettings( // and the IPP attribute appears to return more accurate defaults on Linux. #ifdef XP_MACOSX hasDefaultMedia = mShim.cupsGetDestMediaDefault( - CUPS_HTTP_DEFAULT, mPrinter, lock->mPrinterInfo, CUPS_MEDIA_FLAGS_DEFAULT, - &media); + CUPS_HTTP_DEFAULT, lock->mPrinter, lock->mPrinterInfo, + CUPS_MEDIA_FLAGS_DEFAULT, &media); #else { ipp_attribute_t* defaultMediaIPP = mShim.cupsFindDestDefault - ? mShim.cupsFindDestDefault(CUPS_HTTP_DEFAULT, mPrinter, + ? mShim.cupsFindDestDefault(CUPS_HTTP_DEFAULT, lock->mPrinter, lock->mPrinterInfo, "media") : nullptr; @@ -251,7 +254,7 @@ PrintSettingsInitializer nsPrinterCUPS::DefaultSettings( hasDefaultMedia = defaultMediaName && mShim.cupsGetDestMediaByName( - CUPS_HTTP_DEFAULT, mPrinter, lock->mPrinterInfo, + CUPS_HTTP_DEFAULT, lock->mPrinter, lock->mPrinterInfo, defaultMediaName, CUPS_MEDIA_FLAGS_DEFAULT, &media); } #endif @@ -277,7 +280,7 @@ PrintSettingsInitializer nsPrinterCUPS::DefaultSettings( }; } - http_t* const connection = aConnection.GetConnection(mPrinter); + http_t* const connection = aConnection.GetConnection(lock->mPrinter); // XXX Do we actually have the guarantee that this is utf-8? NS_ConvertUTF8toUTF16 localizedName{ connection ? LocalizeMediaName(*connection, media) : ""}; @@ -291,8 +294,9 @@ PrintSettingsInitializer nsPrinterCUPS::DefaultSettings( nsTArray nsPrinterCUPS::PaperList( Connection& aConnection) const { - http_t* const connection = aConnection.GetConnection(mPrinter); - PrinterInfoLock lock = TryEnsurePrinterInfo(connection); + PrinterInfoLock lock = mPrinterInfoMutex.Lock(); + http_t* const connection = aConnection.GetConnection(lock->mPrinter); + TryEnsurePrinterInfo(lock, connection); if (!lock->mPrinterInfo) { return {}; @@ -300,8 +304,8 @@ nsTArray nsPrinterCUPS::PaperList( const int paperCount = mShim.cupsGetDestMediaCount ? mShim.cupsGetDestMediaCount( - connection, mPrinter, lock->mPrinterInfo, - CUPS_MEDIA_FLAGS_DEFAULT) + connection, lock->mPrinter, + lock->mPrinterInfo, CUPS_MEDIA_FLAGS_DEFAULT) : 0; nsTArray paperList; nsTHashtable paperSet(std::max(paperCount, 0)); @@ -309,9 +313,9 @@ nsTArray nsPrinterCUPS::PaperList( paperList.SetCapacity(paperCount); for (int i = 0; i < paperCount; ++i) { cups_size_t media; - const int getInfoSucceeded = - mShim.cupsGetDestMediaByIndex(connection, mPrinter, lock->mPrinterInfo, - i, CUPS_MEDIA_FLAGS_DEFAULT, &media); + const int getInfoSucceeded = mShim.cupsGetDestMediaByIndex( + connection, lock->mPrinter, lock->mPrinterInfo, i, + CUPS_MEDIA_FLAGS_DEFAULT, &media); if (!getInfoSucceeded || !paperSet.EnsureInserted(media.media)) { continue; @@ -334,22 +338,21 @@ nsTArray nsPrinterCUPS::PaperList( return paperList; } -nsPrinterCUPS::PrinterInfoLock nsPrinterCUPS::TryEnsurePrinterInfo( - http_t* const aConnection) const { - PrinterInfoLock lock = mPrinterInfoMutex.Lock(); - if (lock->mPrinterInfo || - (aConnection == CUPS_HTTP_DEFAULT ? lock->mTriedInitWithDefault - : lock->mTriedInitWithConnection)) { - return lock; +void nsPrinterCUPS::TryEnsurePrinterInfo(PrinterInfoLock& aLock, + http_t* const aConnection) const { + if (aLock->mPrinterInfo || + (aConnection == CUPS_HTTP_DEFAULT ? aLock->mTriedInitWithDefault + : aLock->mTriedInitWithConnection)) { + return; } if (aConnection == CUPS_HTTP_DEFAULT) { - lock->mTriedInitWithDefault = true; + aLock->mTriedInitWithDefault = true; } else { - lock->mTriedInitWithConnection = true; + aLock->mTriedInitWithConnection = true; } - MOZ_ASSERT(mPrinter); + MOZ_ASSERT(aLock->mPrinter); // httpGetAddress was only added in CUPS 2.0, and some systems still use // CUPS 1.7. @@ -427,9 +430,10 @@ nsPrinterCUPS::PrinterInfoLock nsPrinterCUPS::TryEnsurePrinterInfo( // Match the conditional at // https://github.com/apple/cups/blob/23c45db76a8520fd6c3b1d9164dbe312f1ab1481/cups/dest.c#L1144 // but if device-uri is null do not call into CUPS. - if ((cupsDestDeviceFlag || !FindCUPSOption("printer-uri-supported")) && - !FindCUPSOption("device-uri")) { - return lock; + if ((cupsDestDeviceFlag || + !FindCUPSOption(aLock, "printer-uri-supported")) && + !FindCUPSOption(aLock, "device-uri")) { + return; } } } @@ -438,13 +442,12 @@ nsPrinterCUPS::PrinterInfoLock nsPrinterCUPS::TryEnsurePrinterInfo( // All CUPS calls that take the printer info do null-checks internally, so we // can fetch this info and only worry about the result of the later CUPS // functions. - lock->mPrinterInfo = mShim.cupsCopyDestInfo(aConnection, mPrinter); + aLock->mPrinterInfo = mShim.cupsCopyDestInfo(aConnection, aLock->mPrinter); // Even if we failed to fetch printer info, it is still possible we can talk // to the print server and get its CUPS version. - FetchCUPSVersionForPrinter(mShim, mPrinter, lock->mCUPSMajor, - lock->mCUPSMinor, lock->mCUPSPatch); - return lock; + FetchCUPSVersionForPrinter(mShim, aLock->mPrinter, aLock->mCUPSMajor, + aLock->mCUPSMinor, aLock->mCUPSPatch); } void nsPrinterCUPS::ForEachExtraMonochromeSetting( diff --git a/widget/nsPrinterCUPS.h b/widget/nsPrinterCUPS.h index 3ee51b8d968d..3ecf1cb982cf 100644 --- a/widget/nsPrinterCUPS.h +++ b/widget/nsPrinterCUPS.h @@ -42,18 +42,17 @@ class nsPrinterCUPS final : public nsPrinterBase { : nsPrinterBase(aArray), mShim(aShim), mDisplayName(std::move(aDisplayName)), - mPrinter(aPrinter), - mPrinterInfoMutex("nsPrinterCUPS::mPrinterInfoMutex") {} + mPrinterInfoMutex(CUPSPrinterInfo{aPrinter}, + "nsPrinterCUPS::mPrinterInfoMutex") {} static void ForEachExtraMonochromeSetting( mozilla::FunctionRef); - inline const char* FindCUPSOption(const char* name) const { - return mShim.cupsGetOption(name, mPrinter->num_options, mPrinter->options); - } - private: struct CUPSPrinterInfo { + explicit constexpr CUPSPrinterInfo(cups_dest_t* aPrinter) + : mPrinter(aPrinter) {} + cups_dest_t* mPrinter; cups_dinfo_t* mPrinterInfo = nullptr; uint64_t mCUPSMajor = 0; uint64_t mCUPSMinor = 0; @@ -63,9 +62,17 @@ class nsPrinterCUPS final : public nsPrinterBase { bool mTriedInitWithDefault = false; // Whether we have attempted to fetch mPrinterInfo with a connection. bool mTriedInitWithConnection = false; - CUPSPrinterInfo() = default; + CUPSPrinterInfo() = delete; CUPSPrinterInfo(const CUPSPrinterInfo&) = delete; - CUPSPrinterInfo(CUPSPrinterInfo&&) = delete; + CUPSPrinterInfo(CUPSPrinterInfo&& aOther) + : mPrinter(aOther.mPrinter), + mPrinterInfo(aOther.mPrinterInfo), + mCUPSMajor(aOther.mCUPSMajor), + mCUPSMinor(aOther.mCUPSMinor), + mCUPSPatch(aOther.mCUPSPatch) { + aOther.mPrinter = nullptr; + aOther.mPrinterInfo = nullptr; + } }; using PrinterInfoMutex = @@ -91,6 +98,11 @@ class nsPrinterCUPS final : public nsPrinterBase { bool IsCUPSVersionAtLeast(uint64_t aCUPSMajor, uint64_t aCUPSMinor, uint64_t aCUPSPatch) const; + const char* FindCUPSOption(PrinterInfoLock& aLock, const char* name) const { + const cups_dest_t* const printer = aLock->mPrinter; + return mShim.cupsGetOption(name, printer->num_options, printer->options); + } + class Connection { public: http_t* GetConnection(cups_dest_t* aDest); @@ -114,11 +126,24 @@ class nsPrinterCUPS final : public nsPrinterBase { * on older versions of Ubuntu (18 and below). */ PrinterInfoLock TryEnsurePrinterInfo( - http_t* const aConnection = CUPS_HTTP_DEFAULT) const; + http_t* const aConnection = CUPS_HTTP_DEFAULT) const { + PrinterInfoLock lock = mPrinterInfoMutex.Lock(); + TryEnsurePrinterInfo(lock, aConnection); + return lock; + } + + /** + * TryEnsurePrinterInfo that uses a caller-provided PrinterInfoLock. + * + * This can be used to avoid unnecessarily redundant locking of + * mPrinterInfoLock when getting a connection through + * Connection::GetConnection and then passing that into TryEnsurePrinterInfo. + */ + void TryEnsurePrinterInfo(PrinterInfoLock& aLock, + http_t* const aConnection) const; const nsCUPSShim& mShim; nsString mDisplayName; - cups_dest_t* mPrinter; mutable PrinterInfoMutex mPrinterInfoMutex; };