670324 - "ASSERTION: Destroying a currently-showing document" with XSLT

Status: RESOLVED INCOMPLETE

(Core :: XSLT, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

###!!! ABORT: Expecting to be paused for pagehide before disconnect: 'mPauseState & nsSMILTimeContainer::PAUSE_PAGEHIDE', file content/smil/nsSMILAnimationController.cpp, line 98

Some notes about the testcase:

* It must be local and its reference to http://hg.mozilla.org/mozilla-central/file/060d1ceb4f2b/content/test/reftest/xml-stylesheet/xslt_selflink_empty_href.xml must be fixed to be correct on your filesystem.

* The timing is fragile. You might need to adjust the timer, try several times, or add a loop back from b2 to b1.

* The abort happens on GC.

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Daniel Holbert [:dholbert]]

Able to reproduce in my linux debug build.

I get this assertion first, though:
{
###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file mozilla/content/base/src/nsDocument.cpp, line 7146
}
and I suspect that might be the root cause.

Note that the last instance of this (a variant of this assertion, firing slightly later) was in bug 654015, and that ended up being a bug in an edge case of document teardown.

Hoping to get to the bottom of this soon.

Comment 3

[Daniel Holbert [:dholbert]]

Ok -- so at the ABORT_IF_FALSE failure, within the "~nsDocument" destructor, we have nsDocument::mVisible being *true*.

So we haven't gotten the OnPageHide call that we're expecting to happen before destruction.  That's the proximal cause of this bug's assertion-failure.

That's bad, I think.  IIUC, we probably should assert that mVisible is false inside ~nsDocument.  Now to figure out why that doesn't hold in this case...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> IIUC, we probably should assert that mVisible is false inside ~nsDocument.

That would be a bogus assert.  For example, a document that's never associated with a docshell will always have mVisible true (but mIsShowing false).

Comment 5

[Daniel Holbert [:dholbert]]

Ah, sorry -- I think I had my flags mixed up.  Would my proposed assert be valid if it were about mIsShowing instead?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Ok -- so at the ABORT_IF_FALSE failure, within the "~nsDocument" destructor,
> we have nsDocument::mVisible being *true*.

(mIsShowing is true there too, FWIW)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, asserting about mIsShowing would be reasonable there.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: backtrace of call #1 to EndLoad (inside of nsDocShell::Stop())

