Bug 1090444 - The Printing Progress dialog does not show progress or close when opened from the child process

Leftovahs!

So here's the scoop - I've got this Printing Progress dialog opened via the ShowProgress IPC call over PPrinting.

A few things here:

Suppose I have PPrintingDialog…. what would that look like?

Obviously:

StateChange

ProgressChange

DocTitleChange

DocURLChange

child:

__delete__

So let's think this through a scenario. Child calls nsPrintingPromptServiceProxy's ShowProgress method… we call the ShowProgress IPC method, and this returns immediately with something that implements PPrintingDialog.

Whatever that thing is, it also needs to implement nsIWebProgressListener on the child side. On the parent side, we _get back_ a nsIWebProgressListener from the dialog, and that's what we have to connect our stuff to. So the parent implementation of this protocol needs to know how to wrap a returned nsIWebProgressListener from the parent, and send it the data that the child sends.

At the same time, I want this thing to not just be for progress dialogs, if possible. Like, I want it to be usable for non-sync-ifying those IPC calls to the parent in PPrinting.

So I have this light, generic implementation on the child and parent side. It just knows how to communicate "I've been closed", with maybe a reason for closing (cancel vs window close?).

How does that work? Ah… looks like nsPrintEngine::ShowPrintErrorDialog is called if the rv from ShowPrintDialog is not NS_OK, and can handle:

#define ENTITY_FOR_ERROR(label) \

case NS_ERROR_##label: stringName.AssignLiteral("PERR_" #label); break

ENTITY_FOR_ERROR(GFX_PRINTER_NO_PRINTER_AVAILABLE);

ENTITY_FOR_ERROR(GFX_PRINTER_NAME_NOT_FOUND);

ENTITY_FOR_ERROR(GFX_PRINTER_COULD_NOT_OPEN_FILE);

ENTITY_FOR_ERROR(GFX_PRINTER_STARTDOC);

ENTITY_FOR_ERROR(GFX_PRINTER_ENDDOC);

ENTITY_FOR_ERROR(GFX_PRINTER_STARTPAGE);

ENTITY_FOR_ERROR(GFX_PRINTER_DOC_IS_BUSY);

ENTITY_FOR_ERROR(ABORT);

ENTITY_FOR_ERROR(NOT_AVAILABLE);

ENTITY_FOR_ERROR(NOT_IMPLEMENTED);

ENTITY_FOR_ERROR(OUT_OF_MEMORY);

ENTITY_FOR_ERROR(UNEXPECTED);

default:

ENTITY_FOR_ERROR(FAILURE);

#undef ENTITY_FOR_ERROR

So I need to communicate that sort of thing back.

It looks as if NS_ERROR_ABORT is used to communicate that the window was closed by something other than pressing OK - like hitting the X on the window, or hitting cancel. Ok, that's useful information.

So that __delete__ should take some kind of status.

/**

*/

void showProgress(in nsIDOMWindow parent,

out boolean notifyOnOpen);

This progress dialog is NOT MODAL - and does not spin its own event loop.

At least for the XUL progress dialog (which is really the one I care about), it's opened with openDialog, without the modal option. So it returns immediately. Great!

nsPrintingPromptServiceProxy showProgress is called.

We resolve the nsIDOMWindow to a PBrowser, and then call PPrinting's showProgress method, passing isForPrinting and the PBrowser.

On the parent side… open the progress dialog, getting back webProgressListener and a printProgressParams pointers and a notifyOnOpen boolean. Also create and pass an observer. Construct a PPrintingDialog with the webProgressListener and printProgressParams, and pass the PPrintingDialog, and notifyOnOpen to the child. Also passes nsresult.

Child gets nsresult status. Assuming NS_SUCCEEDED, also gets PPrintingDialog, and wraps it to be a nsIWebProgressListener and nsIPrintProgressParams, and passes it nsIObserver. Returns it and notifyOnOpen to the caller. The observer might fire when dialog is open.

Parent opens the dialog on the next tick, or at least tries to. If it fails out, tells PPrintingDialog about it, and the child hears about it, stops spinning event loop, and returns result.

So the dialog is open, and the child has handed back the caller of ShowProgress an nsIWebProgressListener and an nsIPrintProgressParams. Those things receive information, which gets forwarded through PPrintingDialog.

When the dialog closes, __delete__ the PPrintingDialog.

So, I think that's how the progress dialog will work.

What about the other dialogs?

ShowPrintDialog and ShowPageSetup are both modal.

