Skip to content

Commit

Permalink
Merge r187133 - StyleSheetContents::wrapperInsertRule() can create ru…
Browse files Browse the repository at this point in the history
…les that overflow RuleData's selector index

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

Patch by Benjamin Poulain <bpoulain@apple.com> on 2015-07-21
Reviewed by Alex Christensen.

Source/WebCore:

RuleData identifies selectors by the index in a large array. The index only has 13 bits
so rules with more than 8192 selectors should be split.

One of the paths was not splitting the rule: StyleSheetContents::wrapperInsertRule().
When rules with too many selectors were added, the index would overflow and
some RuleData would point to selectors in the middle of selector chains. The resulting
behavior is random based on the selectors and the DOM.

We cannot easily fix that because the CSS OM API do not expect to create
several rules in response to calls to the API.
In this patch, I don't do anything fancy and just let the calls fail
if we cannot use the rules safely.

Content Extensions were also running into this problem. Large Selector lists are
pretty common, and ContentExtensionStyleSheet::addDisplayNoneSelector() was
overflowing the RuleData, creating broken page.

Unlike CSSOM, there is no problem with splitting rules coming from Content Extensions.
Instead of creating new APIs for that case, I rely on the parser to extend the StyleSheetContents.
That code already knows how to break rules correctly.

Tests: fast/css/insert-rule-overflow-rule-data.html
       http/tests/contentextensions/css-display-none-overflows-rule-data-1.html
       http/tests/contentextensions/css-display-none-overflows-rule-data-2.html

* contentextensions/ContentExtensionStyleSheet.cpp:
(WebCore::ContentExtensions::ContentExtensionStyleSheet::addDisplayNoneSelector):
* css/StyleSheetContents.cpp:
(WebCore::StyleSheetContents::wrapperInsertRule):

LayoutTests:

This bug was affecting two parts of WebKit:
-In CSSOM, StyleSheet.insertRule() could create bogus rules.
 The new test verifies that the call fails instead of creating undefined
 behaviors.
-In ContentExtensions, large selectors are now working correctly. The tests
 cover the case of a default stylesheet and an dynamic stylesheet.

* fast/css/insert-rule-overflow-rule-data-expected.txt: Added.
* fast/css/insert-rule-overflow-rule-data.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6.html.json: Added.
  • Loading branch information
Benjamin Poulain authored and carlosgcampos committed Aug 4, 2015
1 parent 9bc33b0 commit 01aadef
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 1 deletion.
35 changes: 35 additions & 0 deletions LayoutTests/ChangeLog
@@ -1,3 +1,38 @@
2015-07-21 Benjamin Poulain <bpoulain@apple.com>

StyleSheetContents::wrapperInsertRule() can create rules that overflow RuleData's selector index
https://bugs.webkit.org/show_bug.cgi?id=147144

Reviewed by Alex Christensen.

This bug was affecting two parts of WebKit:
-In CSSOM, StyleSheet.insertRule() could create bogus rules.
The new test verifies that the call fails instead of creating undefined
behaviors.
-In ContentExtensions, large selectors are now working correctly. The tests
cover the case of a default stylesheet and an dynamic stylesheet.

* fast/css/insert-rule-overflow-rule-data-expected.txt: Added.
* fast/css/insert-rule-overflow-rule-data.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-1.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-2.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-3.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-4.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-5.html.json: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6-expected.txt: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6.html: Added.
* http/tests/contentextensions/css-display-none-overflows-rule-data-6.html.json: Added.

2015-07-21 Simon Fraser <simon.fraser@apple.com>

Safari mis-applies "animation-fill-mode: forwards" when using fractional iteration count
Expand Down
24 changes: 24 additions & 0 deletions LayoutTests/fast/css/insert-rule-overflow-rule-data-expected.txt

Large diffs are not rendered by default.

50 changes: 50 additions & 0 deletions LayoutTests/fast/css/insert-rule-overflow-rule-data.html

Large diffs are not rendered by default.

38 changes: 38 additions & 0 deletions Source/WebCore/ChangeLog
@@ -1,3 +1,41 @@
2015-07-21 Benjamin Poulain <bpoulain@apple.com>

StyleSheetContents::wrapperInsertRule() can create rules that overflow RuleData's selector index
https://bugs.webkit.org/show_bug.cgi?id=147144

Reviewed by Alex Christensen.

RuleData identifies selectors by the index in a large array. The index only has 13 bits
so rules with more than 8192 selectors should be split.

One of the paths was not splitting the rule: StyleSheetContents::wrapperInsertRule().
When rules with too many selectors were added, the index would overflow and
some RuleData would point to selectors in the middle of selector chains. The resulting
behavior is random based on the selectors and the DOM.

We cannot easily fix that because the CSS OM API do not expect to create
several rules in response to calls to the API.
In this patch, I don't do anything fancy and just let the calls fail
if we cannot use the rules safely.


Content Extensions were also running into this problem. Large Selector lists are
pretty common, and ContentExtensionStyleSheet::addDisplayNoneSelector() was
overflowing the RuleData, creating broken page.

Unlike CSSOM, there is no problem with splitting rules coming from Content Extensions.
Instead of creating new APIs for that case, I rely on the parser to extend the StyleSheetContents.
That code already knows how to break rules correctly.

Tests: fast/css/insert-rule-overflow-rule-data.html
http/tests/contentextensions/css-display-none-overflows-rule-data-1.html
http/tests/contentextensions/css-display-none-overflows-rule-data-2.html

* contentextensions/ContentExtensionStyleSheet.cpp:
(WebCore::ContentExtensions::ContentExtensionStyleSheet::addDisplayNoneSelector):
* css/StyleSheetContents.cpp:
(WebCore::StyleSheetContents::wrapperInsertRule):

2015-07-21 Tim Horton <timothy_horton@apple.com>

Placing video in fullscreen caused WebKit crash at WebCore::Range::textQuads
Expand Down
6 changes: 5 additions & 1 deletion Source/WebCore/css/StyleSheetContents.cpp
Expand Up @@ -233,7 +233,11 @@ bool StyleSheetContents::wrapperInsertRule(PassRefPtr<StyleRuleBase> rule, unsig
if (is<StyleRuleImport>(*rule))
return false;
childVectorIndex -= m_importRules.size();


// If the number of selectors would overflow RuleData, we drop the operation.
if (is<StyleRule>(*rule) && downcast<StyleRule>(*rule).selectorList().componentCount() > RuleData::maximumSelectorComponentCount)
return false;

m_childRules.insert(childVectorIndex, rule);
return true;
}
Expand Down

0 comments on commit 01aadef

Please sign in to comment.