415772 - XML templates from chrome not working

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Neil Deakin]

The following template no longer works before XMLDocument::Load is throwing a security exception loading the datasource. A newsgroup poster suggests this started happening around Jan 25, which imples a regression from bug 397791.

  <hbox align="center">
    <label value="Sport:" control="sportID" width="100" />
    <menulist label="Sport" id="sportID">
      <menupopup id="sportlist" datasources="http://ic-gallery.com/sports.xml" ref="." querytype="xml">
        <template expr="sport">
          <menuitem uri="?" value="?sportID" label="?sportName" />
        </template>
      </menupopup>
    </menulist>
  </hbox>

Comment 1

[Neil Deakin]

This problem means that templates in chrome documents that use xml sources cannot load non-chrome urls. The fix here is to switch to use XMLHttpRequest to load XML rather than document.load. I have a partial fix but is depedant on 392322 as well as additional crash fixes.
 

Comment 2

[Neil Deakin]

Attachment: switch to use XMLHttpRequest to load xml

This patch incorporates the patch from bug 410812. Also needs the fix for bug 392322 to avoid crashes.

Comment 3

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

Comment on attachment 301744 [details] [diff] [review]
switch to use XMLHttpRequest to load xml

>+     * The datasource that is used for the template. This object is created by
>+     * the getDataSource method of the query processor. May be null if the
>+     * datasource has not been loaded yet. Set this attribute to use a different
>+     * datasource and rebuild the template.
>+     */
>+    attribute nsISupports datasource;
So this really needs to be nsISupports? We don't have any common interface for datasources?
At least document what kinds of object this is expected to be.

>+NS_IMETHODIMP
>+nsXULTemplateBuilder::SetDatasource(nsISupports* aResult)
>+{
>+    mDataSource = aResult;
>+    if (mCompDB)
>+        mCompDB = do_QueryInterface(mDataSource);
Could you explain why setting mCompDB only if there was already mCompDB set?

>+    nsCOMPtr<nsIXMLHttpRequest> req =
>+        do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
What keeps req alive after this method? I guess necko somehow does that via requests.
One could test if everything works properly when cycle collecting happens
before load event. Set req = nsnull and call nsJSContext::CC() (in nsJSEnvironment.h) 
just before returning from this method.

Could you add a comment somewhere that nsXULTemplateQueryProcessorXML::GetDatasource
returns always null datasource, but that the datasource will be updated after document gets 
loaded, or something like that.

>-        if (target) {
>-            target->RemoveEventListener(NS_LITERAL_STRING("load"), this, PR_FALSE);
>-        }
Why you remove this?

Comment 4

[Neil Deakin]

(In reply to comment #3)
> (From update of attachment 301744 [details] [diff] [review])
> >+     * The datasource that is used for the template. This object is created by
> >+     * the getDataSource method of the query processor. May be null if the
> >+     * datasource has not been loaded yet. Set this attribute to use a different
> >+     * datasource and rebuild the template.
> >+     */
> >+    attribute nsISupports datasource;
> So this really needs to be nsISupports? We don't have any common interface for
> datasources?
> At least document what kinds of object this is expected to be.
> 

The type is opaque to the template builder, depending on the type of query (nsIRDFDataSource, nsIDOMNode or nsIStorageConnection for the provided ones)

> Could you explain why setting mCompDB only if there was already mCompDB set?

There isn't any reason; I will change this.

> 
> >+    nsCOMPtr<nsIXMLHttpRequest> req =
> >+        do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
> What keeps req alive after this method? I guess necko somehow does that via
> requests.

Don't know, but cycle collecting as you suggest works fine. Would be good to get some input here from someone who would know why this works.

> >-        if (target) {
> >-            target->RemoveEventListener(NS_LITERAL_STRING("load"), this, PR_FALSE);
> >-        }
> Why you remove this?
> 

nsIXMLHttpRequest documentation and the implementation says the event listeners will be removed after sending. I could put the RemoveEventListener call back if desired just to be safe.

Comment 5

[Neil Deakin]

mpDB only if there was already mCompDB set?
> 
> >+    nsCOMPtr<nsIXMLHttpRequest> req =
> >+        do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
> What keeps req alive after this method? I guess necko somehow does that via
> requests.

req::Send causes an addref on itself twice. One is because it has a reference to the channel and the channel has a reference back to the xmlhttprequest, via the notification callback. Channels don't look to be cycle collected, so this circle doesn't get removed.

The other reference might be the CrossSiteListener but I'm not sure.

I think maybe I'll keep a reference to the xmlhttprequest in the query processor anyway. Also, I should add an error listener.
 

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 301744 [details] [diff] [review]
switch to use XMLHttpRequest to load xml

New patch coming?

Comment 7

[Neil Deakin]

Attachment: updated patch

Comment 8

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

Comment on attachment 302813 [details] [diff] [review]
updated patch

>+     * The opaque datasource object that is used for the template. This object
>+     * is created by the getDataSource method of the query processor. May be
>+     * null if the datasource has not been loaded yet. Set this attribute to
>+     * use a different datasource and rebuild the template.
>+     */
>+    attribute nsISupports datasource;
Would be still great to document, as an example at least, what this object possibly is.

>+NS_IMETHODIMP
>+nsXULTemplateBuilder::SetDatasource(nsISupports* aResult)
>+{
>+    mDataSource = aResult;
>+    if (mCompDB)
>+        mCompDB = do_QueryInterface(mDataSource);
You were going to change this, right?

With those, r=me. And some mochitests for this would be great.

Comment 9

[Neil Deakin]

(In reply to comment #8)
> Would be still great to document, as an example at least, what this object
> possibly is.

Sure.

> >+    mDataSource = aResult;
> >+    if (mCompDB)
> >+        mCompDB = do_QueryInterface(mDataSource);
> You were going to change this, right?

Yes, I thought I had.

> With those, r=me. And some mochitests for this would be great.

Yes, the template test framework is needed in bug 378893.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 302813 [details] [diff] [review]
updated patch

>Index: content/xul/templates/public/nsIXULTemplateBuilder.idl
>===================================================================

>+     * is maintained for backwards compatibility. It will be the same object
>+     * as the datasource property.

when datasource is an RDF datasource?

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>===================================================================

>+nsXULTemplateBuilder::SetDatasource(nsISupports* aResult)
>+{
>+    mDataSource = aResult;
>+    if (mCompDB)
>+        mCompDB = do_QueryInterface(mDataSource);

You were going to change this, right? ;-)

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
>===================================================================

>+ * Only the first datasource in aDataSource is used, which should be either an
>+ * nsIURI of an XML document, or a DOM node. If the former, GetDatasource will
>+ * load the document asynhcronously and return null in aResult. Once the

asynchronously

>     nsIScriptGlobalObject* scriptObject =
>       doc->GetScriptHandlingObject(hasHadScriptObject);

>+    req->Init(docPrincipal, context, nsnull);

I wonder whether the third parameter should be the inner window gotten from scriptObject, Smaug?

Comment 11

[Neil Deakin]

Attachment: patch for check in