Make the PPrinting::ShowProgress IPDL method async

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Printing
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Comment 1

3 months ago
Created attachment 8848635 [details] [diff] [review]
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
Attachment #8848635 - Flags: feedback?(mconley)
(Assignee)

Comment 2

3 months ago
Created attachment 8848636 [details] [diff] [review]
Part 2: Make the PPrinting::SavePrintSettings message async

MozReview-Commit-ID: D9duONiE5Z5
Attachment #8848636 - Flags: feedback?(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.
Attachment #8848635 - Flags: feedback?(mconley) → feedback+
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.
Attachment #8848636 - Flags: feedback?(mconley) → feedback+
(Assignee)

Comment 5

3 months ago
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?
Flags: needinfo?(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!)
Flags: needinfo?(mconley)
(Assignee)

Updated

3 months ago
Attachment #8848635 - Flags: review?(mconley)
(Assignee)

Updated

3 months ago
Attachment #8848636 - Flags: review?(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!
Attachment #8848635 - Flags: review?(mconley) → review+
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?
Attachment #8848636 - Flags: review?(mconley) → review+
(Assignee)

Comment 9

3 months ago
Created attachment 8856663 [details] [diff] [review]
Part 2: Make the PPrinting::SavePrintSettings message async

MozReview-Commit-ID: D9duONiE5Z5
Attachment #8856663 - Flags: review?(mconley)
(Assignee)

Updated

3 months ago
Attachment #8848636 - Attachment is obsolete: true
(Assignee)

Comment 10

3 months ago
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!
Flags: needinfo?(andrei.vaida)
(Assignee)

Comment 11

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

oops - didn't mean to re-request review
Attachment #8856663 - Flags: review?(mconley)
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.
Flags: needinfo?(andrei.vaida) → needinfo?(michael)
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".
(Assignee)

Comment 14

2 months ago
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.
(Assignee)

Comment 15

2 months ago
(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).
Flags: needinfo?(michael)
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 16

2 months ago
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).
Flags: needinfo?(nihsanullah)
Whiteboard: [qf:p1] → [qf]
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.
(Assignee)

Comment 18

a month ago
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
Flags: needinfo?(nihsanullah) → needinfo?(andrei.vaida)
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?
(Assignee)

Comment 20

a month ago
(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.

Updated

a month ago
Whiteboard: [qf] → [qf:p3]
(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?
Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)
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!
Flags: needinfo?(emil.ghitta)
(Assignee)

Updated

a month ago
Blocks: 1366332
(Assignee)

Updated

a month ago
Attachment #8856663 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Summary: Make PPrinting's IPDL methods async → Make the PPrinting::ShowProgress IPDL method async
(Assignee)

Comment 23

a month ago
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?
Attachment #8848635 - Flags: review?(wmccloskey)
Attachment #8848635 - Flags: review?(wmccloskey) → review+

Comment 24

a month ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99aacc535fdb
Make the PPrinting::ShowProgress IPC message async, r=mconley, r=billm
https://hg.mozilla.org/mozilla-central/rev/99aacc535fdb
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.