164384 - Print Preview does not close

Status: VERIFIED FIXED

(Core :: Print Preview, defect)

Description

[Sam]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1b) Gecko/20020823 MultiZilla/v1.1.20
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1b) Gecko/20020823 MultiZilla/v1.1.20

After using Print Preview to print pages, it will not close when pressing the 'Close' button. Each time I must then clos out the browser and re-start it.

Reproducible: Always

Steps to Reproduce:
1.Select Print Preview
2.Print a page
3.Press the Close button

Actual Results:  
The Close button is unresponsive. The browser must be closed and re-started.

Expected Results:  
Closed the Print Preview and returned to normal browsing.

This only started with some of the most recent nightly builds and did not happen to me earlier this week (week of 8-18-02).

Comment 1

[Tracy Walker [:tracy]]

confirmed with windows98 trunk build 2002-08-26-04-trunk

Comment 2

[sujay]

--> samir

Comment 3

[Jessica Blanco]

This also happens on OS/2 and Win NT. The fix for bug 139905 checked in on
08/21/2002 by rods caused this problem.

Comment 4

[Bill Mason]

Nominating for 1.2.

Comment 5

[Brendan Eich [:brendan]]

Why is this assigned to sgehani and not rods?  Drivers will want this fixed for
1.2alpha.

/be

Comment 6

[Pete Zha]

Attachment: purpose patch

Maybe we shouldn't release mPrintEngine when we in printpreview since we need
it to return to GalleyPresentation by press close button.

Comment 7

[rods (gone)]

taking

Comment 8

[rods (gone)]

really taking

Comment 9

[rods (gone)]

Comment on attachment 98111 [details] [diff] [review]
purpose patch

r=rods for pete's patch (thank you)

Comment 10

[Asa Dotzler [:asa]]

rods, who can super-review this. Time is short for 1.2alpha.

Comment 11

[Simon Fraser [no longer active]]

Do we really understand the refcounting behaviour here?

     mPrintEngine->Destroy();
-    NS_RELEASE(mPrintEngine);
+    if (!GetIsPrintPreview()) {
+      NS_RELEASE(mPrintEngine);
+    }

Why would we not want to release something that has just been destroyed? And why
does the failure to do a release prevent the window from being closed?

Comment 12

[rods (gone)]

Kin and I talked, this needs more work.

Comment 13

[rods (gone)]

Attachment: patch v2

Actually we do understand the ref count behavior.

The DV holds a strong ref to the PrintEngine (PE) and the PE holds a weak ref
to DV. Other services and objects in the system may hold a strong pointer to
either the PE or an object that it has a strong ref to.

In the DV it is necessary to call "Destroy" before the release of the PE
because it needs a chance to cleanup its internal objects (like the PrintData
object). By cleaning up its internal data object release the PE. Then the DV
can release the PE. It is never assumed that when the DV releases the PE that
it will get destroyed at that moment. Typically it gets detroyed when the
PrintProgress finishes and goes away.

I have tested this with the debugger and verified that the PE is getting
detroyed when printing, when PP, and when printing from PP.

In summary, Pete's patch was correct except that the Printing data was being
cleaned up when in PP, this patch makes it so the printing data gets cleaned
up.

Comment 14

[Asa Dotzler [:asa]]

kin, pete, can one of you review? who can sr? time is short and we need
something asap.

Comment 15

[Pete Zha]

Comment on attachment 98210 [details] [diff] [review]
patch v2

>+    if (!mPrintEngine->GetIsPrintPreview()) {
>+      mPrintEngine->Destroy();
>+      NS_RELEASE(mPrintEngine);
>+    } else {
>+      mPrintEngine->DestroyPrintingData();
>+    }
Maybe we can avoid using "!" op by exchange code in if(){} and else{}.
r=pete

Comment 16

[rods (gone)]

Attachment: patch with pete's suggestion

Comment 17

[rods (gone)]

Comment on attachment 98443 [details] [diff] [review]
patch with pete's suggestion

bringing pete's r= forward r=pete

Comment 18

[kinmoz]

Comment on attachment 98443 [details] [diff] [review]
patch with pete's suggestion

sr=[e-mail]

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 98443 [details] [diff] [review]
patch with pete's suggestion

a=asa (on behalf of drivers) for checkin to 1.2a

Comment 20

[rods (gone)]

fixed

Comment 21

[Tracy Walker [:tracy]]

verified fixed