420609 - XMLHttpRequest no longer able to parse XHTML documents with entity references

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

See http://groups.google.com/group/mozilla.dev.apps.firefox/browse_frm/thread/edf78712691b8aa6/468737365d8b148b#468737365d8b148b

The regression range cited in that thread is http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-21+04&maxdate=2008-02-22+04&cvsroot=%2Fcvsroot

At a guess, this is a regression from bug 418119: When we try to load the XHTML DTD, the data document content policy blocks the load of the DTD.  Then we can't parse the XML, since the entities are undefined.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm not sure how common this is in the wild and in particular whether it should block the beta...

Comment 2

[Sylvain Fourmanoit]

Attachment: Exemplification page

Repost of a page affected by this, initially discussed on mozilla.dev.apps.firefox. Save locally as "test.html", as the XMLHttpRequest object relies on the name.

It can also be retrieved online:
http://xmlhttprequesttest.googlecode.com/svn/trunk/test.html

Comment 3

[Wladimir Palant]

Yes, bug 418119 should be the reason. I see a request for file:///.../Mozilla%20Firefox/res/dtd/xhtml11.dtd via content policies. From what I can see, nsDataDocumentContentPolicy blocks it. If we want to add an exception for DTDs in nsDataDocumentContentPolicy we probably want to add a dedicated type for DTD requests (similar to bug 375314).

Comment 4

[Boris Zbarsky [:bzbarsky]]

That might be the way to go, yes.

Comment 5

[Wladimir Palant]

Attachment: Proposed patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 306986 [details] [diff] [review]
Proposed patch

Looks good, but could you also assert that is($("content").textContent, reqest.responseXML.getElementById("content").textContent, "Entities didn't get expanded") ?

Comment 7

[Wladimir Palant]

Attachment: Patch v 1.1

Added additional check to the testcase - make sure the entities are actually expanded.

Comment 8

[Reed Loden [:reed]]

Checking in parser/htmlparser/src/nsExpatDriver.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsExpatDriver.cpp,v  <--  nsExpatDriver.cpp
new revision: 3.109; previous revision: 3.108
done
Checking in content/base/public/nsContentPolicyUtils.h;
/cvsroot/mozilla/content/base/public/nsContentPolicyUtils.h,v  <--  nsContentPolicyUtils.h
new revision: 3.17; previous revision: 3.16
done
Checking in content/base/public/nsIContentPolicy.idl;
/cvsroot/mozilla/content/base/public/nsIContentPolicy.idl,v  <--  nsIContentPolicy.idl
new revision: 3.10; previous revision: 3.9
done
Checking in content/base/src/nsDataDocumentContentPolicy.cpp;
/cvsroot/mozilla/content/base/src/nsDataDocumentContentPolicy.cpp,v  <--  nsDataDocumentContentPolicy.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in content/base/test/Makefile.in;
/cvsroot/mozilla/content/base/test/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
RCS file: /cvsroot/mozilla/content/base/test/test_bug420609.xhtml,v
done
Checking in content/base/test/test_bug420609.xhtml;
/cvsroot/mozilla/content/base/test/test_bug420609.xhtml,v  <--  test_bug420609.xhtml
initial revision: 1.1
done
Checking in extensions/permissions/nsContentBlocker.cpp;
/cvsroot/mozilla/extensions/permissions/nsContentBlocker.cpp,v  <--  nsContentBlocker.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in extensions/permissions/nsContentBlocker.h;
/cvsroot/mozilla/extensions/permissions/nsContentBlocker.h,v  <--  nsContentBlocker.h
new revision: 1.5; previous revision: 1.4
done

Comment 9

[Boris Zbarsky [:bzbarsky]]

If we take bug 418119 on branch, we should take this too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I do have one question about XMLHttpRequest here.  The principal for XMLHttpRequest will be the principal of the page doing the request, not the principal of the document being requested.  Is that what we want in terms of the URI we pass to content policy?

Comment 11

[Wladimir Palant]

Problem is that we probably don't want to change nsIContentPolicy on branch - meaning that we will need a different solution.

As to the URI passed to content policy - I think requestOrigin is indeed the URI that initiated the request (called XMLHttpRequest).

Comment 12

[Daniel Veditz [:dveditz]]

Not needed if bug 418119 isn't taken.