async PPrintingDialog {

parent:

async StateChange

async DocURLChange

child:

__delete__(status, PrintData (optional))

}

nsPrintingPromptServiceProxy showPrintDialog is called.

We resolve the nsIDOMWindow to a PBrowser, then call PPrinting's showPrintDialog, passing PBrowser and the current nsIPrintSettings as PrintData.

On the parent side, instantiate a PPrintingDialog, queue opening the showPrintDialog dialog on next tick, return it to the child. On the child side, take that PPrintingDialog it gets passed, and spin event loop until __delete__ gets called.

In parent, once dialog function returns, if success, construct a PrintData to return, return nsresult and PrintData to child in __delete__ call.

__delete__ in child is received. Stop blocking. Looks at nsresult, and potentially PrintData and returns to caller.

I think I need to think through the wrapping that showProgress does, and the blocking on __delete__. Once I've got that figured out, I think I can say whether or not this is going to work.

Y'know, I think I'm making things too complicated by having a single protocol. I was originally trying to avoid a separate protocol for the modal print settings dialog, but it really doesn't have a whole lot in common with the print progress dialog. I think I'm getting myself set up for troubles by trying to mix them.

So let's assume two different protocols instead :

async PPrintProgressDialog {

async DocURLChange

__delete__()

child:

async DialogOpened

}

async PPrintSettingsModalDialog {

child:

__delete__(nsresult status, PrintData)

We might be able to re-use this second protocol for ShowPageSetup, by the by.

So let's think through PPrintProgressDialog now in terms of the wrapping of things and the proxying of nsIWebProgressListener and nsIPrintProgressParams stuff.

ONCE MORE FROM THE TOP.

On the parent side… create an observer to pass, then open the progress dialog, getting back webProgressListener and a printProgressParams pointers and a notifyOnOpen boolean. Construct a PPrintProgressDialog with the webProgressListener and printProgressParams, and pass the PPrintingDialog, and notifyOnOpen to the child. Also passes nsresult.

Pseudo-ish code follows

class PrintProgressDialogParent MOZ_FINAL: public PPrintProgressDialogParent,

public nsIObserver {

public:

virtual PrintProgressDialogParent(nsIWebProgressListener* aWPL,

nsIPrintProgressParams* aPPP);

virtual bool

RecvStateChange(long flags, nsresult status);

virtual bool

RecvProgressChange(long curSelfProgress,

long maxSelfProgress,

long curTotalProgress,

long maxTotalProgress);

virtual bool

RecvDocTitleChange(nsAString title);

virtual bool

RecvDocURLChange(nsAString url);

NS_DECL_ISUPPORTS

NS_DECL_NSIOBSERVER

private:

nsCOMPtr<nsIWebProgressListener> mWPL;

nsCOMPtr<nsIPrintProgressParams> mPPP;

};

class PrintProgressDialogChild MOZ_FINAL: public PPrintProgressDialogChild,

public nsIWebProgressListener,

public nsIPrintProgressParams

{

public:

PrintProgressDialogChild(nsIObserver* aOpenObserver);

virtual bool

RecvDialogOpened(); // Calls mObs->Observe

NS_DECL_ISUPPORTS

NS_DECL_NSIWEBPROGRESSLISTENER

NS_DECL_NSIPRINTPROGRESSPARAMS

private:

nsCOMPtr<nsIObserver> mObs;

};

This is what gets passed back to the caller of showProgress. The implementation methods for nsIWebProgressListener and nsIPrintProgressParams will call the methods to communicate with the parent, naturally.

Now let's think about the lifetime of the PrintProgressDialogParent/Child implementations now. Who controls how long this thing lives?

My first instinct is to tie the lifetime of the channel to the open-ness of the dialog - to shut down the protocol channel once the dialog closes. But I don't think there's a super-easy way of doing that. Perhaps the better approach would be to tie the lifetime to the lifetime of nsIWebProgressListener and nsIPrintProgressParams in the child.

Like, the child implementation of PrintProgressDialogChild _is_ the nsIWebProgressListener and nsIPrintProgressParams. When the caller drops the nsCOMPtrs to us, we'll decrement the refcount, and eventually we'll destruct - at which point we should tell the parent that we destructed.

So, let's suppose that the child drops the references, and we destruct. On the parent-side, what do we do? We'll drop the references to mWPL and mPPP… if anything is still depending on them, they'll stay alive, but they just won't receive any updates. Ok, I think that works.

A problem might arise if we destruct somehow before we're received the nsIObserver notification… so I think what should probably happen is that the manager of PPrintProgressDialog, when deleting, doesn't actually do any deleting, but decrements the refcount. That way, the nsIObserver will stay alive as long as anybody cares about it.

But what if the channel is down and somehow we get the nsIObserver notification? I guess on __delete__, we should also set a flag internally for PrintProgressDialogParent that tells it that it's without a child, and then if an observer notification comes in, just warn and return.

Ok, I think that'll work. I think I can implement this now.

Now let's think through PPrintSettingsModalDialog.

showPrintSettings is called.

On the parent side, instantiate a PPrintSettingsModalDialog, store it in the PrintingParent somewhere (I think?)… because opening multiple print settings dialogs is probably not going to be supported. Why would we even support that.

Queue opening the showPrintDialog dialog on next tick, return the showPrintDialog to the child. On the child side, take that PPrintingDialog it gets passed, and spin event loop until __delete__ gets called. HOW DOES THIS WORK

On the parent side, once we hit next tick, open the print settings dialog. Whatever happens, after that function exits, ensure that the stashed PPrintSettingsModalDialog member exists for PrintingParent, and then call __delete__ on it, passing the status and PrintData).

