599473 - Assertion failure: "Leaking script object: !mScriptObject.mObject"

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Arisu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Build Identifier: 

Essentially, the problem is that nsXULDocument::LoadScript assumes that
non-chrome URIs aren't able to have chrome scripts loaded, which isn't true for
documents coming from "about:*". This then leads to a redundant second
compilation and assignment attempt, causing this assertion failure.

An easy fix to this particular problem would be to change IsChromeURI() to
IsOverlayAllowed() in nsXULDocument::LoadScript. This would consider URIs with
"about" scheme, but I don't know if that is sufficient to cover all cases.


Here's a detailed (real world) example of how this leak develops:

1. nsXULDocument::StartDocumentLoad is called with mDocumentURI="about:addons".

2. IsChromeURI(mDocumentURI) evals to false because it only checks for the
   "about" scheme. Therefore, the document isn't fetched from XUL cache.

3. The document will be loaded normally, so control is transferred to
   nsXULDocument::PrepareToLoad resp. nsXULDocument::PrepareToLoadPrototype
   to set up the parser and its content sink.

4. nsXULDocument::StartDocumentLoad lets the parser eat through the document.

5. At some point XULContentSinkImpl::HandleStartElement encounters a script
   element. This transfers control to XULContentSinkImpl::OpenTag and then to
   XULContentSinkImpl::OpenScript.

6. The function finds out that the script element hat a src attribute with
   "chrome://global/content/contentAreaUtils.js" as value.
   Thus, nsXULPrototypeScript::DeserializeOutOfLine is called to try to fetch
   the script from XUL cache first.

7. nsXULPrototypeScript::DeserializeOutOfLine successfully* retrieves the
   script from XUL cache and calls nsXULPrototypeScript::Set to assign it to
   the script prototype element.

8. Later the parser will be finished and call XULContentSinkImpl::DidBuildModel
   -> nsXULDocument::EndLoad -> nsXULDocument::OnPrototypeLoadDone and finally
   go into to nsXULDocument::PrepareToWalk and nsXULDocument::ResumeWalk.

9. While trying to build the elements from the prototypes, the builder will
   sooner or later encounter our nsXULPrototypeNode::eType_Script again.
   Because it (again) finds out that there's an URI assigned, it calls
   nsXULDocument::LoadScript to handle loading it. 

From here on, things go a bit wrong:

A. nsXULDocument::LoadScript evals IsChromeURI(mDocumentURI) and finds out that
   the document the script will be contained in is not chrome (as explained).
   It continues by making a NS_NewStreamLoader to fetch the script source.

B. The nsXULDocument::OnStreamComplete callback is triggered, which tries to
   compile the now-loaded script through nsXULPrototypeScript::Compile.

C. nsXULPrototypeScript::Compile works as expected, but now tries to call
   nsXULPrototypeScript::Set on the prototype which already has a mScriptObject
   assigned because of #6 and #7 and the NS_ASSERTION fails.

*Note: chrome://global/content/contentAreaUtils.js is usually already in the
       XUL cache because browser.xul and many other chrome documents use it.


I'll attach a patch as described. Compiles and works as intended, but I'm not
sure if it might have some unpleasant side-effects I'm not aware of.

Reproducible: Always

Comment 1

[Arisu]

Attachment: Simple fix

Proposed fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, hold on.  Won't this leak just the same way if !IsOverlayAllowed?  I assume you read through bug 419846?

Comment 3

[Arisu]

(In reply to comment #2)
Sorry, I'm not very familiar with the code yet, but here are my thoughts:

If !IsOverlayAllowed, that would mean the document is neither from about nor from chrome scheme, so it's (probably) remote XUL. But ShouldLoadScript() would disallow this load attempt, so nothing should be attempting to compile and set it again... or am I mistaken here?

Or is the about scheme generally not to be trusted?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The about scheme, in general, is not trustworthy (think about:blank).

Comment 5

[Arisu]

(In reply to comment #4)
That sure makes it quite complicated and impracticable to fix this. This particular assert was frustrating me a bit because it fails so often, making the Tinderbox debug builds hard to use. I'll think this through again.

Comment 6

[Boris Zbarsky [:bzbarsky]]

We could just fix the leak; if we have a script object but don't plan to use it, we should release it correctly.  We need that no matter whether we take the first patch in this bug.

Comment 7

[Arisu]

Attachment: Second attempt

Yes, I was just too fixated on solving this by reusing the cached object for about URIs because I didn't realize what implications this has on security.

So here's a new patch that releases any present cached script object after we decided that the script isn't eligible for FastLoad in this context.
I'm not entirely sure if this is the right way to do it, especially if this might mess with the contents of the nsXULPrototypeCache, but I didn't see any problems so far.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478519 [details] [diff] [review]
Second attempt

Yes, this is exactly right.  I'm sorry it took me so long to review...  :(

We should take this for 2.0; we hit this leak on about:addons, for example.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d4e32be1a1ca

Thank you again for the patch, and again my apologies for the lag there.