Bug 42806 - Crash when CSS selector is very long.
Summary: Crash when CSS selector is very long.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 43783 43784
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-21 23:08 PDT by Hayato Ito
Modified: 2022-07-23 12:58 PDT (History)
10 users (show)

See Also:


Attachments
That would cause stack overflow (799 bytes, text/html)
2010-07-21 23:10 PDT, Hayato Ito
no flags Details
make-checkSelector-iterative (21.27 KB, patch)
2010-08-10 01:42 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2010-07-21 23:08:23 PDT
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
Comment 1 Hayato Ito 2010-07-21 23:10:03 PDT
Created attachment 62265 [details]
That would cause stack overflow
Comment 2 Hayato Ito 2010-07-21 23:12:37 PDT
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.
Comment 3 Mark Rowe (bdash) 2010-07-21 23:46:01 PDT
<rdar://problem/8220988>
Comment 4 Hayato Ito 2010-08-04 01:50:42 PDT
Is there anyone working on fix this? If no one is working, I'll investigate the issue further.
Comment 5 Hayato Ito 2010-08-10 01:42:09 PDT
Created attachment 63988 [details]
make-checkSelector-iterative
Comment 6 Hayato Ito 2010-08-10 02:00:26 PDT
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.
Comment 7 WebKit Review Bot 2010-08-10 02:02:37 PDT
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.
Comment 8 Hayato Ito 2010-08-10 04:11:35 PDT
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.
Comment 10 Hayato Ito 2010-10-19 19:32:36 PDT
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.
Comment 11 Hayato Ito 2010-10-21 19:25:11 PDT
https://bugs.webkit.org/show_bug.cgi?id=43783 is reopened. So this master bug should be also reopened.
Comment 12 Ahmad Saleem 2022-07-23 07:53:35 PDT
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.