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] Fix Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.
10 years ago
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 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 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....
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.
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.
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 22.214.171.124 and 126.96.36.199, a=dveditz for release-drivers
Checked in on both branches.
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) Gecko/20071004 BonEcho/184.108.40.206pre I now get green text and a checkbox input, while I got red text and a text input with Firefox220.127.116.11, testing: http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/394534-1.html