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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

32 Branch
mozilla39
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm5+, firefox39 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

20.67 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.31 KB, application/zip
Details
39 bytes, text/x-review-board-request
Details
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.
smaug wrote:

"Couldn't you create a PrintDialog protocol, and then in __delete__ continue. While you're waiting __delete__ you'd call NS_ProcessNextEvent"
Assignee: nobody → jmathies
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
Flags: needinfo?(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.
Flags: needinfo?(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?
Flags: needinfo?(jmathies)
(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.
Assignee: jmathies → mconley
Flags: needinfo?(jmathies)
Attachment #8563011 - Attachment description: PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=? → [WIP] PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?
See Also: → 1136280
Attachment #8563011 - Attachment is obsolete: true
Attachment #8569946 - Attachment is obsolete: true
Attachment #8569947 - Attachment is obsolete: true
Attachment #8571525 - Attachment is obsolete: true
Attachment #8572756 - Attachment is obsolete: true
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.
Attachment #8572761 - Flags: feedback?(jmathies)
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).
(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.
Attachment #8572761 - Flags: feedback?(jmathies) → feedback+
Hmm, this conflicted heavily, I pulled yesterday. Please post an updated wip when you get a chance.
/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
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.
Attachment #8572761 - Attachment is obsolete: true
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.
Attachment #8574022 - Flags: review?(bugs)
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
Attachment #8574022 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/48634beaa636
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Filed bug 1140929 about refactoring nsPrintEngine to call those blocking prompt functions off of an event.
Attachment #8573494 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.