Bug 137032

Summary: Chaining multiple :nth-child() does not work properly
Product: WebKit Reporter: Mikael Jergefelt <mike>
Component: CSSAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Major CC: barraclough, benjamin, kling, phiw2
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch barraclough: review+

Description Mikael Jergefelt 2014-09-23 11:12:43 PDT
nth-child ranges does not work properly in Safari on iOS8 and Mac OSX Yosemite (DP8).

Examples can be viewed at:
http://nthmaster.com

nth-child(n+4):nth-child(-n+8) and most other ranges doesn't work as they should, and select way more elements.
Comment 1 Benjamin Poulain 2014-09-24 22:10:30 PDT
This is a really really dumb bug, it is sad nobody reported it earlier. The register that keeps the element's position is reused, making its value useless when evaluating a following :nth-child().

That only happen in certain cases. It looks like we were very unlucky with our tests, only hitting the case that do not destroy the counter.
Comment 2 Benjamin Poulain 2014-09-25 17:18:49 PDT
Created attachment 238685 [details]
Patch
Comment 3 Chris Dumez 2014-09-25 17:45:28 PDT
Comment on attachment 238685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238685&action=review

> Source/WebCore/ChangeLog:13
> +        1) When A and B are possitive, the counter would be the destination of "counter - B".

nit: "positive"
Comment 4 Gavin Barraclough 2014-09-26 16:12:45 PDT
Comment on attachment 238685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238685&action=review

> Source/WebCore/cssjit/SelectorCompiler.cpp:1768
> +            if (m_registerAllocator.availableRegisterCount() > 1) {

Is this (plus twice below) "> 1" because we need to allocate more registers below (e.g. "LocalRegister divisorRegister(m_registerAllocator);")
If so, perhaps it might simplify to just hoist the further register allocation above this point?
Comment 5 Benjamin Poulain 2014-09-27 11:45:04 PDT
(In reply to comment #4)
> (From update of attachment 238685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238685&action=review
> 
> > Source/WebCore/cssjit/SelectorCompiler.cpp:1768
> > +            if (m_registerAllocator.availableRegisterCount() > 1) {
> 
> Is this (plus twice below) "> 1" because we need to allocate more registers below (e.g. "LocalRegister divisorRegister(m_registerAllocator);")
> If so, perhaps it might simplify to just hoist the further register allocation above this point?

It is indeed the reason, but moving the allocation of divisorRegister above creates more problems. Since idiv only works on RAX and RDX, I need to get those two in priority. If I were to allocate divisorRegister first, it may get RAX and RDX.
Comment 6 Benjamin Poulain 2014-09-27 11:49:26 PDT
Committed r174035: <http://trac.webkit.org/changeset/174035>