671976 - "ASSERTION: Destroying a currently-showing document: '!mIsShowing', file nsDocument.cpp, line 1583" on test_printpreview.xul & other test_printpreview_* and every other chrome mochitest

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Daniel Holbert [:dholbert]]

Per bug 670324 comment 7 (and the few comments before that), we should be able to assert in ~nsDocument that mIsShowing is false.

However, if I add that assertion, we fail it in the mochitest-chrome test "test_printpreview.xul".

This happens because we call OnPageShow, and then later switch out the document for a clone (with viewer->SetDOMDocument()), without ever having fired OnPageHide on the old document.

That document-switcheroo happens with this backtrace:
{
  0x00002ae25e6dd3de in DocumentViewerImpl::SetDocumentInternal (this=0x2ae279b69580, aDocument=0x2ae279ed2800, aForceReuseInnerWindow=0) at layout/base/nsDocumentViewer.cpp:1790
#4  0x00002ae25e6dd27d in DocumentViewerImpl::SetDOMDocument (this=0x2ae279b69580, aDocument=0x2ae279ed29a0) at layout/base/nsDocumentViewer.cpp:1771
#5  0x00002ae25f0efcd5 in nsPrintObject::Init (this=0x2ae278f11b20, aDocShell=0x2ae272a644f8, aDoc=0x2ae279cd89a0, aPrintPreview=1) at layout/printing/nsPrintObject.cpp:126
#6  0x00002ae25f0e2d17 in nsPrintEngine::DoCommonPrint (this=0x2ae276fd2e10, aIsPrintPreview=1, aPrintSettings=0x2ae27b44ee00, aWebProgressListener=0x2ae27b94f540, aDoc=0x2ae279cd89a0) at layout/printing/nsPrintEngine.cpp:543
#7  0x00002ae25f0e24bc in nsPrintEngine::CommonPrint (this=0x2ae276fd2e10, aIsPrintPreview=1, aPrintSettings=0x2ae27b44ee00, aWebProgressListener=0x2ae27b94f540, aDoc=0x2ae279cd89a0) at layout/printing/nsPrintEngine.cpp:444
#8  0x00002ae25f0e3ea5 in nsPrintEngine::PrintPreview (this=0x2ae276fd2e10, aPrintSettings=0x2ae27b44ee00, aChildDOMWin=0x2ae279ec3000, aWebProgressListener=0x2ae27b94f540) at layout/printing/nsPrintEngine.cpp:786
#9  0x00002ae25e6e528f in DocumentViewerImpl::PrintPreview (this=0x2ae279b69580, aPrintSettings=0x2ae27b44ee00, aChildDOMWin=0x2ae279ec3000, aWebProgressListener=0x2ae27b94f540) at layout/base/nsDocumentViewer.cpp:3747
}

We probably need to make sure that the switched-out document gets an OnPageHide notification at some point.

