545644 - REGRESSION: HTML "nbsp" entity is not handled correctly in xHTML mode

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[nbelaevski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.78 Safari/532.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

Prototype.js framework uses innerHTML for text escapement. Regression issue in FF 3.6 make innerHTML incorrectly handle "\u00A0" symbol in xHTML mode.

Reproducible: Always

Steps to Reproduce:
Open attached file in FF 3.5.6 & FF 3.6
Actual Results:  
Error is caught in FF 3.6 and alert is shown

Expected Results:  
Error should not occur

Comment 1

[nbelaevski]

Attachment: Proposed test case

Comment 2

[Mats Palmgren (inactive)]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dfff608ebad0&tochange=b552b1ef8aa0
Likely a regression from bug 422403.

Comment 3

[Laurent Jouanneau]

the XHTML serializer now translates entities like the HTML serializer does, and not like the XML serializer does. (methods nsXHTMLContentSerializer::AppendAndTranslateEntities and nsXMLContentSerializer::AppendAndTranslateEntities in the current trunk).

So now, it converts some characters to its entity equivalent. It converts then 0xA0 characters to  .

However, the XML parser doesn't like entities in text we insert in text nodes (which is normal I guess). The HTML parser seems less strict with that, this is why we don't have the error in HTML.

Perhaps I made a mistake by translating entities in XHTML.

Henry, any thought ? You confirm that we shouldn't create entities (except for < > & "), because of the xhtml parser ?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

We'd be interested in a patch, but I don't think this blocks based on my understanding of the impact. bz, you nominated it, if you disagree please let me know why!

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #3)
> Henry, any thought ? You confirm that we shouldn't create entities (except for
> < > & "), because of the xhtml parser ?

Confirmed. Creating entity references that aren't pre-defined in XML is unsafe in Web-exposed serialization APIs when outputting any flavor of XML.

In the XMLSerializer case, I explicitly set the flags so that &nbsp; isn't emitted. Perhaps that mode should be the default so that anyone who wants to use internal APIs to emit &nbsp; (etc.) in XML needs to pass an explicit flag.

Comment 6

[Laurent Jouanneau]

Ok, I will provide a patch soon.

Comment 8

[Laurent Jouanneau]

Attachment: the patch

With this patch, the XHTML serializer does not support any more these flags: OutputEncodeBasicEntities, OutputEncodeLatin1Entities, OutputEncodeHTMLEntities and OutputEncodeW3CEntities. So it have the same behavior with entities as it had before the rewrite of the xhtml serializer.

Comment 9

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

Comment on attachment 428398 [details] [diff] [review]
the patch

>+static const PRUint16 kValNBSP = 160;
>+static const char kEntityNBSP[] = "nbsp";
Make sure the handling of these two is tested both in
HTML and XHTML innerHTML.


>+    for (aStr.BeginReading(iter);
>+          iter != done_reading;
>+          iter.advance(PRInt32(advanceLength))) {
align iter under aStr

>diff --git a/content/base/test/test_bug545644.html b/content/base/test
...
>+function testInner () {
>+  var div = document.getElementById("test_inner");
>+			
>+    try {
>+      div.innerHTML = "some \u00A0 text";
>+      div.innerHTML += "!";
>+      ok (true, "innerHTML in xhtml", "test ok");
>+    } catch (e) {
>+      ok(false, "innerHTML in xhtml", "test failed, unwanted exception "+e);
The test comments are wrong here. The file is html, not xhtml.

Comment 10

[Laurent Jouanneau]

Checked in the patch http://hg.mozilla.org/mozilla-central/rev/cc8f7dad8107, with adressed comments.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Is this bug fixed?

Comment 12

[Laurent Jouanneau]

Yes it is fixed on mozilla-central (gecko 1.9.3)

Comment 13

[Laurent Jouanneau]

Attachment: Patch for the 1.9.2 branch

the patch for gecko 1.9.2 is the same as for the trunk

Comment 14

[marx.hannah]

Will this fix be in 3.6? I checked 3.6.4 beta, and the problem persists there...

Comment 15

[Laurent Jouanneau]

Comment on attachment 433947 [details] [diff] [review]
Patch for the 1.9.2 branch

This patch fixes a serious regression in the use of innerHtml in gecko 1.9.2, so it should be applied in the 1.9.2 branch

Comment 16

[marx.hannah]

I checked Apr 28 nightly of 3.6.5pre, and the fix is not in that build either.

I guess "should be applied" is a bit ambiguous. Do you mean has already been applied, or that it will be applied sometime in the future? If it has been applied my testing says the problem is not fixed...

V 3.6.4 would be nice, but 3.6.5 should be a certainty!

Comment 17

[Laurent Jouanneau]

@marx : stop to check all versions of Firefox. When the patch will be applied (if it will) in the 3.6 branch, there will be a comment here with the url of the changeset.

I'm waiting for an approval to land it in the branch.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 433947 [details] [diff] [review]
Patch for the 1.9.2 branch

a=beltzner for mozilla-1.9.2 default

Thanks for writing tests!

Comment 19

[Laurent Jouanneau]

landed into mozilla-1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d0c1677f3e4d  (gecko 1.9.2.6)

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Verified as fixed in 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6pre) Gecko/20100624 Namoroka/3.6.6pre ( .NET CLR 3.5.30729) using testcase.