So that's all dandy. I just have to figure out this event loop thingy. Are there examples of us spinning the event loop waiting for a __delete__?

From smaug:

"Couldn't you create a PrintDialog protocol, and then in __delete__ continue. While you're waiting __delete__ you'd call NS_ProcessNextEvent"

So in the child, we get that __delete__ call on our implementation of PPrintSettingsModalDialogChild, and maybe that PPrintSettingsModalDialogChild...

Wait. Wait, do I even need a protocol here? This is really just a message that sends a status and PrintData. Why do I need a new protocol? Can't I just re-use PPrinting?

Just hear me out here, self.

Let's make it so that the child can receive a PrintDialogResult message, that takes an nsresult and a PrintData.

Then, let's make it so that we async call the parent with ShowPrintDialog, and then nsPrintingPromptServiceProxy spins the event loop while we wait for nsresult to be set on a member of nsPrintingPromptServiceProxy.

Parent receives the message, and opens the dialog, and messages down the result once the function returns, passing back both the nsresult and the serialized nsIPrintSettings as PrintData.

nsPrintingPromptServiceProxy spins and spins, and when it receives the PrintDialogResult message, it sets nsresult and PrintData on member variables and exits. The nested event loop exits as soon as that happens.

On exit of the loop, we clear our the PrintData and result, deserialize to an nsIPrintSettings, and return the result and nsIPrintSettings. Boom, done.

What about showPageSetup? How does that work?

The mechanism is the same, actually. It's the exact same thing. I think that'll work.

Ok, I think this is ready to try. I think this will work.

I can probably do the progress bar patch in this bug, and do the showPrintDialog thing in the bug having to do with the sync IPC call.

Steps:

Define a new protocol PPrintProgressDialog, managed by PPrinting, with stubs so that we compile

Huh… build failure:

6:51.20 In file included from /Users/mikeconley/Projects/mozilla-central/embedding/components/printingui/ipc/PrintingParent.cpp:7:

What the hell is this? I didn't change PrintingParent.cpp too much - just added the PPrintProgressDialogParent construction stuff because of IPDL management.

Welp… let's see what happens when I start removing the things I added?...

Oh - notice that it's attempting to use the mozilla::embedding namespace. That looks just flat out wrong - mozilla::embedding::JS, unsurprisingly, is not a thing that exists.

So somehow, I've changed things so that something we've included is using the mozilla::embedding namespace.

Ok, so I just commented out the introduction of my new protocol, and my stubs, and of course things compile properly now. It's unfortunate that it's kind of "all or nothing" - it's hard for me to introduce or remove just part of this stuff, since it all kinda relies on its other bits.

AHGAGAGG - I forgot to close my namespaces inside PrintProgressDialogParent.cpp. I found this by splitting out the PrintProgressDialog stuff into SOURCES instead of UNIFIED_SOURCES.

Ok, back on track now.

YEAH - I can build it. We're in good shape here.

Define PrintProgressDialogParent, using something like the header I laid out above. Stub out the implementation.

Define PrintProgressDialogChild, using something like the header I laid out above. Stub out the implementation.

Do the observer work on the parent side - calling into the actual nsPrintingPromptService::ShowProgress.

So I've got a flaw in my design. PrintProgressDialogParent takes the nsIWebProgressListener and nsIPrintProgressParams as constructor arguments, but we need the PrintProgressDialogParent (which implements nsIObserver) to pass to nsIPrintingPromptService::ShowProgress (which is what hands us the nsIWebProgressListener and nsIPrintProgressParams!).

