98295 – Selector specificity categories "overflow" into higher categories

RESOLVED FIXED Bug 98295

Selector specificity categories "overflow" into higher categories

https://bugs.webkit.org/show_bug.cgi?id=98295

Summary Selector specificity categories "overflow" into higher categories

Tab Atkins

Reported 2012-10-03 13:26:51 PDT

Our implementation of Selector specificity doesn't guard against overflow - we store the specificity in an unsigned int, using one byte each for each of the specificity groups. If you have 256 or more of a particular specificity group, it will overflow into the next highest one. This violates the Selectors spec, which defines that specificity must track the groups separately and then use a lexicographic order. The spec does now have an explicit exception allowing for UA-defined storage limits, but requires in that case that the groups clamp at their maximum. Testcase: <!doctype html> <style> #id { background-color: green; } .c0.c1.c2.c3.c4.c5.c6.c7.c8.c9.c10.c11.c12.c13.c14.c15.c16.c17.c18.c19.c20.c21.c22.c23.c24.c25.c26.c27.c28.c29.c30.c31.c32.c33.c34.c35.c36.c37.c38.c39.c40.c41.c42.c43.c44.c45.c46.c47.c48.c49.c50.c51.c52.c53.c54.c55.c56.c57.c58.c59.c60.c61.c62.c63.c64.c65.c66.c67.c68.c69.c70.c71.c72.c73.c74.c75.c76.c77.c78.c79.c80.c81.c82.c83.c84.c85.c86.c87.c88.c89.c90.c91.c92.c93.c94.c95.c96.c97.c98.c99.c100.c101.c102.c103.c104.c105.c106.c107.c108.c109.c110.c111.c112.c113.c114.c115.c116.c117.c118.c119.c120.c121.c122.c123.c124.c125.c126.c127.c128.c129.c130.c131.c132.c133.c134.c135.c136.c137.c138.c139.c140.c141.c142.c143.c144.c145.c146.c147.c148.c149.c150.c151.c152.c153.c154.c155.c156.c157.c158.c159.c160.c161.c162.c163.c164.c165.c166.c167.c168.c169.c170.c171.c172.c173.c174.c175.c176.c177.c178.c179.c180.c181.c182.c183.c184.c185.c186.c187.c188.c189.c190.c191.c192.c193.c194.c195.c196.c197.c198.c199.c200.c201.c202.c203.c204.c205.c206.c207.c208.c209.c210.c211.c212.c213.c214.c215.c216.c217.c218.c219.c220.c221.c222.c223.c224.c225.c226.c227.c228.c229.c230.c231.c232.c233.c234.c235.c236.c237.c238.c239.c240.c241.c242.c243.c244.c245.c246.c247.c248.c249.c250.c251.c252.c253.c254.c255 { background-color: red; } </style> <div id="id" class="c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 c10 c11 c12 c13 c14 c15 c16 c17 c18 c19 c20 c21 c22 c23 c24 c25 c26 c27 c28 c29 c30 c31 c32 c33 c34 c35 c36 c37 c38 c39 c40 c41 c42 c43 c44 c45 c46 c47 c48 c49 c50 c51 c52 c53 c54 c55 c56 c57 c58 c59 c60 c61 c62 c63 c64 c65 c66 c67 c68 c69 c70 c71 c72 c73 c74 c75 c76 c77 c78 c79 c80 c81 c82 c83 c84 c85 c86 c87 c88 c89 c90 c91 c92 c93 c94 c95 c96 c97 c98 c99 c100 c101 c102 c103 c104 c105 c106 c107 c108 c109 c110 c111 c112 c113 c114 c115 c116 c117 c118 c119 c120 c121 c122 c123 c124 c125 c126 c127 c128 c129 c130 c131 c132 c133 c134 c135 c136 c137 c138 c139 c140 c141 c142 c143 c144 c145 c146 c147 c148 c149 c150 c151 c152 c153 c154 c155 c156 c157 c158 c159 c160 c161 c162 c163 c164 c165 c166 c167 c168 c169 c170 c171 c172 c173 c174 c175 c176 c177 c178 c179 c180 c181 c182 c183 c184 c185 c186 c187 c188 c189 c190 c191 c192 c193 c194 c195 c196 c197 c198 c199 c200 c201 c202 c203 c204 c205 c206 c207 c208 c209 c210 c211 c212 c213 c214 c215 c216 c217 c218 c219 c220 c221 c222 c223 c224 c225 c226 c227 c228 c229 c230 c231 c232 c233 c234 c235 c236 c237 c238 c239 c240 c241 c242 c243 c244 c245 c246 c247 c248 c249 c250 c251 c252 c253 c254 c255">Test passes if this text has a green background.</div>

