!current.value().isInheritedValue() in StyleResolver.cpp |
||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=5310567831306240 Fuzzer: inferno_twister Job Type: linux_debug_content_shell_drt Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: !current.value().isInheritedValue() in StyleResolver.cpp blink::StyleResolver::applyProperties<> blink::StyleResolver::applyMatchedProperties<> Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=268656:269696 Minimized Testcase (0.43 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94MneXmu9l7khCUXN_7UaJfSb4Gwp-0dt4ftkPTB3W6CqpbOMjRsWmuyTqZrLhOG6m97fA4GszOw7QKMGFKjqqZ6mdDPIl4fqt9G1GNm9aJTyuWuyEzHWxkfv4X_Ng4oCoD21AEPERVuJXTGXqhYs1VWjaJ3Q?testcase_id=5310567831306240 Issue manually filed by: mmohammad See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Aug 11 2016
My change is a basically a rename (no-op) just adding const in places so fairly confident it didn't cause this, it could be from https://chromium.googlesource.com/chromium/blink/+/b5e841c078dec293187d3f3e54fc3be65d7dae11 but its unclear. andersr@opera.com could you ptal? It's hard to bisect from pre-blink-merge, I've set clusterfuzz to re-bisect to hopefully get some better results.
,
Aug 11 2016
,
Aug 11 2016
style rules in the minimized test case can be made more specific as:
html { stop-opacity: 1; }
body { perspective: inherit; }
,
Aug 15 2016
,
Aug 16 2016
It happens because we erroneously cache explicitly inherited properties in some cases.
Consider the following minimal example (which does not assert, but does make the wrong thing happen internally):
<!DOCTYPE html>
<script>
document.documentElement.createShadowRoot();
</script>
<style>
body { perspective: inherit; }
</style>
1. <html> has a shadow root.
2. Style for <body> is calculated. (Document::inheritHtmlAndBodyElementStyles)
3. ElementResolveContext for <body> has no parent node, since <body> is not distributed.
4. Style applicator does not setHasExplicitlyInheritedProperties on parent style, since we have no parent node. (StyleBuilder::applyProperty)
5. Result: With nothing to prevent it, the properties of <body> end up in the matched properties cache.
Then, on the next cache hit, we applyInheritedOnly, and hit the no-inherited-value assert.
Not sure what the best solution is, at the moment. Possibly just not caching properties if there is no parent node?
For the record this has nothing to do with b5e841c078dec293187d3f3e54fc3be65d7dae11.
,
Aug 18 2016
Thanks for investigating andersr! That's a really useful result. I have a few suggestions for a solution: 1. Why is setHasExplicitlyInheritedProperties not set on the current element's style when 'inherit' is used? It feels StyleBuilder::applyProperty should set this on the current style as well as the parent style. 2. Or at least the matched properties cache should check if ComputedStyle has any properties with the value 'inherit', and if so, not cache it 3. I like the idea of not caching styles if there is no parent node, but this could affect the performance of certain pages. I'm thinking of the situation where you create a bunch of identical elements in JS before adding them to the DOM, really unless they have 'inherit' as any of the values we can safely share their styles.
,
Aug 18 2016
I think if there's no insertion point we should probably not cache the style, and should probably abort computing it as well.
setHasExplicitlyInheritedProperties is for the parent of the things that use inherit, not the thing itself, otherwise all style changes on the thing would result in a subtree style change.
ex.
<a>
<b> border-width: inherit;
<c1 />
<c2 />
<c3 />
We want to setHasExplicitlyInheritedProperties() on <a> so style changes for border-width on <a> get propagated down to <b>. We *don't* want to set it on <b> though, otherwise any property change (ex. padding, border-width, etc.) on <b> forces a reacalc of c1-c3.
How does this work if the parent is display: none? :) The explicit inheritance is on the ComputedStyle, should it be in the ElementStyleFlags instead?
,
Aug 18 2016
This is not a security issue, we're just hitting a correctness assert about styled things.
,
Aug 19 2016
,
Aug 26 2016
,
Aug 26 2016
,
Sep 2 2016
,
Sep 3 2016
ClusterFuzz has detected this issue as fixed in range 415934:416233. Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5310567831306240 Fuzzer: inferno_twister Job Type: linux_debug_content_shell_drt Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: !current.value().isInheritedValue() in StyleResolver.cpp blink::StyleResolver::applyProperties<> blink::StyleResolver::applyMatchedProperties<> Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=268656:269696 Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=415934:416233 Minimized Testcase (0.43 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94MneXmu9l7khCUXN_7UaJfSb4Gwp-0dt4ftkPTB3W6CqpbOMjRsWmuyTqZrLhOG6m97fA4GszOw7QKMGFKjqqZ6mdDPIl4fqt9G1GNm9aJTyuWuyEzHWxkfv4X_Ng4oCoD21AEPERVuJXTGXqhYs1VWjaJ3Q?testcase_id=5310567831306240 See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mmohammad@chromium.org
, Aug 10 2016Status: Assigned (was: Untriaged)