394534 - [FIX]First attribute doesn't win in misplaced content

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Problem is also mentioned at http://sla.ckers.org/forum/read.php?2,15336 and http://ha.ckers.org/blog/20070830/overwriting-attributes/

Fix is coming up.  Writing tests now.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 279208 [details] [diff] [review]
Fix

Index: parser/htmlparser/src/CNavDTD.cpp
+          // XXXbz would it be faster to get the tokens out with ObjectAt and
+          // the PopFront them all?

"then"

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 279208 [details] [diff] [review]
Fix

>Index: parser/htmlparser/src/CNavDTD.cpp
>===================================================================

>+          // XXXbz would it be faster to get the tokens out with ObjectAt and
>+          // the PopFront them all?

then PushFront ...?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279208 [details] [diff] [review]
Fix

No, "then PopFront".  Maybe I should say "then PopFront them all from mMisplacedContent".

Requesting approval for trunk and branches.  This is bug apparently being used for content-injection attacks...  Risk is as low as it gets in parser code, which is probably "medium to large", so we might want to bake on the trunk for a week or so before landing on the branches.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> (From update of attachment 279208 [details] [diff] [review])
> No, "then PopFront".  Maybe I should say "then PopFront them all from
> mMisplacedContent".

Ah, and use PushTokenFront again? (which is what I meant instead of PushFront)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right.  That approach would be to ObjectAt() and PushTokenFront() starting at the _end_ of the things I want to move, then pop them all off.

Comment 7

[Martijn Wargers (dead)]

You might want to check if this piece of html:
<table><input type="password">
crashes with the patch.
It seems to crash in my debug build, but I might have done something wrong.

It seems to crash at:
    CToken* theAttrToken = theAttrNode.PopAttributeTokenFront();
in PushMisplacedAttributes

Comment 8

[Boris Zbarsky [:bzbarsky]]

Doesn't seem to crash over here....

Comment 9

[Daniel Veditz [:dveditz]]

Would be nice to fix on the branch, but probably not a stop-ship. We'll look at approvals after it's landed on the trunk.

Comment 10

[Johnny Stenback (:jst)]

Fix landed on the trunk (by manually applying the change to reftest.list as that part of the patch had bit-rotted). Marking FIXED.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

It looks like the test file itself didn't land, so I reverted the reftest.list change to fix the orange.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I checked the test in now that I was able to verify that it passed.

Comment 13

[Johnny Stenback (:jst)]

Duh, must have forgotten to cvs add that file. Thanks for dealing with it!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in a followup patch addressing the review comments too.

Comment 15

[Martijn Wargers (dead)]

Apparently there is something wrong with my vc build, so you can just ignore comment 7 any further (which you already did).

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 279208 [details] [diff] [review]
Fix

approved for 1.8.1.8 and 1.8.0.14, a=dveditz for release-drivers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to branch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in on both branches.

Comment 19

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

I now get green text and a checkbox input, while I got red text and a text input with Firefox2.0.0.7, testing:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/394534-1.html