573969 - innerHTML getter on <xmp> is no longer special-cased, causing tuenti.com to display unparsed markup

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Guillermo López :willyaranda (probably SLOW response)]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; es-ES; rv:1.9.2.4) Gecko/20100611 Firefox/3.6.4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; es-ES; rv:1.9.2.4) Gecko/20100611 Firefox/3.6.4

It's a regression between 21st of June 2010 and 22nd, range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7eaf9387b1f&tochange=ce18b4287791

Reproducible: Always

Steps to Reproduce:
1. Log in 
2. You'll see HTML code instead of the parsed one.
3.

Comment 1

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Screenshot

Comment 2

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Screenshot #2

Comment 3

[Guillermo López :willyaranda (probably SLOW response)]

Juan, could you investigate this problem? We have a user/pass to test

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #3)
> Juan, could you investigate this problem? We have a user/pass to test

Guillermo, can you please send me those data in a private message? Thanks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I, too, would appreciate the login/pass; I should be able to bisect with that.

Comment 7

[Guillermo López :willyaranda (probably SLOW response)]

I've sent you an email to your bugzilla emails, if you want it on other mail, please, tell me.

Thanks

Comment 8

[Boris Zbarsky [:bzbarsky]]

The first bad revision is:
changeset:   43929:11aa64d1d612
user:        Mounir Lamouri <[e-mail]>
date:        Tue Jun 22 02:02:07 2010 +0200
summary:     Bug 568515 - xmp and plaintext element should be considered as HTMLElement, not HTMLSpanElement, r=sicking

I verified via local backout that it's the innerHTML changes that are responsible.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Looks like the page uses <xmp> with display:none or something as template containers, and uses .innerHTML to get the template text.  Since we stopped special-casing the innerHTML getter on <xmp> but still special-case parsing, if you have:

 <xmp><div></div></xmp>

and get the innerHTML of the <xmp>, you get "&lt;div&gt;&lt;/div&gt;" instead of "<div></div>".

I just tried Chrome, Safari, Opera, and IE8 on that testcase, all in standards mode.  All of those hand back "<div></div>".  Sounds to me like we need to undo at least the change to our innerHTML getter, and get the spec changed to match reality.

Comment 10

[Anne (:annevk)]

http://www.whatwg.org/html#html-fragment-serialization-algorithm has special cases for style, script, xml, etc. This is not something that changed recently. Someone must have misunderstood the specification. Might be good to get a clarification where it went wrong so the specification can be improved.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Indeed.  It looks like no one in bug 568515 bothered to worry about the getter, just the setter.

Comment 12

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

Tuenti.com is the most important social network in Spain (8+ million users), it would be great to have this regression fixed for Firefox 4 betas ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Is this serious enough that it needs to block beta1?  Because if so, it needs a different owner...

Comment 14

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

At least in Spain, it would be a big marketing issue to test Firefox 4 beta and see that one of the most popular sites does not work.

Comment 15

[Mateo Matachana López]

(In reply to comment #13)
> Is this serious enough that it needs to block beta1?  Because if so, it needs a
> different owner...

Judge for yourself: http://www.alexa.com/siteinfo/tuenti.com

Tuenti is the 10th (410 worldwide) in the Alexa rank, so I think yes, it should block beta1.

Comment 16

[Damon Sicore (:damons)]

Is anyone working on a patch for this?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Not yet, but I can write one up if we want this for b1...

Comment 18

[Guillermo López :willyaranda (probably SLOW response)]

Well, I think the patch should be on beta 1, per comment 14 and comment 15 (and because Mozilla want a lot of feedback on the betas of Firefox 4 and we have planned some things to encourage users to test the new version).

Just my 0,02€, but I'm not the developer...

Comment 19

[Boris Zbarsky [:bzbarsky]]

The commenters in comment 14 and 15 don't get to set my priorities (in the "drop everything else, no matter how important, and work on this right now" sense)....  If drivers decide we want this, thought, that's a different story.

Comment 20

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

For b1 couldn't we just backout  Bug 568515?

Comment 21

[Boris Zbarsky [:bzbarsky]]

If we're ok with that, then that would be the easy way, yes.

Comment 22

[Guillermo López :willyaranda (probably SLOW response)]

FYI, we have at least one report in our community forum:

http://www.mozilla-hispano.org/foro/viewtopic.php?f=37&t=8496 (in spanish, but believe me, it's about this bug ;) )

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Arg, I thought I had posted this already but apparently I forgot to push 'commit'.

I talked with drivers about this on the 30th and the 1st, and consensus was that it was too late to sneak this one in.

However hopefully we can relnote this bug though, as to assure people that this is a known problem that will be fixed.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ok, then let's make sure we fix this for next beta.

Comment 25

[Guille (bisho)]

Please, be sure this is included in next beta. Tuenti is easily #1 in Spain despite Alexa (that doesn't track well ajax and invitation only website), with ~25 billion page views per month, and a lot of our users use Firefox.

If you need any help or know possible workarounds for <xmp> as template container, please just tell us.

Comment 26

[Boris Zbarsky [:bzbarsky]]

One workaround is to use .textContent instead of .innerHTML (which should be the same thing for <xmp>)...  The main issue with that is that IE doesn't support .textContent, at least in old enough versions.

Comment 27

[Mounir Lamouri (:mounir)]

I'm back from vacation. I'm going to write a patch for that ASAP.
Sorry for the delay.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I've restored GetInnerHTMTL only.

Comment 29

[Boris Zbarsky [:bzbarsky]]

We should file a followup bug on doing this the way the spec calls for (i.e. in the serializer).

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 456891 [details] [diff] [review]
Patch v1

>+nsHTMLElement::GetInnerHTML(nsAString& aInnerHTML)
>+{
>+  /**
>+   * nsGenericHTMLElement::GetInnerHTML escapes < and > characters (at least).
>+   * .innerHTML should return the HTML code for xmp and plaintext element.
>+   * See bug 573969.
>+   */

Remove the reference to the bug#. We generally don't put those in as it would result in way too many bug references. You can always find the bug# through hg-log. You might instead want to say that this behavior is what the HTML5 spec calls for, given that it's somewhat unexpected.

r=me with that and assuming that you've checked that this is the behavior the HTML5 spec calls for.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, didn't see comment 29. Hmm.. I guess it's ok that we fix this in a non-specified way until we actually implement a HTML5 serializer.

Comment 32

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

r=sicking

Comment 33

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/41f8758b72f0

Comment 34

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

Confirmed, tuenti.com loads correctly using

Mozilla/5.0 (X11; Linux i686; es-ES; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Comment 35

[Guille -bisho-]

Tuenti works fine for me too.

Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Comment 36

[rosadoivor]

Using

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

Tuenti keeps freezing at 90% - 91% when loading, this issue didn´t occur with the 6th beta, I´m guessing something broke again.

Comment 37

[Guillermo López :willyaranda (probably SLOW response)]

comment 36: that was bug 604612 which is fixed now and will be shipped in a near beta!! (a few days, I hope).