1348401 - Make the PPrinting::ShowProgress IPDL method async

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently PPRinting has 2 synchronous IPC messages: PPrinting::ShowProgress, and PPrinting::SavePrintSettings This bug is trying to make these messages async.

I made this change a few days ago, and it seems to pass try, but I know that our printing code isn't super well tested. I have been meaning to do manual testing to make sure printing still works on all platforms, but I just haven't had time to.

This is an initial attach of the patches.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Make the PPrinting::ShowProgress IPC message async

I haven't tested manually to make sure that these patches work yet, so I'm just doing an initial feedback request from mconley to see if I'm doing anything obviously wrong (as he added these IPC messages).

MozReview-Commit-ID: 5pK08I3itYa

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Make the PPrinting::SavePrintSettings message async

MozReview-Commit-ID: D9duONiE5Z5

Comment 3

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

Comment on attachment 8848635 [details] [diff] [review]
Part 1: Make the PPrinting::ShowProgress IPC message async

Review of attachment 8848635 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense to me. Who would have thought that the old notifyOnOpen stuff would actually make things kinda straight forward here.

Comment 4

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

Comment on attachment 8848636 [details] [diff] [review]
Part 2: Make the PPrinting::SavePrintSettings message async

Review of attachment 8848636 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is okay. I'm not sure the child can do much if saving printing prefs somehow fails anyhow.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I'm not sure what a solid testing plan for this would be in terms of making sure that our printing interface works correctly. Do we have a QA document somewhere which covers what I should test to make sure that the printing feature still works correctly?

Comment 6

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

