Bug 136906 – CSS JIT: minimumRegisterRequirements() can be one register short for :nth-child()

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

CSS JIT: The backtracking register can be ignored from the minimumRegisterRequirements

[Benjamin Poulain]

Created attachment 238286 [details]
Patch

[Benjamin Poulain]

Yusuke, could you please double check I am not missing any case?

I'll add more testing infrastructure tomorrow but I would like multiple reviews on this.

[Benjamin Poulain]

rdar://problem/18368294

[Yusuke Suzuki]

(In reply to comment #2)
> Yusuke, could you please double check I am not missing any case?
> 
> I'll add more testing infrastructure tomorrow but I would like multiple reviews on this.

Sure. I'll check it now :)

[Yusuke Suzuki]

Comment on attachment 238286 [details]
Patch

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

Maybe I don't understand the issue's background enough, sorry, I can't see the rdar link...

If I understand this issue correctly, I think there's 2 meanings. Please point it :)

1.
When there's no :not, :-webkit-any or attributes, `backtrackingRegisterRequirements` is not added to the minimum requirements.

However, seeing the comment on `minimumRequiredRegisterCount`, it contains `BacktrackingRegister`. So I think that `BacktrackingRegister` count is already added, right?

2.
`BacktrackingRegister` count is always added to the minimum register requirements whenever it isn't used. I think this pointing is right. So to fix it,

1) changing `minimumRequiredRegisterCount` to 5, it doesn't contains `BacktrackingRegister`.
2) defining `minimum` as `minimumRequiredRegisterCount + backtrackingRegisterRequirements` after the `InChainWithDescendantTail` check. OR adding `backtrackingRegisterRequirements` to `minimum` inside the `InChainWithDescendantTail` check. (like your change!)

Is it correct?

> Source/WebCore/cssjit/SelectorCompiler.cpp:859
> +        minimum += minimumRequiredRegisterCount;

I think it would be `+= backtrackingRegisterRequirements`. Is it correct?

[Benjamin Poulain]

Comment on attachment 238286 [details]
Patch

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

>> Source/WebCore/cssjit/SelectorCompiler.cpp:859
>> +        minimum += minimumRequiredRegisterCount;
> 
> I think it would be `+= backtrackingRegisterRequirements`. Is it correct?

Arg, ffs. I did a last minute change and I messed up the copy-paste.

I am glad I asked you for a review.

[Benjamin Poulain]

Created attachment 238289 [details]
Patch

[Benjamin Poulain]

You are right, this is wrong as you pointed out, minimumRequiredRegisterCount already account for the backtracking register.

Looks like :nth-child() uses 6 registers in some path then. I'll look into it and upload a new patch asap.

[Benjamin Poulain]

Created attachment 238292 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238292 [details]
Patch

The patch is for the branches. ToT will get a different patch, I'll work that out tomorrow.

The test is for ToT and the branches.

[Benjamin Poulain]

Committed r173730: <http://trac.webkit.org/changeset/173730>

[Benjamin Poulain]

Closing:

Patch submitted in the branch.
Test submitted in ToT.

Added some code to catch such case earlier next time: https://bugs.webkit.org/show_bug.cgi?id=136933

ToT does not need the extra register. There are two patch of maximum allocation:
1) currentElement
2) elementCounter
3) previousSibling
4) elementRareData
5) cachedChildIndex
and
1) currentElement
2) elementCounter
3) checkingContext
4) childStyle
5) flags
6) isFirstChildStateFlagImmediate

The second path is safe for now because it is only executed on for the rightmost element. By definition, it cannot have a backtracking register.