(In reply to comment #2)
> ###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls;

I dug into this a bit -- so, we hit this assertion because we get *two* calls to nsDocument::EndLoad, which makes us queue up *two* asynchronous calls to DispatchContentLoadedEvents.  (each of which calls UnblockOnload(), the latter of which is redundant)

The first EndLoad call seems to be the "normal" one, resulting from a stack involving nsParser::DidBuildModel() and nsDocShell::Stop().

The second EndLoad call, on the other hand, is from a stack inside of txMozillaXSLTProcessor::notifyError.  I'm guessing that this call is saying "Oops, we hit an XSLT error -- interrupt the document load!"  But in this case, it's doing that *after* the doc has finished loading[1].

Attaching the stacktrace here, in case it's useful.  Second one coming up in a sec.

[1] for some definition of "finished loading" (i.e. it's already gotten through EndLoad)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: backtrace of call #2 to EndLoad (inside of txMozillaXSLTProcessor::notifyError())

Comment 10

[Daniel Holbert [:dholbert]]

(FWIW, I don't think the "More UnblockOnload" assertion is actually the root of the other problems here -- that NOTREACHED's clause has an early-return that prevents us from doing any damage.)

Comment 11

[Daniel Holbert [:dholbert]]

Ok, sotxMozillaXSLTProcessor::notifyError calls nsXMLContentSink::OnTransformDone, which passes a *new document* to the DocumentViewerImpl, without hiding / cleaning up the old one.
http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp?mark=403-403,409-409#387

So our DocumentViewerImpl does eventually fire OnPageHide on its document, but at that point it's got a new document.  The old one never gets an OnPageHide call.

I'm guessing we should be making that call inside of nsXMLContentSink::OnTransformDone.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v1: call OnPageHide on old document, inside of OnTransformDone

(In reply to comment #11)
> The old one never gets an OnPageHide call.
> 
> I'm guessing we should be making that call inside of
> nsXMLContentSink::OnTransformDone.

This patch adds that call.  I pass in aPersisted = PR_FALSE as the first argument, since AFAIK we aren't intending to keep the original document around.

Comment 13

[Daniel Holbert [:dholbert]]

I pushed an earlier patch-verison to TryServer, with just an ABORT_IF_FALSE for mIsShowing in ~nsDocument, and that check was tripped by the mochitest-chrome testcase "test_printpreview.xul". e.g.:
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1310694542.1310697638.12075.gz

I plan to dig into that in the morning.

Comment 14

[Daniel Holbert [:dholbert]]

Filed bug 671976 on test_printpreview.xul failing the proposed assertion.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  So this fires a pagehide event on the window, right?  Do we then fire a pageshow event on the XSLT result doc?

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> Hmm.  So this fires a pagehide event on the window, right?

Well, it fires it on the document.  Not sure what "on the window" means in this context.  Note that at this point, we've already swapped the result doc into the DocumentViewerImp, via a call to DocumentViewerImpl::SetDOMDocument -- so the document we're firing a pagehide for has no DocumentViewer anymore.

> Do we then fire
> a pageshow event on the XSLT result doc?

We don't, actually - good point, that does seem asymmetric / bad.

Maybe we should swap the document-showing states between old-doc & new-doc inside of DocumentViewerImpl::SetDocumentInternal()?  (e.g. hide the old doc, show the new doc)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Not sure what "on the window" means in this context. 

pagehide events are actually targeted at the window, not the document. So "the window" means the window.

> so the document we're firing a pagehide for has no DocumentViewer anymore.

Does it still have a script global object?  That is, do we still fire a pagehide DOM event on the window?

Do we really never fire pageshow/onload on xslt result docs right now?  Summoning sicking!

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to comment #17)
> Does [the old document] still have a script global object?  That is, do we still fire a
> pagehide DOM event on the window?

The old document keeps its script global object -- in particular, mScriptGlobalObject is non-null in its destructor.  (Perhaps that's something we should assert about, too?)

> Do we really never fire pageshow/onload on xslt result docs right now? 
> Summoning sicking!

At least in this bug's testcase, we don't fire pageshow in the result doc.  More specifically -- the |aResultDocument| that's passed to nsXMLContentSink::OnTransformDone() has mIsShowing = PR_FALSE, and that stays true until that doc is destructed (which is actually pretty soon, since Jesse's testcase has us redirect to a trivial data URI).

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> (In reply to comment #17)
> > Does [the old document] still have a script global object?  That is, do we still fire a
> > pagehide DOM event on the window?
> 
> The old document keeps its script global object

[without the patch, that is -- that's what I initially thought you were asking about, but now I think I might have misread]

Comment 20

[Boris Zbarsky [:bzbarsky]]

> At least in this bug's testcase, we don't fire pageshow in the result doc.

So if you attach a pageshow listener above this load (e.g. attach it to a chrome <browser> you load the page in), does it not fire at all during this load?

(You can attach it to the window too, but that's racy.)

Comment 21

[Daniel Holbert [:dholbert]]

(Hmm - even with the attached patch, the old document still keeps its script global object for its lifetime (up to ~nsDocument).  That is, the PageHide call added in my patch doesn't get propagated to the window.)

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: attempt at chrome mochitest to answer comment 20

I played around with a chrome mochitest to try to answer comment 20, but it refused to fire any pageshow events on the <browser> element.

bz, let me know if the attached is along the lines of what you had in mind, or if I'm misunderstanding.  When running this test, I get one pageshow event fired for the window per pageload (alert 'win pageshow'), but I never got any pageshow events fired for the <browser> element -- even with simplified versions of this testcase with the redirects removed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I think you want to add a listener to the browser using addEventListener in this case (and may need to make it a capturing listener, since I'm not sure this event bubbles).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 546075 [details] [diff] [review]
fix v1: call OnPageHide on old document, inside of OnTransformDone

I'm just going to punt to sicking on this in general....  He recalls how the XSLT stuff works way better than I do.

Comment 25

[Daniel Holbert [:dholbert]]

I'm not sure how much trouble this actually causes for XSLT, and it doesn't cause any trouble for SMIL, so I filed bug 675722 on reducing the severity of the assertion that this bug's testcase trips to no longer be an "ABORT".

Comment 26

[Daniel Holbert [:dholbert]]

Bug 675722 is now landed on m-i and will be making it to m-c at the next merge.
  --> Updating the summary of this bug to note that this is now an ASSERTION.

Also, per that bug's patch, the assertion here will now be preceded by
a failure in a new & related assertion in nsDocument.cpp, which says "Destroying a currently-showing document".

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 546075 [details] [diff] [review]
fix v1: call OnPageHide on old document, inside of OnTransformDone

Canceling review & unassigning, as I'm not actively working on this anymore, and Bug 675722's patch keeps this from disrupting our fuzzers.  (and the assertion-failure here isn't necessarily scary at all. Definitely un-scary from SMIL's perspective - it's just an anomalous & unexpected pattern of page-showing-management.)

For the record -- the "OnPageHide" call in the attached patch is likely part of what we need here, but comment 15/16 points out that we might need a balancing "OnPageShow" on the result doc. (if the old document was showing)

Comment 28

[Jesse Ruderman]

Attachment: testcase for comment 26 (requires nearby source tree)

Comment 29

[Jesse Ruderman]

Attachment: stack traces

Comment 30

[Daniel Holbert [:dholbert]]

Shifting component to XSLT, since this is a bug in XSLT (as noted in comment 8 & beyond).

Also updating summary to reflect the earlier assertion "Destroying a currently-showing document" noted in comment 26 (which reflects that we're in a busted state at that point).  The original assertion from SMIL code was just a symptom of being in that busted state.

Comment 31

[Jesse Ruderman]

With testcase in comment 28 (adjusted to point to 'dom' instead of 'content'), I now get the following assertions on shutdown:

###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file dom/base/nsDocument.cpp, line 1619

###!!! ASSERTION: Document leaked to shutdown, then the observer service dropped its ref to us so we were able to go away.: '!mObservingAppThemeChanged', file dom/base/nsDocument.cpp, line 1625

Comment 33

[Andrei Purice]

Hey Jesse,
Can you still reproduce this issue or should we close it?

Comment 34

[Andrei Purice]

Marking this as Resolved > Incomplete as per reporter's lack of response.
If anyone can still reproduce this issue re-open it or file a new bug.