1419775 - Crash in mozilla::ipc::MessageChannel::CxxStackFrame::CxxStackFrame | mozilla::ipc::MessageChannel::Send | mozilla::embedding::PPrintProgressDialogParent::SendCancelledCurrentJob

Status: RESOLVED FIXED

(Core :: Printing: Setup, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-4d91604d-b865-4a82-ac7a-1414f0171121.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::ipc::MessageChannel::CxxStackFrame::CxxStackFrame ipc/glue/MessageChannel.cpp:247
1 xul.dll mozilla::ipc::MessageChannel::Send ipc/glue/MessageChannel.cpp:893
2 xul.dll mozilla::embedding::PPrintProgressDialogParent::SendCancelledCurrentJob ipc/ipdl/PPrintProgressDialogParent.cpp:73
3 xul.dll mozilla::embedding::PrintProgressDialogParent::Observe toolkit/components/printingui/ipc/PrintProgressDialogParent.cpp:100
4 xul.dll nsPrintProgress::SetProcessCanceledByUser toolkit/components/printingui/nsPrintProgress.cpp:139
5 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
6 xul.dll CallMethodHelper::CallMethodHelper js/xpconnect/src/XPCWrappedNative.cpp:1261
7 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1282
8 xul.dll XPC_WN_GetterSetter js/xpconnect/src/XPCWrappedNativeJSOps.cpp:957
9 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:473

=============================================================

There is 1 crash in nightly 59 with buildid 20171120100042. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1409971.

[1] https://hg.mozilla.org/mozilla-central/rev/154ec3c19c88

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Thanks,

I guess need to check IPC connection is alive berfore calling SendCancelCurrentJob(). (i.e. should check whether mActive is true or not[1])

[1] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/toolkit/components/printingui/ipc/PrintProgressDialogParent.cpp#90

In any case, I'll look into this.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

OK. I can reproduce this.

STR:
 1) Open several tabs. (i.e. boot two or more content process in order to prevent to exit parent process)
 2) Open print dialog.
 3) Push [OK] of print dialog.
 4) Close current tab. (In this step, print progress dialog will display)
 5) Push [Cancel] of print progress dialog.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

The progress dialog parent should check that IPC channel is open before sending to a child. Furthermore, if IPC connection is disconnected, layout can't send status to parent. As result, the parent process will not display progress dialog next time since a status of parent process is displaying the progress dialog. So I guess we should notify to parent dialog that IPC connection is disconnected.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1419775 - Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed.

If target content tab is already closed, progress dialog can't notify to content
process that cancelling the print job. So this patch will prevent send
cancelling message in this case.

Review commit: https://reviewboard.mozilla.org/r/202754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202754/

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

If content which printing target content is closed when printing,
PrintProgressService will continue display progress dialog even if cancelling
the print progress dialog. [1]

This patch will:
 * If IPC connection is closed, notify to PrintProgressService via
   nsIWebProgressListener.

Review commit: https://reviewboard.mozilla.org/r/202756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202756/

Comment 6

[Bob Owen (:bobowen)]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

https://reviewboard.mozilla.org/r/202756/#review208074

