Closed Bug 394534 Opened 14 years ago Closed 14 years ago
[FIX]First attribute doesn't win in misplaced content
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.
Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M8
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"
Attachment #279208 - Flags: review?(mrbkap) → review+
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 ...?
Attachment #279208 - Flags: superreview?(peterv) → superreview+
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.
(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
Doesn't seem to crash over here....
Attachment #279208 - Flags: approval1.9? → approval1.9+
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.
Fix landed on the trunk (by manually applying the change to reftest.list as that part of the patch had bit-rotted). Marking FIXED.
Status: NEW → RESOLVED
Closed: 14 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.
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] Fix approved for 18.104.22.168 and 22.214.171.124, a=dveditz for release-drivers
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:126.96.36.199pre) Gecko/20071004 BonEcho/188.8.131.52pre I now get green text and a checkbox input, while I got red text and a text input with Firefox184.108.40.206, testing: http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/394534-1.html
You need to log in before you can comment on or make changes to this bug.