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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
HTML: Parser
P1
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({fixed1.8.0.14, verified1.8.1.8})

Trunk
mozilla1.9alpha8
x86
Linux
fixed1.8.0.14, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(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]
Fix

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)
Flags: blocking1.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M8

Updated

10 years ago
Blocks: 301375
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.
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....
Attachment #279208 - Flags: approval1.9? → approval1.9+
Flags: blocking1.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.
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.
Status: NEW → RESOLVED
Last Resolved: 10 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]
Fix

approved for 1.8.1.8 and 1.8.0.14, 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: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
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.