(In reply to Michael Layzell [:mystor] from comment #5)
> I'm not sure what a solid testing plan for this would be in terms of making
> sure that our printing interface works correctly. Do we have a QA document
> somewhere which covers what I should test to make sure that the printing
> feature still works correctly?

Our automated testing story for printing is not great. Bug 1299848 should soon give us some ref-tests, but the actual talking to printer drivers, etc... that is not wonderfully tested automatically.

I believe this is something that might need to be coordinated with QA, with manual testing (perhaps with software printers to save trees!)

Comment 7

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

Comment on attachment 8848635 [details] [diff] [review]
Part 1: Make the PPrinting::ShowProgress IPC message async

Review of attachment 8848635 [details] [diff] [review]:
-----------------------------------------------------------------

This makes good sense to me, thanks!

Comment 8

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

Comment on attachment 8848636 [details] [diff] [review]
Part 2: Make the PPrinting::SavePrintSettings message async

Review of attachment 8848636 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/printingui/ipc/PrintingParent.cpp
@@ +206,2 @@
>  
> +  rv = mPrintSettingsSvc->SavePrintSettingsToPrefs(settings,

If this rv isn't NS_OK, I wonder if we should warn here?

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Make the PPrinting::SavePrintSettings message async

MozReview-Commit-ID: D9duONiE5Z5

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Hey, is there any chance I could get QA to look at this patch to make sure that printing still works correctly on all platforms before I try to land it? I am doing a try build which should generate usable builds here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=790aceb93b8fbd70fc3e255e6b9aff0490057649

Thanks!

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8856663 [details] [diff] [review]
Part 2: Make the PPrinting::SavePrintSettings message async

oops - didn't mean to re-request review

Comment 12

[Emil Ghitta, QA [:emilghitta]]

Hi Michael!

I have tested the printing functionality with the builds you provided in comment 10 and I have found a strange behavior, reproducible only on Windows Oses, when trying to print a specific page from a pdf document.

It seems that if trying to print a specific page (example: page 4) from this https://www.stmarysca.edu/sites/default/files/attachments/files/Faust.pdf pdf document, the printer prints a blank page. 

Please note that this behavior was reproduced on the following OSes:
Windows 10 64bit
Windows 8.1 64bit

This behavior was not reproduced on the following OSes (The printer successfully printed the page):
Mac 10.12.1
Ubuntu 14.04 32bit.

Please note that the mentioned issue is not reproducible with Firefox 55.0a1 (Build Id:20170411030208).

Also please note that I have used the same printer with all the mentioned platforms.

Please let me know if you have further questions.

Comment 13

[Andrew Overholt [:overholt]]

For me on Windows 10, printing a page range (I was selecting 1-1) gives blank pages (XPS or PDF) but printing *all* pages says "A print preview error occurred".

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I'm having a lot of problems reproducing this problem - and this isn't a super high priority qf issue (as printing isn't exactly a common user interaction) so I am inclined to put this bug on hold for a little bit, and I can come back to it later and reproduce the bugs.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(I should note that I _think_ the bug is in the SavePrintSettings patch as it was the one which seemed like the least likely to just work).

Comment 16

[Nika Layzell [:nika] (ni? for response)]

As I said in comment 14, I don't think that this bug should be [qf:p1] as these sync IPC calls are happening in APIs which will be hard to change if changes are needed, and in code which already blocks the entire UI (the print dialog).

Comment 17

[Bas Schouten (:bas.schouten)]

So much of our graphics code is heavily suboptimal for printing as well. If we truly care about the perf of printing, we should talk about this. But once you're printing, our philosophy has always been you're waiting on the printer, not the software.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

I still don't think this should be a high priority for qf, but I don't think we can easily land the second patch (as I think it is what is causing the problems on windows).

I've made a try push with builds with only the first patch, which will remove one of the sync IPCs. I'd appreciate it if someone from QA could test this patch and make sure nothing is broken when we don't change PPrinting::SavePrintSettings :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7da642500dc37df819598152f0a86c3c4a5b61d5

Comment 19

[(no longer active)]

Here is some telemetry data on this from Windows Nightly:

PPrinting::Msg_ShowProgress
Start	End	IPC_SYNC_MAIN_LATENCY_MS Count
0	32	172.44k (90.48%)
32	35	2.66k (1.4%)
35	38	1.87k (0.98%)
38	41	1.51k (0.79%)
41	45	1.75k (0.92%)
45	49	1.56k (0.82%)
49	53	1.11k (0.58%)
53	58	1.11k (0.58%)
58	63	911 (0.48%)
63	68	756 (0.4%)
68	74	631 (0.33%)
74	80	590 (0.31%)
80	87	464 (0.24%)
87	95	451 (0.24%)
95	103	321 (0.17%)
103	112	329 (0.17%)
112	122	241 (0.13%)
122	132	202 (0.11%)
132	143	186 (0.1%)
143	155	141 (0.07%)
155	168	136 (0.07%)
168	183	188 (0.1%)
183	199	155 (0.08%)
199	216	129 (0.07%)
216	235	100 (0.05%)
235	255	75 (0.04%)
255	277	76 (0.04%)
277	301	51 (0.03%)
301	327	48 (0.03%)
327	355	41 (0.02%)
355	386	36 (0.02%)
386	419	31 (0.02%)
419	455	31 (0.02%)
455	495	42 (0.02%)
495	538	23 (0.01%)
538	585	26 (0.01%)
585	636	21 (0.01%)
636	691	16 (0.01%)
691	750	17 (0.01%)
750	Infinity	105 (0.06%)

PPrinting::Msg_SavePrintSettings
Start	End	IPC_SYNC_MAIN_LATENCY_MS Count
0	32	316.66k (92.73%)
32	35	2.66k (0.78%)
35	38	2.08k (0.61%)
38	41	1.7k (0.5%)
41	45	2.06k (0.6%)
45	49	2.07k (0.61%)
49	53	1.4k (0.41%)
53	58	1.44k (0.42%)
58	63	1.37k (0.4%)
63	68	1.15k (0.34%)
68	74	906 (0.27%)
74	80	946 (0.28%)
80	87	739 (0.22%)
87	95	923 (0.27%)
95	103	733 (0.21%)
103	112	830 (0.24%)
112	122	714 (0.21%)
122	132	614 (0.18%)
132	143	448 (0.13%)
143	155	329 (0.1%)
155	168	271 (0.08%)
168	183	239 (0.07%)
183	199	189 (0.06%)
199	216	148 (0.04%)
216	235	144 (0.04%)
235	255	81 (0.02%)
255	277	74 (0.02%)
277	301	85 (0.02%)
301	327	66 (0.02%)
327	355	54 (0.02%)
355	386	42 (0.01%)
386	419	23 (0.01%)
419	455	32 (0.01%)
455	495	28 (0.01%)
495	538	17 (0%)
538	585	23 (0.01%)
585	636	23 (0.01%)
636	691	24 (0.01%)
691	750	17 (0%)
750	Infinity	147 (0.04%)

There is unfortunately a pretty long tail on both of these messages...  But the majority of which finish in a short amount of time.

Do I understand correctly that PPrinting::Msg_ShowProgress is easier to get rid of?  Can we do that part for 57 and defer the rest to a later time?

Comment 20

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #19)
> There is unfortunately a pretty long tail on both of these messages...  But
> the majority of which finish in a short amount of time.

As Bas said, this only runs in the printing code, and it being slow is not as big of an issue as other sync IPCs. Despite the long tail being pretty long, I don't think it's quite worth putting the effort in to make it go away.

> Do I understand correctly that PPrinting::Msg_ShowProgress is easier to get
> rid of?  Can we do that part for 57 and defer the rest to a later time?

I believe that the Msg_ShowProgress patch is not the one which caused the regressions, and that in order to do Msg_SavePrintSettings it will be much trickier. I am asking QA for help testing the Msg_ShowProgress patch to make sure that it is OK to land. If it is I'll just land it and that message will go away. I don't currently plan to put any more effort into getting rid of Msg_SavePrintSettings.

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Michael Layzell [:mystor] from comment #18)
> I still don't think this should be a high priority for qf, but I don't think
> we can easily land the second patch (as I think it is what is causing the
> problems on windows).
> 
> I've made a try push with builds with only the first patch, which will
> remove one of the sync IPCs. I'd appreciate it if someone from QA could test
> this patch and make sure nothing is broken when we don't change
> PPrinting::SavePrintSettings :)
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7da642500dc37df819598152f0a86c3c4a5b61d5

Emil, could you please have a look at this?

Comment 22

[Emil Ghitta, QA [:emilghitta]]

Hi Michael! 

I have tested the printing functionality with the build that you provided in comment 18 using macOS 10.11.6, Windows 10 64bit, Windows 8 64bit and Ubuntu 16.04 64bit.

It seems that the printing functionality works as expected and the issues encountered in comment 12 are not reproducible with this build.

Please needinfo me if you have any questions or further requests!

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8848635 [details] [diff] [review]
Part 1: Make the PPrinting::ShowProgress IPC message async

I need review from an IPC peer to remove this sync message from sync-messages.ini, would you do that for me?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99aacc535fdb
Make the PPrinting::ShowProgress IPC message async, r=mconley, r=billm

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/99aacc535fdb