::: layout/printing/ipc/RemotePrintJobParent.cpp:242
(Diff revision 1)
>  
>  void
>  RemotePrintJobParent::ActorDestroy(ActorDestroyReason aWhy)
>  {
> +  uint32_t numberOfListeners = mPrintProgressListeners.Length();
> +  for (uint32_t i = 0; i < numberOfListeners; ++i) {

I think this is OK, although does this still happen when we've completed printing normally?
The progress dialog may not be the only listener here, so if they get another STATE_STOP message could that cause problems?

Also, can we just use a range-based for loop nowadays.

Comment 7

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

Comment on attachment 8931606 [details]
Bug 1419775 - Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed.

https://reviewboard.mozilla.org/r/202754/#review208180

Thanks!

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

https://reviewboard.mozilla.org/r/202756/#review208880

Clearing review for the moment.
I think that sending an extra STATE_STOP will be OK, but can you check existing listeners please.

Comment 9

[Andrew Overholt [:overholt]]

(In reply to Bob Owen (:bobowen) from comment #8)
> Comment on attachment 8931607 [details]
> Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.
> 
> https://reviewboard.mozilla.org/r/202756/#review208880
> 
> Clearing review for the moment.
> I think that sending an extra STATE_STOP will be OK, but can you check
> existing listeners please.

Just want to be sure Mantaroh saw this (I'm doing some regression triage for 59 :).

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Andrew Overholt [:overholt] from comment #9)
> (In reply to Bob Owen (:bobowen) from comment #8)
> > Comment on attachment 8931607 [details]
> > Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.
> > 
> > https://reviewboard.mozilla.org/r/202756/#review208880
> > 
> > Clearing review for the moment.
> > I think that sending an extra STATE_STOP will be OK, but can you check
> > existing listeners please.
> 
> Just want to be sure Mantaroh saw this (I'm doing some regression triage for
> 59 :).

I'm Sorry, I was taking a leave. I'll confirm this soon.

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931606 [details]
Bug 1419775 - Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202754/diff/1-2/

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202756/diff/1-2/

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

https://reviewboard.mozilla.org/r/202756/#review208074

I'm sorry for my late response.

> I think this is OK, although does this still happen when we've completed printing normally?
> The progress dialog may not be the only listener here, so if they get another STATE_STOP message could that cause problems?
> 
> Also, can we just use a range-based for loop nowadays.

If gecko send an extra STATE_STOP when RemotePrintJobParent has listener, displayed dialog will close due to this dialog's listener[1].

Detail:
This listener is a part of a RemotePrintJobParent[2], So this listener is always referring to nsPrintParams which created by nsPrintinPromptService when showing print dialog(prepare / printing / print preview)[3]. Furthermore, this nsPrintParmas has nsIWebProgressListener, this listener add when initializing the dialog[4].

[1] https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/toolkit/components/printing/content/printProgress.js#51-81
[2] https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/layout/printing/ipc/RemotePrintJobParent.h#88
[3] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/toolkit/components/printingui/ipc/PrintingParent.cpp#64
[4] https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/toolkit/components/printing/content/printProgress.js#231

Comment 14

[Bob Owen (:bobowen)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #13)
> Comment on attachment 8931607 [details]
> Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.
> 
> https://reviewboard.mozilla.org/r/202756/#review208074
> 
> I'm sorry for my late response.
> 
> > I think this is OK, although does this still happen when we've completed printing normally?
> > The progress dialog may not be the only listener here, so if they get another STATE_STOP message could that cause problems?
> > 
> > Also, can we just use a range-based for loop nowadays.
> 
> If gecko send an extra STATE_STOP when RemotePrintJobParent has listener,
> displayed dialog will close due to this dialog's listener[1].

I understand why it fixes this issue for that listener, but there is an array of listeners, so we should check that any other listeners won't have a problem with it.
I suspect that in reality the dialog is normally the only listener, but I'm not sure. I think we are adding another one for the web extension API for saveAsPDF, but I don't think that has landed yet.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

Clearing while waiting for need info.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

(In reply to Bob Owen (:bobowen) from comment #15)
> Comment on attachment 8931607 [details]
> Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.
> 
> Clearing while waiting for need info.

I'm sorry for my late reply.
This patch will send 'nsIWebProgressListener::STATE_STOP' event for listeners. Gecko will add this listener at following code:
 * PrintingParent::RecvShowProgress()
 * nsFrameLoader::Print()

'PrintingParent::RecvShowProgress' will set a nsPrintProgress as listener, and this event is propagated to printProgress.js(nsIPrintProgress::RegisterListener)[1]. So progress dialog will close if gecko sends this event to this listener. This listener of printProgress.js will call unregisterListner when a dialog is closed.

'nsFrameLoader::Print()' will set null as listener[2].

[1] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/toolkit/components/printing/content/printProgress.js#48-86
[2] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/components/extensions/ext-tabs.js#953

So I think that this patch will not occur the other problem.

Comment 17

[Bob Owen (:bobowen)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #16)
...
> 'nsFrameLoader::Print()' will set null as listener[2].

As I said there are plans to change this, it's bug 1415507, but the current patch looks like it will ignore the extra STATE_STOP on its own.

> [2]
> https://searchfox.org/mozilla-central/rev/
> cf149b7b63ff97023e28723167725e38cf5df757/browser/components/extensions/ext-
> tabs.js#953

Comment 18

[Bob Owen (:bobowen)]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

https://reviewboard.mozilla.org/r/202756/#review217182

::: commit-message-1f6fb:5
(Diff revision 2)
> +Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService. r?bobowen
> +
> +If content which printing target content is closed when printing,
> +PrintProgressService will continue display progress dialog even if cancelling
> +the print progress dialog. [1]

I'm not quite sure what this is trying to say, is it:
"If the content process which is printing target content is closed during printing, PrintProgressService will continue to display progress dialog even if Cancel is clicked."

Also the [1] can be dropped I think, as there is no reference.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931606 [details]
Bug 1419775 - Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202754/diff/2-3/

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202756/diff/2-3/

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

https://reviewboard.mozilla.org/r/202756/#review217182

> I'm not quite sure what this is trying to say, is it:
> "If the content process which is printing target content is closed during printing, PrintProgressService will continue to display progress dialog even if Cancel is clicked."
> 
> Also the [1] can be dropped I think, as there is no reference.

Thank you for your review.
I addressed this comment.

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f466c92f42bf
Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed. r=mconley
https://hg.mozilla.org/integration/autoland/rev/54fefcd4dc62
Part 2.Notify closing IPC connection to PrintProgressService. r=bobowen

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f466c92f42bf
https://hg.mozilla.org/mozilla-central/rev/54fefcd4dc62