Crash when CSS selector is very long.

• Component: CSS
• Status: NEW ---

[Hayato Ito]

Take a look at the attached html. That would cause stack overflow like:


#0  0x00007ffff5b204a2 in WebCore::CSSSelector::hasTag (this=Cannot access memory at address 0x7fffff5aeff8
) at ../../../../webkit3/WebKit/WebCore/css/CSSSelector.h:238
#1  0x00007ffff5b29613 in WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:2078
#2  0x00007ffff5b28acb in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1911
#3  0x00007ffff5b28e6a in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1972
#4  0x00007ffff5b28e6a in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045fc0, e=0x3ee6f30, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1972

[Hayato Ito]

Created attachment 62265 [details]
That would cause stack overflow

[Hayato Ito]

https://bugs.webkit.org/show_bug.cgi?id=41129 is a similar bug which was already fixed.

We have to use an iterative approach instead of recursive one in order to avoid stack overflow.

I found the following functions use recursion:

- CSSSelector::specifity()
- CSSStyleSelector::SelectorChecker::checkSelector()

We need to investige further. There might be other functions which use recursion.

[Mark Rowe (bdash)]

<rdar://problem/8220988>

[Hayato Ito]

Is there anyone working on fix this? If no one is working, I'll investigate the issue further.

[Hayato Ito]

Created attachment 63988 [details]
make-checkSelector-iterative

[Hayato Ito]

I've made CSSStyleSelector::SelectorChecker::checkSelector() iterative to avoid stack overflow.
To achieve it, I have to maintain CallStack manually.

Hi reviewers,

I am afraid the code became unintuitive. But I guess it is inevitable. If you have any ideas to make it clean and intuitive, please let me know.

I also have a concern about performance. Is there any standard way to make sure a performance degradation won't happen by this change?


I'll fix CSSSelector:specifity() in another patch later. That might be an pretty easy task.

[WebKit Review Bot]

Attachment 63988 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1999:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/css/CSSStyleSelector.cpp:2092:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hayato Ito]

I've canceled the review request.
It might be better to make a couple of bugs to fix them separately step by step.

I'd like to use this bug as a master bug to manage them.

[Hayato Ito]

I've filed the bugs:
- https://bugs.webkit.org/show_bug.cgi?id=43783
- https://bugs.webkit.org/show_bug.cgi?id=43784

[Hayato Ito]

I'll close this bug because all sub bugs are now fixed.
If you find any other cases which causes a crash, please reopen this.

[Hayato Ito]

https://bugs.webkit.org/show_bug.cgi?id=43783 is reopened. So this master bug should be also reopened.

[Ahmad Saleem]

This test case is still crashing tab in Safari 15.6 on macOS 12.5 and after trying multiple refresh, you will notice that the banner "This web page..." starts to render slowly like 15 fps and does not show quickly before change the error to page body like "A problem ...".

I think it might be good test case to improve "banner" loading performance of tab crash.