Closed Bug 1419775 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Printing: Setup, defect)

Product:
Core
Component:
Printing: Setup
Version:
59 Branch
Platform:
Unspecified
Windows 10
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: calixte, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(2 files)

Bug 1419775 - Part 1.Prevent send IPC message when cancelling the print job if IPC connection already closed.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.
59 bytes, text/x-review-board-request
bobowen
: review+
Details 
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
Flags: needinfo?(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.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Flags: needinfo?(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.
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 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 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!
Attachment #8931606 - Flags: review?(mconley) → review+
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.
Attachment #8931607 - Flags: review?(bobowencode)
(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 :).
Flags: needinfo?(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.
Flags: needinfo?(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
(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.
Flags: needinfo?(mantaroh)
Comment on attachment 8931607 [details]
Bug 1419775 - Part 2.Notify closing IPC connection to PrintProgressService.

Clearing while waiting for need info.
Attachment #8931607 - Flags: review?(bobowencode)
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.
Flags: needinfo?(mantaroh)
Attachment #8931607 - Flags: review?(bobowencode)
(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 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.
Attachment #8931607 - Flags: review?(bobowencode) → review+
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.
Pushed by mantaroh@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/f466c92f42bf
https://hg.mozilla.org/mozilla-central/rev/54fefcd4dc62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: