1090439 - PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

In non-e10s for Windows and Linux (and probably OS X), when the print settings dialog is open, the window is modal and spins its own event loop which feeds the Gecko main loop.

Bug 1082579 introduced a mechanism to open those dialogs from the child process over IPC - but those IPC calls are synchronous from the child. So the child is blocked while the user works with the print settings dialog, for example.

This is no good - especially since we've only got a single content process. Consider if a user has multiple windows open and opens the print settings dialog in one - this means that the other window will no longer have responsive content.

We should run our own event loop in nsPrintingPromptServiceProxy for as long as the print settings dialog is open, as this is what platform already does in the non-e10s case.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

smaug wrote:

"Couldn't you create a PrintDialog protocol, and then in __delete__ continue. While you're waiting __delete__ you'd call NS_ProcessNextEvent"

Comment 2

[Jim Mathies [:jimm]]

Hey Mike, curious, did you look into creating a new async api and calling that from layout? I'm curious what might have prevented that. We did something similar a couple years ago with file pickers.

http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#619

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I was under a bit of time pressure to get the printing stuff done, so I don't think I explored that avenue, I'm afraid. Definitely could be worth poking at.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey jimm,

I'm looking to grab more M5's, and I've recently been down in Printing, so it's kinda swapped in. Feel like giving this one up to me?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> Hey jimm,
> 
> I'm looking to grab more M5's, and I've recently been down in Printing, so
> it's kinda swapped in. Feel like giving this one up to me?

Sure, all yours.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8572761 [details] [diff] [review]
PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Hey jimm, can I get a sanity check here?

This patch will make the showing of the print settings dialog not hang the content process - instead, the content process will spin that event loop waiting for a response from the parent.

I'm using IDs and a LinkedList of structs to manage the case where we might have multiple windows getting the print dialog opened in them. I really don't know if I've addressed this properly, but I think my theory is that even if print dialogs are OK'd in a different order than they're opened (which Windows allows us to do it seems), then we'll still dispatch the right results to the right callers to ShowPrintDialog.

However, nested event loops are evil, and maybe I'm not seeing a corner-case here. My testing of whether or not this approach even works is inconclusive.

If I open up two e10s windows on Windows 7 (A and B), open the print dialog in A, then in B, then press "OK" in A, and then press "OK" in B, I only end up seeing B going to my PDF printer. This is, however, the same behaviour that we seem to have on single-process as well without my patch.

So, kinda dubious. :/ Open to feedback.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Oh, and I didn't make opening the progress dialog async, since we don't block on the lifetime on the window. We just block the content process on the spawning of the dialog, which is probably OK for now (we do the same thing for new popup windows as well).

Comment 14

[Jim Mathies [:jimm]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #12)
> Comment on attachment 8572761 [details] [diff] [review]
> PPrinting calls from child to parent via ShowProgress and ShowPrintDialog
> should not be sync. r=?
> 
> Hey jimm, can I get a sanity check here?

Generally this looks ok to me. I was thinking about the loop in the content process a bit. We have problems with these in the parent process but in the child where the are no native events, I think we should be fine.
 
> This patch will make the showing of the print settings dialog not hang the
> content process - instead, the content process will spin that event loop
> waiting for a response from the parent.
> 
> I'm using IDs and a LinkedList of structs to manage the case where we might
> have multiple windows getting the print dialog opened in them. I really
> don't know if I've addressed this properly, but I think my theory is that
> even if print dialogs are OK'd in a different order than they're opened
> (which Windows allows us to do it seems), then we'll still dispatch the
> right results to the right callers to ShowPrintDialog.
> 
> However, nested event loops are evil, and maybe I'm not seeing a corner-case
> here. My testing of whether or not this approach even works is inconclusive.
> 
> If I open up two e10s windows on Windows 7 (A and B), open the print dialog
> in A, then in B, then press "OK" in A, and then press "OK" in B, I only end
> up seeing B going to my PDF printer. This is, however, the same behaviour
> that we seem to have on single-process as well without my patch.

When I test this in non-e10s using a builtin software 'wps printer', I get a save as dialog for each, one after the other.

Applying this and will test it a bit.

Comment 15

[Jim Mathies [:jimm]]

Hmm, this conflicted heavily, I pulled yesterday. Please post an updated wip when you get a chance.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: bz://1090439/mconley

/r/4795 - Bug 1090439 - PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Pull down this commit:

hg pull review -r 4df3e1a9f61317f2bbe79e7ea491f7eb5c796808

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Yeah, this stuff is based on the work in bug 1088070, which was supposed to have landed, but has bounced. I'll land it again tonight once the tree re-opens.

Until then, you can test out this patch via:

hg pull http://reviewboard-hg.mozilla.org/gecko -r 4df3e1a9f61317f2bbe79e7ea491f7eb5c796808
hg update -r 4df3e1a9f61317f2bbe79e7ea491f7eb5c796808

and build from there.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

I've changed tack here, and decided to try smaug's idea from comment 1. Instead of manually managing the potentiality for multiple messages with a LinkedList and IDs to differentiate between different nested loop scopes, I use the IPC infrastructure and a new PPrintSettingsDialog protocol.

The child opens the protocol to the parent, and spins an event loop. When the parent gets a result from the print settings dialog, it deletes the protocol (passing along the results from the dialog). When the child protocol implementation notices the delete, the nested event loop exits, and the results are returned to the caller.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8574022 [details] [diff] [review]
PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

>+PrintingParent::ShowPrintDialog(PBrowserParent* parent,
>+                                const PrintData& data,
>+                                PrintData* result)
Could you use Mozilla coding style for the parameters


>+PrintingParent::RecvShowPrintDialog(PPrintSettingsDialogParent* dialog,
>+                                    PBrowserParent* parent,
>+                                    const PrintData& data)
ditto

>+PrintingParent::DeallocPPrintSettingsDialogParent(PPrintSettingsDialogParent* doomed)
ditto

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Thanks!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48634beaa636

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/48634beaa636

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Filed bug 1140929 about refactoring nsPrintEngine to call those blocking prompt functions off of an event.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1090439.html

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1090439 - PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?