Chicken and egg problem. I think I'm going to have to alter the constructor to not take the nsIWebProgressListener and nsIPrintProgressParams, and instead, after we call ShowProgress, we should take the returned nsIWebProgressListener and nsIPrintProgressParams and pass them to the PrintProgressDialogParent.

Let's think about the lifetime of this thing again… and how does it tie in with the AllocPPrintProgressDialogParent defined on PrintingParent?

So jimm and smaug had an interesting idea that make things a little less complicated - how about instead of instantiating the PPrintProgressDialog in the parent, I instantiate it in the child, and pass it to the parent?

How would that work?

We resolve the nsIDOMWindow to a PBrowser, instantiate a PPrintProgressDialog child, and then call PPrinting's showProgress method, passing isForPrinting, the PBrowser, and the PPrintProgressDialog.

On the parent side… open the progress dialog by passing it the PPrintProgressDialog as the observer, and get back webProgressListener and a printProgressParams pointers and a notifyOnOpen boolean.

Set the nsIWebProgressListener and nsIPrintProgressParams on the PPrintProgressDialog.

Pass notifyOnOpen to the child. Also passes success.

Child gets success status. Assuming true, returns the PPrintProgressDialog (which implements nsIPrintProgressParams and nsIWebProgressListener) and notifyOnOpen to the caller. The observer might fire when dialog is open.

So the dialog is open, and the child has handed back the caller of ShowProgress an nsIWebProgressListener and an nsIPrintProgressParams. Those things receive information, which gets forwarded through PPrintingDialog.

Now let's think about the lifetime of the PrintProgressDialogParent/Child implementations now. Who controls how long this thing lives? Let's continue to tie the lifetime to the lifetime of nsIWebProgressListener and nsIPrintProgressParams in the child.

Phew, ok - I think this strategy might work.

Alright, so I've kinda (kinda?) got things wired up to at least send progress events over, but now I'm getting a runtime failure that crashes the tab:

"IPDL protocol error: bad ID for PPrinting

IPDL protocol error: Error deserializing 'PPrintProgressDialogParent'"

So something is wrong with how I've instantiated PPringProgressDialogChild - maybe I have to send a construction request to the parent? I'll try that.

Yeah, that worked - but no progress is being sent from the child. Lemme find out why… attach ye olde MSVC debugger...

Ok! We're hooked up! I've got a progress dialog that shows progress, and then closes!

Do the send message work on the child side.

Figure out garbled logging and crash on close.

So we're crashing in the parent process because we're attempting to ReleaseListeners in nsPrintProgress, and… I think we're attempting to free something that's already been freed. Like, I think the nsIObserver has been destroyed already, but it didn't tell everybody. :/

Let me confirm that by putting a breakpoint in the PrintProgressDialogParent destructor...

Ah, yep - so, we delete the PPrintProgressDialogParent, when we should probably be decrementing the refcount instead.

Right, so that's working - and I can confirm that the PPrintProgressDialogParent/Child are going down at the right time (need to wait for garbage collection after printing, but it happens).

So that's great!

What happens if somebody cancels the progress dialog?

Oh this is hilarious. This doesn't work, even without e10s, and even without any of my printing work. Cancelling print jobs from the progress dialog does not work, and has not worked for some time. Bug 424965 and bug 761008. Yeesh. So, uh, not my division I guess - though I'll likely have the skills to fix this when I'm done. I might want to get those bugs backlogged.

Test lifetime scenarios to ensure we're not leaking the world

We seem to be good here. It looks like the PrintProgressDialogParent needs to wait a while to get garbage collected, but it eventually does.

Alright, posted. Let's see what blassey thinks.

He digs it! Just some formatting issues, and then let's land this puppy… actually, I might do a try push first, just to be sure.

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c785f606a53b

Phew, glad I did that. Busted build on Linux debug.

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82ded1c64f38

Re-push with some includes in the .cpp file...

Guh, and again:

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9011873fd29

GUH, and AGAIN.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a346670b7a1a

If this fails to build again, I'm booting up my Ubuntu VM and doing this once and for all over there.

Woo, been building for 44 minutes. That's a good sign.

WOO BUILT - aaaaand landed on fx-team.

TODO:

File bug to support ShowPrinterProperties, which blocks Linux printing - just appending information on bug 1090448.

File bug to remove observer from ShowPageSetup call in nsIPrintingPromptService - filed bug 1100988.