Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(68)

Issue 1556963002: Avoid crash when updating stylesheets during a remove operation. (Closed)

Created:
4 years, 1 month ago by rune
Modified:
4 years, 1 month ago
Reviewers:
esprehn
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid crash when updating stylesheets during a remove operation. When we are in the middle of removing a subtree of a shadow tree containing a style element, and one of the other elements schedules style invalidation, we are synchronously trying to update rule features when the style node is still inDocument() and isInShadowTree() while the treeScope() has been reset to the document scope in preparation for removing it from the tree. That caused us to add the sheet for the style element being removed to our style data/rule features. We should make updateActiveStyleSheets asynchronous (crbug.com/567021) and schedule invalidations with the current rule features instead of forcing an update of rule features through appendPendingAuthorStyleSheets. Since updateActiveStyleSheets is currently synchronous and appendPendingAuthorStyleSheets happens lazily, we are in an inconsistent state which means we need to execute the latter in order to avoid glitches in style invalidation because we are marking for invalidation/recalc in the former step. This crasher surfaced when we started looking up the treeScope() directly in https://codereview.chromium.org/1285293003 R=esprehn@chromium.org BUG=559292 Committed: https://crrev.com/0b0f3ace8820d371e6f4b2b61354728a71ce8356 Cr-Commit-Position: refs/heads/master@{#369004}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -0 lines) Patch
A third_party/WebKit/LayoutTests/fast/css/remove-stylesheet-from-shadow-form-crash.html View 1 chunk +19 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/css/remove-stylesheet-from-shadow-form-crash-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp View 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
rune
I'm not proud of this workaround, but the issue's been nagged for a while. crbug.com/567021 ...
4 years, 1 month ago (2016-01-04 15:18:39 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1556963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1556963002/1
4 years, 1 month ago (2016-01-04 15:19:09 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 1 month ago (2016-01-04 18:10:20 UTC) #5
rune
ping
4 years, 1 month ago (2016-01-11 11:37:51 UTC) #6
esprehn
Lgtm, gross but this seems okay as a bandaid. Hopefully we don't have it for ...
4 years, 1 month ago (2016-01-12 20:41:01 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1556963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1556963002/1
4 years, 1 month ago (2016-01-12 20:42:57 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 1 month ago (2016-01-12 22:11:32 UTC) #10
commit-bot: I haz the power
4 years, 1 month ago (2016-01-12 22:12:56 UTC) #12
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/0b0f3ace8820d371e6f4b2b61354728a71ce8356
Cr-Commit-Position: refs/heads/master@{#369004}

Powered by Google App Engine
This is Rietveld 408576698