Closed Bug 671976 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Print Preview, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: billm)

References

Details

(Keywords: assertion)

Attachments

(1 file)

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.
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.)
(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.
Does adding OnPageHide to DocumentViewerImpl::SetDocumentInternal
if (mDocument->IsStaticDocument()) {
  // Add the pagehide call here
  mDocument->SetScriptGlobalObject(nsnull);
  mDocument->Destroy();
}
help?
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.)
Summary: test_printpreview.xul destroys a nsDocument that has mIsShowing == PR_TRUE → test_printpreview.xul & other test_printpreview_* tests destroy a nsDocument that has mIsShowing == PR_TRUE
(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
(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.)
Depends on: 675722
No longer depends on: 670324
Keywords: assertion
Summary: test_printpreview.xul & other test_printpreview_* tests destroy a nsDocument that has mIsShowing == PR_TRUE → "ASSERTION: Destroying a currently-showing document: '!mIsShowing', file nsDocument.cpp, line 1583" on test_printpreview.xul & other test_printpreview_* chrome mochitests
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.
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.
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?
(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 .....
Oh, that explains it. Thanks! :)
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)
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
Whiteboard: [leave open]
Summary: "ASSERTION: Destroying a currently-showing document: '!mIsShowing', file nsDocument.cpp, line 1583" on test_printpreview.xul & other test_printpreview_* chrome mochitests → "ASSERTION: Destroying a currently-showing document: '!mIsShowing', file nsDocument.cpp, line 1583" on test_printpreview.xul & other test_printpreview_* and every other chrome mochitest
Attached patch assertion fixSplinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #725101 - Flags: review?(bugs)
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);
    }
Attachment #725101 - Flags: review?(bugs) → review+
It looked good on try without the null checks, but I added them anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3df2b971c106
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/3df2b971c106
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 877612
Depends on: 880304
You need to log in before you can comment on or make changes to this bug.