762753 - "Assertion failure: win->IsClosedOrClosing()" with XSLT, carefully-timed GC

Status: RESOLVED INCOMPLETE

(Core :: XSLT, defect)

Description

[Jesse Ruderman]

Attachment: e.html

1. Save e.html
2. Save 458637-1-inner.xhtml
3. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
4. Load e.html

Assertion failure: win->IsClosedOrClosing(), at dom/base/nsDOMClassInfo.cpp:2012

This assertion was added in bug 691178.

Comment 1

[Jesse Ruderman]

Attachment: 458637-1-inner.xhtml

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Daniel Veditz [:dveditz]]

Assigning to bholley to answer how bad this assertion is, and figure out if this is a problem in the DOM goop or in XSLT itself (and reassign appropriately)

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #0)
> This assertion was added in bug 691178.

It was actually added in bug 640904.

Comment 5

[Bobby Holley (:bholley)]

I'm pretty swamped with security bugs (did 6 last week).

jst and I were talking about this in our 1-on-1 today, and jst thought this might be a good thing for johns to look into. John, can you have a look? It's more or less going to involve crawling around and figuring out exactly how the testcase manages to call into the nsDOMClassInfo PreCreate code when the window has no JS object.

Feel free to ping me for any input I can provide. If this is your first time poking around nsDOMClassInfo stuff, this conversation might be useful: http://people.mozilla.org/~bjacob/bholley-on-wrappers-scriptablehelpers-expandos

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi John, any traction on this one?

Comment 7

[John Schoenick [:johns]]

So the culprit is here:
http://hg.mozilla.org/mozilla-central/file/f2146a6c104e/content/xml/document/src/nsXMLContentSink.cpp#l372

When XSLT fails, it generates a simple error document, and returns it to nsXMLContentSink. It then gets swapped into the docshell, and EndLoad() called on the now-replaced document. This means the DOMContentLoaded event is queued and fired for the old document, which is now sharing its JS Object with the window of the new document. However, SetDOMDocument un-roots said JS object, so the GC can race with it and destroy it.

What we should be doing is calling SetScriptGlobalObject(nullptr) on the old document as it should not be associated with that window any longer -- but because we're still loading, this will cut off the final onload event from firing, meaning onload for an XML/XSLT iframe might not fire in case of failure.

I'm looking into a proper fix

Comment 8

[Jesse Ruderman]

The assertion in comment 0 was removed in bug 784792.  But it sounds like the underlying problem in the XSLT case is not yet fixed.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

This isn't a security bug (also, the assertion we're hitting here was backed out because it was deemed to be incorrect). The failure here is conservative, since we will refuse to hand out any new wrappers for the given scope meaning that we should be safe.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has severity 'critical'.
:peterv, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The code changed a lot and the assertion is no longer there.
I'm closing this old bug.