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)

enhancement

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
Component: Widget: Gtk → Security: Process Sandboxing
Whiteboard: sb?
Is this about Firefox's content sandboxing, or running the entire browser under flatpak, or both?
Flags: needinfo?(jhorak)
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)
Priority: -- → P3
Whiteboard: sb?
Is still an issue with the Flatpak?
Yes, non trivial changes needs to be done to have this fixed. Still on my radar.
Comment on attachment 8960096 [details]
Bug 1411589 - Export ShouldUseFlatpakPortal();

https://reviewboard.mozilla.org/r/228890/#review235496
Attachment #8960096 - Flags: review?(stransky) → 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 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 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 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.
Attachment #8960097 - Flags: review?(jmathies)
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 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 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 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 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 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 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 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
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)
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)
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)
Attachment #8960098 - Attachment is obsolete: true
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+
Keywords: checkin-needed
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
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)
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
Depends on: 1456183
You need to log in before you can comment on or make changes to this bug.