Attachments
Patch (10.66 KB, patch) 2012-10-03 16:00 PDT, Tab Atkins	no flags	Details Formatted Diff Diff
Patch (10.86 KB, patch) 2012-10-03 17:30 PDT, Tab Atkins	no flags	Details Formatted Diff Diff
Patch (11.02 KB, patch) 2012-10-04 11:36 PDT, Tab Atkins	no flags	Details Formatted Diff Diff
Patch for landing (11.03 KB, patch) 2012-10-04 15:49 PDT, Tab Atkins	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Tab Atkins

Comment 1 2012-10-03 13:30:27 PDT

The shortest-path change to fix this is to just add some guards to CSSSelector::specificity, in CSSSelector.cpp:51. However, that looks kinda yucky - you've lost information about which component you're adding to, so you have to check *all* of the components just to be safe. I think a better solution is to do an earlier check for whether it's a page or normal selector, and then do the loop-over-simple-selectors thing for each of them. Effectively, move the loop into CSSSelector::specificityForOneSelector(). Our whole specificity implementation is kinda bizarre anyway. There's some very odd design decisions that I need to poke at to see what their motivation was.

Tab Atkins

Comment 2 2012-10-03 16:00:30 PDT

Created attachment 166982 [details] Patch

Build Bot

Comment 3 2012-10-03 16:28:23 PDT

Comment on attachment 166982 [details] Patch Attachment 166982 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14137760 New failing tests: fast/selectors/specificity-overflow.html

Tab Atkins

Comment 4 2012-10-03 17:30:48 PDT

Created attachment 167005 [details] Patch

Ojan Vafai

Comment 5 2012-10-04 11:17:23 PDT

Comment on attachment 167005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167005&action=review > LayoutTests/fast/selectors/specificity-overflow.html:1 > +<!doctype html> /doctype/DOCTYPE > LayoutTests/fast/selectors/specificity-overflow.html:25 > +<script> Can you import js-test-pre.js here and use shouldBe()? Gives more consistency across tests and gives better output when it fails.

Tab Atkins

Comment 6 2012-10-04 11:36:33 PDT

Created attachment 167146 [details] Patch

Tab Atkins

Comment 7 2012-10-04 11:36:54 PDT

(In reply to comment #5) > (From update of attachment 167005 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167005&action=review > > > LayoutTests/fast/selectors/specificity-overflow.html:1 > > +<!doctype html> > > /doctype/DOCTYPE > > > LayoutTests/fast/selectors/specificity-overflow.html:25 > > +<script> > > Can you import js-test-pre.js here and use shouldBe()? Gives more consistency across tests and gives better output when it fails. All done!

Antti Koivisto

Comment 8 2012-10-04 12:09:01 PDT

Comment on attachment 167146 [details] Patch r=me

WebKit Review Bot

Comment 9 2012-10-04 15:15:58 PDT

Comment on attachment 167146 [details] Patch Rejecting attachment 167146 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: erging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Qt rebaseline after r130411. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/14172397

Tab Atkins

Comment 10 2012-10-04 15:49:37 PDT

Created attachment 167192 [details] Patch for landing

WebKit Review Bot

Comment 11 2012-10-04 19:04:35 PDT

Comment on attachment 167192 [details] Patch for landing Clearing flags on attachment: 167192 Committed r130444: <http://trac.webkit.org/changeset/130444>

WebKit Review Bot

Comment 12 2012-10-04 19:04:40 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Tab Atkins

Reported

2012-10-03 13:26 PDT

Modified

2012-10-04 19:04 PDT History

CC List

7 users Show

URL

Keywords

Depends on

Blocks