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

RESOLVED FIXED in mozilla1.9alpha8



12 years ago
11 years ago


(Reporter: bzbarsky, Assigned: bzbarsky)


({fixed1.8.0.14, verified1.8.1.8})

fixed1.8.0.14, verified1.8.1.8
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: baking on trunk (9/10), URL)


(2 attachments)

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.
Created attachment 279208 [details] [diff] [review]

Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.
Attachment #279208 - Flags: superreview?(peterv)
Attachment #279208 - Flags: review?(mrbkap)


12 years ago
Flags: blocking1.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M8


12 years ago
Blocks: 301375
Comment on attachment 279208 [details] [diff] [review]

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

Attachment #279208 - Flags: review?(mrbkap) → review+
Comment on attachment 279208 [details] [diff] [review]

>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 ...?
Attachment #279208 - Flags: superreview?(peterv) → superreview+
Comment on attachment 279208 [details] [diff] [review]

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.
Attachment #279208 - Flags: approval1.9?
Attachment #279208 - Flags: approval1.8.1.7?
Attachment #279208 - Flags: approval1.8.0.14?
(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)
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.
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
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
Doesn't seem to crash over here....
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.9?
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
Fix landed on the trunk (by manually applying the change to reftest.list as that part of the patch had bit-rotted). Marking FIXED.
Last Resolved: 12 years ago
Flags: blocking1.9? → blocking1.9+
Resolution: --- → FIXED
It looks like the test file itself didn't land, so I reverted the reftest.list change to fix the orange.
Flags: in-testsuite?
Whiteboard: baking on trunk (9/10)
I checked the test in now that I was able to verify that it passed.
Flags: in-testsuite? → in-testsuite+
Duh, must have forgotten to cvs add that file. Thanks for dealing with it!
Checked in a followup patch addressing the review comments too.
Apparently there is something wrong with my vc build, so you can just ignore comment 7 any further (which you already did).
Comment on attachment 279208 [details] [diff] [review]

approved for and, a=dveditz for release-drivers
Attachment #279208 - Flags: approval1.8.1.8?
Attachment #279208 - Flags: approval1.8.1.8+
Attachment #279208 - Flags: approval1.8.0.14?
Attachment #279208 - Flags: approval1.8.0.14+
Created attachment 282270 [details] [diff] [review]
Merged to branch
Checked in on both branches.
Keywords: fixed1.8.0.14, fixed1.8.1.8
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20071004 BonEcho/

I now get green text and a checkbox input, while I got red text and a text input with Firefox2.0.0.7, testing:
Keywords: fixed1.8.1.8 → verified1.8.1.8
You need to log in before you can comment on or make changes to this bug.