(also: I don't trigger this bug when entering Print Preview normally -- only when running that mochitest-chrome testcase)

Filing this bug to investigate/fix.

Comment 1

[Daniel Holbert [:dholbert]]

Not sure if it's related, but I also see this debugging output when running this test:
{
WARNING: Using docshell.printPreview is the preferred way for print previewing!: 'IsInitializedForPrintPreview()', file ../../../mozilla/layout/base/nsDocumentViewer.cpp, line 3694
}

(Might be related to why we hit this issue in the testcase but not with "real" print-previews.)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> Not sure if it's related, but I also see this debugging output when running
> this test:
That could be related, but we should be able to handle that case anyway.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Does adding OnPageHide to DocumentViewerImpl::SetDocumentInternal
if (mDocument->IsStaticDocument()) {
  // Add the pagehide call here
  mDocument->SetScriptGlobalObject(nsnull);
  mDocument->Destroy();
}
help?

Comment 4

[Daniel Holbert [:dholbert]]

Nope, that doesn't help.  I pushed that change to try (along with a patch to assert about mIsShowing) -- http://tbpl.mozilla.org/?tree=Try&rev=88137b42baa4 -- and we still get instances of the assertion:
http://tinderbox.mozilla.org/showlog.cgi?tree=Try&errorparser=unittest&logfile=1310933544.1310937401.4500.gz&buildtime=1310933544&buildname=Rev3%20Fedora%2012%20try%20debug%20test%20mochitest-other&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?tree=Try&errorparser=unittest&logfile=1310934850.1310938618.9144.gz&buildtime=1310934850&buildname=Rev3%20Fedora%2012x64%20try%20debug%20test%20mochitest-other&fulltext=1

The assertion is:
> ###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing

In the above logs, this assertion is triggered during test_printpreview.xul, as well as during test_printpreview_bug396024.xul and test_printpreview_bug482976.xul.  (When I filed this bug, I was using ABORT_IF_FALSE rather than ASSERTION, so I hadn't hit the other failures yet at that point.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Does adding OnPageHide to DocumentViewerImpl::SetDocumentInternal
> if (mDocument->IsStaticDocument()) {
>   // Add the pagehide call here
(In reply to comment #4)
> Nope, that doesn't help.

In particular, it doesn't help because mDocument->IsStaticDocument() is false for the doc that fails the assertion, in test_printpreview.xul

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Per bug 670324 comment 7 (and the few comments before that), we should be
> able to assert in ~nsDocument that mIsShowing is false.

(Bug 675722 has now added the above-suggested assertion on mozilla-central, FWIW.  Updating summary to include assertion message.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20170495&tree=Mozilla-Inbound

Rev3 WINNT 6.1 mozilla-inbound debug test mochitest-other on 2013-02-27 20:19:47 PST for push 146a68b8d9c4
slave: talos-r3-w7-024

20:34:09     INFO -  12666 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/generic/test/test_selection_preventDefault.html | Assertion count 2 is greater than expected range 0-1 assertions.


Probably assertion-spatter from an earlier test.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20187775&tree=Try

Rev3 Fedora 12x64 try debug test mochitest-other on 2013-02-28 09:25:56 PST for push 8b0f6388506d
slave: talos-r3-fed64-051

09:34:59     INFO -  12598 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/generic/test/test_bug508115.xul | Assertion count 2 is greater than expected range 0-0 assertions.

Comment 9

[Daniel Holbert [:dholbert]]

No reports in 1.5 years, and then 2 reports w/in 10 minutes --> something changed in the last day or so.

dbaron, do you know if your recent "adjust expected assertion count" commits changed the counts for these tests?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> No reports in 1.5 years, and then 2 reports w/in 10 minutes --> something
> changed in the last day or so.
> 
> dbaron, do you know if your recent "adjust expected assertion count" commits
> changed the counts for these tests?

We just started checking assertions for these tests .....

Comment 11

[Daniel Holbert [:dholbert]]

Oh, that explains it. Thanks! :)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

histogram of mochitest assertions in my data:

 52  chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul 
 14  chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview_bug396024.xul 
  1  chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview_bug482976.xul 
  1  chrome://mochitests/content/chrome/layout/base/test/chrome/test_transformed_scrolling_repaints_3.html 
296  chrome://mochitests/content/chrome/layout/forms/test/test_bug536567_perwindowpb.html 
 67  chrome://mochitests/content/chrome/layout/forms/test/test_bug665540.html 
  7  chrome://mochitests/content/chrome/layout/generic/test/test_bug469613.xul 
  5  chrome://mochitests/content/chrome/layout/generic/test/test_bug469774.xul 
 11  chrome://mochitests/content/chrome/layout/generic/test/test_bug508115.xul 
 22  chrome://mochitests/content/chrome/layout/generic/test/test_bug514732-2.xul 
 12  chrome://mochitests/content/chrome/layout/generic/test/test_bug632379.xul 
  6  chrome://mochitests/content/chrome/layout/generic/test/test_selection_preventDefault.html 
 95  chrome://mochitests/content/chrome/layout/generic/test/test_selection_underline.html 
119  between TEST-END and TEST-START (oops, should have counted these to prior test)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I added SpecialPowers.gc() calls so this stops bouncing between tests, and annotated its assertions better:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bed36b752a

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And fixed my copy/paste error in the comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/109ab43e98b5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20223558&tree=Mozilla-Inbound&full=1

Comment 17

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20229028&tree=Mozilla-Inbound&full=1

Comment 18

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20228382&tree=Mozilla-Inbound&full=1

Comment 19

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=20231998&tree=Mozilla-Inbound

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4bed36b752a
https://hg.mozilla.org/mozilla-central/rev/109ab43e98b5

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Tweaked GC and annotations further in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da718adb6621

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38d783eae64e

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/da718adb6621
https://hg.mozilla.org/mozilla-central/rev/38d783eae64e

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: assertion fix

This fixes the assertion and removes all the annotations on the tests. The GCs were there to ensure that the assertions didn't fire in some later test.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 725101 [details] [diff] [review]
assertion fix

Make sure to push this to tryserver.

+    nsCOMPtr<nsIContentViewer> viewer;
+    webContainer->GetContentViewer(getter_AddRefs(viewer));
+    if (viewer->GetDocument()->IsShowing()) {
+      viewer->GetDocument()->OnPageHide(false, nullptr);
+    }
Needs some more null checks

    nsCOMPtr<nsIContentViewer> viewer;
    webContainer->GetContentViewer(getter_AddRefs(viewer));
    if (viewer && viewer->GetDocument() && viewer->GetDocument()->IsShowing()) {
      viewer->GetDocument()->OnPageHide(false, nullptr);
    }

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looked good on try without the null checks, but I added them anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3df2b971c106

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3df2b971c106