Closed
Bug 1411589
Opened 7 years ago
Closed 6 years ago
[flatpak] No printer in the list of printers
Categories
(Core :: Widget: Gtk, enhancement, P3)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jhorak, Assigned: jhorak)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
The sandboxed application does not have direct access to the printers which current implementation requires. We need to use GtkPrintOperation for this instead: https://developer.gnome.org/gtk3/stable/gtk3-High-level-Printing-API.html
Updated•7 years ago
|
Component: Widget: Gtk → Security: Process Sandboxing
Whiteboard: sb?
Comment 1•7 years ago
|
||
Is this about Firefox's content sandboxing, or running the entire browser under flatpak, or both?
Flags: needinfo?(jhorak)
Assignee | ||
Comment 2•7 years ago
|
||
It has nothing to do with Firefox sandbox implementation. It's because the flatpak deny direct access to the printers. I'm going to look into it how this can be fixed. Sorry for not being clear.
Assignee: nobody → jhorak
Component: Security: Process Sandboxing → Widget: Gtk
Flags: needinfo?(jhorak)
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: sb?
Comment 3•6 years ago
|
||
Is still an issue with the Flatpak?
Assignee | ||
Comment 4•6 years ago
|
||
Yes, non trivial changes needs to be done to have this fixed. Still on my radar.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8960096 [details] Bug 1411589 - Export ShouldUseFlatpakPortal(); https://reviewboard.mozilla.org/r/228890/#review235496
Attachment #8960096 -
Flags: review?(stransky) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8960098 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228894/#review235508 ::: layout/base/nsDocumentViewer.cpp:4507 (Diff revision 1) > // nsPrintJob::PrintPreview(), mPrintJob is already nullptr here. > // So, the following clean up does nothing in such case. > // (Do we need some of this for that case?) > if (mPrintJob) { > RefPtr<nsPrintJob> printJob = mPrintJob; > + This needs to be reviewed by layout peer. ::: widget/gtk/mozgtk/mozgtk.c:619 (Diff revision 1) > STUB(gtk_color_chooser_dialog_get_type) > STUB(gtk_color_chooser_get_type) > STUB(gtk_color_chooser_set_rgba) > STUB(gtk_color_chooser_get_rgba) > STUB(gtk_color_chooser_set_use_alpha) > +STUB(gdk_threads_leave) Those two are marked as obsoleted. We need to make sure we call Gtk code from main thread. ::: widget/gtk/nsPrintDialogGTK.cpp:981 (Diff revision 1) > + // Check for the flatpak portal first > + nsCOMPtr<nsIGIOService> giovfs = > + do_GetService(NS_GIOSERVICE_CONTRACTID); > + bool shouldUsePortal; > + giovfs->ShouldUseFlatpakPortal(&shouldUsePortal); > + if (shouldUsePortal && !gtk_check_version(3, 22, 0)) { Please use == nullptr here isntead of !gtk_check_version()
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8960098 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228894/#review235516 This patch does not build. Please fix that and also separate the layout part for layout peer review.
Attachment #8960098 -
Flags: review?(stransky) → review-
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8960098 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228894/#review235740 ::: widget/gtk/nsPrintDialogGTK.cpp:904 (Diff revision 1) > + return GTK_PRINT_OPERATION_RESULT_ERROR; > + } > + gdk_threads_leave(); > + // Calling g_main_loop_run stops current code until g_main_loop_quit is called > + g_main_loop_run(mLoop); > + gdk_threads_enter(); gdk_threads_leave/enter here does not look right. Why is it here? (beside it's deprecated).
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8960098 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228894/#review235742 ::: widget/gtk/nsPrintDialogGTK.cpp:535 (Diff revision 1) > +typedef void (*GtkWindowHandleExported) (GtkWindow *window, > + const char *handle, > + gpointer user_data); > +#ifdef MOZ_WAYLAND > +typedef struct { > + GtkWindow *window; Don't mix 8/2 spaces indentation. Use just 4 spaces everywhere. ::: widget/gtk/nsPrintDialogGTK.cpp:556 (Diff revision 1) > + g_free (handle_str); > +} > +#endif > + > +static void > +free_exported_data (gpointer data) This is not needed. Just pass g_free to s_gdk_wayland_window_export_handle() directly. ::: widget/gtk/nsPrintDialogGTK.cpp:583 (Diff revision 1) > + callback (window, handle_str, user_data); > + g_free (handle_str); > + return TRUE; > + } > +#ifdef MOZ_WAYLAND > + if (GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (GTK_WIDGET (window)))) Use just else instead of "GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (GTK_WIDGET (window)))" check. We don't support other non-x11 displays right now. nit: mozilla style is without space between function name and braces (). ::: widget/gtk/nsPrintDialogGTK.cpp:593 (Diff revision 1) > + data = g_new0 (WaylandWindowHandleExportedData, 1); > + data->window = window; > + data->callback = callback; > + data->user_data = user_data; > + > + static auto s_gdk_wayland_window_export_handle = 80 chars/column here. ::: widget/gtk/nsPrintDialogGTK.cpp:596 (Diff revision 1) > + data->user_data = user_data; > + > + static auto s_gdk_wayland_window_export_handle = > + reinterpret_cast<gboolean (*)(GdkWindow*, GdkWaylandWindowExported, gpointer, GDestroyNotify)> > + (dlsym(RTLD_DEFAULT, "gdk_wayland_window_export_handle")); > + if (!s_gdk_wayland_window_export_handle || You're missing matching gdk_wayland_window_unexport_handle() call to release the window handle. see gdkwindow-wayland.c for details. ::: widget/gtk/nsPrintDialogGTK.cpp:601 (Diff revision 1) > + if (!s_gdk_wayland_window_export_handle || > + !s_gdk_wayland_window_export_handle(gdk_window, > + wayland_window_handle_exported, > + data, free_exported_data)) > + { > + return FALSE; You need to free data explicitly here (see gtkwindow.c/gtk_window_export_handle(). ::: widget/gtk/nsPrintDialogGTK.cpp:603 (Diff revision 1) > + wayland_window_handle_exported, > + data, free_exported_data)) > + { > + return FALSE; > + } > + else "} else {" and brace { after if () is mozilla style.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8960097 -
Flags: review?(jmathies)
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8965607 [details] Bug 1411589 - Implement printing support for the flatpak portal, https://reviewboard.mozilla.org/r/234444/#review240102 ::: widget/gtk/nsPrintDialogGTK.cpp:988 (Diff revision 1) > + > +nsFlatpakPrintPortal::~nsFlatpakPrintPortal() { > + if (mProxy) > + g_object_unref(mProxy); > + if (mLoop) > + g_main_loop_quit (mLoop); niw: Please don't mix gtk+ style and mozilla style - space between function name and brace. Use only mozilla style here.
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8965607 [details] Bug 1411589 - Implement printing support for the flatpak portal, https://reviewboard.mozilla.org/r/234444/#review240104 ::: widget/gtk/nsPrintDialogGTK.cpp:669 (Diff revision 1) > + uint64_t aParentWinId): > + mPrintAndPageSettings(aPrintSettings), > + mProxy(nullptr), > + mLoop(nullptr), > + mParentWinId(aParentWinId), > + mNeedToUnexportHandle(false) { brace on next line
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8965607 [details] Bug 1411589 - Implement printing support for the flatpak portal, https://reviewboard.mozilla.org/r/234444/#review240108 ::: widget/gtk/nsPrintDialogGTK.cpp:738 (Diff revision 1) > + GtkPageSetup* pageSetup = mPrintAndPageSettings->GetGtkPageSetup(); > + > +#ifdef MOZ_WAYLAND > + // We arrived there from OnWindowExportHandleDone and we need to remember if > + // unexport of window handle after dialog closing is needed. > + mNeedToUnexportHandle = true; You need to unexport the handle only when you're running on Wayland display. not for all wayland enabled builds. I suggest to remove the mNeedToUnexportHandle and test actual display at nsFlatpakPrintPortal::FinalizePrintDialog. ::: widget/gtk/nsPrintDialogGTK.cpp:901 (Diff revision 1) > + */ > +void > +nsFlatpakPrintPortal::FinalizePrintDialog(GtkWindow *aGtkWindow) > +{ > + if (mNeedToUnexportHandle) { > +#ifdef MOZ_WAYLAND Just test actual display here (IS_WAYLAND_DISPLAY...()) instead of the variable.
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8965607 [details] Bug 1411589 - Implement printing support for the flatpak portal, https://reviewboard.mozilla.org/r/234444/#review240114 ::: widget/gtk/nsPrintDialogGTK.cpp:898 (Diff revision 1) > + * Free resources we've allocated in order to show print dialog. > + * > + * @parm aGtkWindow a window which we used as a parent for the dialog > + */ > +void > +nsFlatpakPrintPortal::FinalizePrintDialog(GtkWindow *aGtkWindow) Given a fact this is wayland only function I'd rather remove it and move this code to GetResults() after g_main_loop_run(mLoop);
Attachment #8965607 -
Flags: review?(stransky)
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8960098 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228894/#review240250 r=me with the crashing issue below fixed. ::: layout/base/nsDocumentViewer.cpp:4519 (Diff revision 2) > } else { > mPrintJob = nullptr; > printJob->Destroy(); > + > + // Notify flatpak printing portal that file is completely written > + nsCOMPtr<nsIGIOService> giovfs = giovfs will be null on non-Linux systems, right? ::: layout/base/nsDocumentViewer.cpp:4522 (Diff revision 2) > + > + // Notify flatpak printing portal that file is completely written > + nsCOMPtr<nsIGIOService> giovfs = > + do_GetService(NS_GIOSERVICE_CONTRACTID); > + bool shouldUsePortal; > + giovfs->ShouldUseFlatpakPortal(&shouldUsePortal); And then this will crash. You probably want to set shouldUsePortal false if giovfs is null. Also if ShouldUseFlatpakPortal returns error.
Attachment #8960098 -
Flags: review?(bzbarsky) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8960097 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228892/#review240644 ::: dom/ipc/ContentParent.cpp:5590 (Diff revision 1) > +{ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + // Pass filename of the print for checking that observer is matching > + // finished print operation > + obs->NotifyObservers(nullptr, "print-to-file-finished", aFilename.get()); I don't see a reference to 'print-to-file-finished' in our code.. who consumes aFilename?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8965607 [details] Bug 1411589 - Implement printing support for the flatpak portal, https://reviewboard.mozilla.org/r/234444/#review240906 With the mParentWinId try failure fixed.
Attachment #8965607 -
Flags: review?(stransky) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8960097 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228892/#review240870 ::: dom/ipc/ContentParent.cpp:5590 (Diff revision 1) > +{ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + // Pass filename of the print for checking that observer is matching > + // finished print operation > + obs->NotifyObservers(nullptr, "print-to-file-finished", aFilename.get()); The nsFlatpakPrintPortal::Observe consumes aFilename, it's implemented in this changeset: https://reviewboard.mozilla.org/r/234444/diff/1#index_header
Comment 31•6 years ago
|
||
It looks to me like this work opens up a possible host of security issues. Content processes currently have temp directory write perms, and print-to-file-finished hands literally any file on the system, including content generated temp files, directly to a native print driver. I'm no Linux expert though, I think Jed's input here might be valuable. Jed, please look this over to see if my concerns are in fact valid.
Flags: needinfo?(jld)
Comment 32•6 years ago
|
||
For what it's worth, this looks like it won't be active (i.e., the observer will never be attached) if not running as a flatpak. If I'm following this correctly: in response to the print dialog, in the parent process, the print job is set up to print to file in PDF; and then something in the parent process needs to be notified with the filename when it's done, so it can open it and pass it to the dbus service. As I understand it the printing itself is remoted via PRemotePrintJob, and the parent process is what writes that file. Is there some way to hook in to the end of printing in the parent process instead? Maybe RemotePrintJobParent::RecvFinalizePrint?
Flags: needinfo?(jld) → needinfo?(jhorak)
Assignee | ||
Comment 33•6 years ago
|
||
It looks like I can use nsDeviceContextSpecGTK::EndDocument (which is supposed to be called from RecvFinalizePrint) to notify the observers and avoid IPC and layout changes completely. Thanks for the hint.
Flags: needinfo?(jhorak)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8960098 -
Attachment is obsolete: true
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8960097 [details] Bug 1411589 - Notify flatpak print portal that print to file has finished, https://reviewboard.mozilla.org/r/228892/#review242644
Attachment #8960097 -
Flags: review?(stransky) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 38•6 years ago
|
||
This bug has some jobs failed on try: 1665 jobs success,67 jobs testfailed,2 jobs exception,35 jobs retry,7 jobs busted,1 jobs running https://treeherder.mozilla.org/#/jobs?repo=try&revision=e539fb8771374f6eb315c3126083b206c1598b95 Can you please take a look?
Flags: needinfo?(jhorak)
Keywords: checkin-needed
Assignee | ||
Comment 39•6 years ago
|
||
I have rerun the try with following results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c215f2dbcb6cb1f106f3ebf00b3e58759cbe66&selectedJob=174071113 I couldn't find correlation between test results and my code changes.
Flags: needinfo?(jhorak)
Comment 40•6 years ago
|
||
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/e2d6c52a232e Export ShouldUseFlatpakPortal(); r=stransky https://hg.mozilla.org/integration/autoland/rev/c470cb1ec203 Implement printing support for the flatpak portal, r=stransky https://hg.mozilla.org/integration/autoland/rev/a6bd62970a9f Notify flatpak print portal that print to file has finished, r=stransky
Comment 41•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2d6c52a232e https://hg.mozilla.org/mozilla-central/rev/c470cb1ec203 https://hg.mozilla.org/mozilla-central/rev/a6bd62970a9f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox58:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•