- Revision
- 218451
- Author
- [email protected]
- Date
- 2017-06-17 06:02:08 -0700 (Sat, 17 Jun 2017)
Log Message
Crash due to infinite recursion via FrameSelection::updateAppearanceAfterLayout
https://bugs.webkit.org/show_bug.cgi?id=173468
Reviewed by Ryosuke Niwa.
Source/WebCore:
Test: editing/selection/updateAppearanceAfterLayout-recursion.html
Calling FrameSelection::updateAppearanceAfterLayout() from Document::resolveStyle is unsafe
because it may cause another call to resolveStyle. We have some cases where the style
is still unclean when updateAppearanceAfterLayout() is called. This can lead to infinite
recursion.
The test case is not the common stack seen in CrashTracer (couldn't quit replicate it) but
the updateAppearanceAfterLayout/resolveStyle recursion is the same.
* dom/Document.cpp:
(WebCore::Document::resolveStyle):
Normally selection appearance update is done in post-layout but not all style resolutions schedule a layout.
Invoke it asynchronously in that case instead of the previous synchronous call.
* editing/FrameSelection.cpp:
(WebCore::FrameSelection::FrameSelection):
(WebCore::FrameSelection::updateAppearanceAfterLayout):
(WebCore::FrameSelection::scheduleAppearanceUpdateAfterStyleChange):
(WebCore::FrameSelection::appearanceUpdateTimerFired):
(WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange):
* editing/FrameSelection.h:
LayoutTests:
* editing/selection/updateAppearanceAfterLayout-recursion-expected.txt: Added.
* editing/selection/updateAppearanceAfterLayout-recursion.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (218450 => 218451)
--- trunk/LayoutTests/ChangeLog 2017-06-17 12:31:13 UTC (rev 218450)
+++ trunk/LayoutTests/ChangeLog 2017-06-17 13:02:08 UTC (rev 218451)
@@ -1,3 +1,13 @@
+2017-06-17 Antti Koivisto <[email protected]>
+
+ Crash due to infinite recursion via FrameSelection::updateAppearanceAfterLayout
+ https://bugs.webkit.org/show_bug.cgi?id=173468
+
+ Reviewed by Ryosuke Niwa.
+
+ * editing/selection/updateAppearanceAfterLayout-recursion-expected.txt: Added.
+ * editing/selection/updateAppearanceAfterLayout-recursion.html: Added.
+
2017-06-17 Per Arne Vollan <[email protected]>
[Win] Update expectations for layout tests.
Added: trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion-expected.txt (0 => 218451)
--- trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion-expected.txt 2017-06-17 13:02:08 UTC (rev 218451)
@@ -0,0 +1,2 @@
+edit
+target
Added: trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion.html (0 => 218451)
--- trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion.html (rev 0)
+++ trunk/LayoutTests/editing/selection/updateAppearanceAfterLayout-recursion.html 2017-06-17 13:02:08 UTC (rev 218451)
@@ -0,0 +1,17 @@
+<script>
+if (window.testRunner) {
+ internals.settings.setTelephoneNumberParsingEnabled(true);
+ testRunner.dumpAsText();
+}
+</script>
+<div id=edit>edit</div>
+<div id=target>target</div>
+<script>
+window.getSelection().selectAllChildren(edit);
+target.offsetWidth;
+const link = document.createElement("link");
+link.setAttribute("rel", "stylesheet");
+link.setAttribute("href", "data:text/css,"+Math.random());
+document.head.appendChild(link);
+location.hash = "#target";
+</script>
Modified: trunk/Source/WebCore/ChangeLog (218450 => 218451)
--- trunk/Source/WebCore/ChangeLog 2017-06-17 12:31:13 UTC (rev 218450)
+++ trunk/Source/WebCore/ChangeLog 2017-06-17 13:02:08 UTC (rev 218451)
@@ -1,3 +1,34 @@
+2017-06-17 Antti Koivisto <[email protected]>
+
+ Crash due to infinite recursion via FrameSelection::updateAppearanceAfterLayout
+ https://bugs.webkit.org/show_bug.cgi?id=173468
+
+ Reviewed by Ryosuke Niwa.
+
+ Test: editing/selection/updateAppearanceAfterLayout-recursion.html
+
+ Calling FrameSelection::updateAppearanceAfterLayout() from Document::resolveStyle is unsafe
+ because it may cause another call to resolveStyle. We have some cases where the style
+ is still unclean when updateAppearanceAfterLayout() is called. This can lead to infinite
+ recursion.
+
+ The test case is not the common stack seen in CrashTracer (couldn't quit replicate it) but
+ the updateAppearanceAfterLayout/resolveStyle recursion is the same.
+
+ * dom/Document.cpp:
+ (WebCore::Document::resolveStyle):
+
+ Normally selection appearance update is done in post-layout but not all style resolutions schedule a layout.
+ Invoke it asynchronously in that case instead of the previous synchronous call.
+
+ * editing/FrameSelection.cpp:
+ (WebCore::FrameSelection::FrameSelection):
+ (WebCore::FrameSelection::updateAppearanceAfterLayout):
+ (WebCore::FrameSelection::scheduleAppearanceUpdateAfterStyleChange):
+ (WebCore::FrameSelection::appearanceUpdateTimerFired):
+ (WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange):
+ * editing/FrameSelection.h:
+
2017-06-17 Alex Christensen <[email protected]>
Fix Mac CMake build.
Modified: trunk/Source/WebCore/dom/Document.cpp (218450 => 218451)
--- trunk/Source/WebCore/dom/Document.cpp 2017-06-17 12:31:13 UTC (rev 218450)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-06-17 13:02:08 UTC (rev 218451)
@@ -1847,8 +1847,9 @@
if (updatedCompositingLayers && !frameView.needsLayout())
frameView.viewportContentsChanged();
+ // Usually this is handled by post-layout.
if (!frameView.needsLayout())
- frameView.frame().selection().updateAppearanceAfterLayout();
+ frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
// As a result of the style recalculation, the currently hovered element might have been
// detached (for example, by setting display:none in the :hover style), schedule another mouseMove event
@@ -1858,6 +1859,8 @@
if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
frameView.scrollToFragment(m_url);
+
+ // FIXME: Ideally we would ASSERT(!needsStyleRecalc()) here but we have some cases where it is not true.
}
bool Document::needsStyleRecalc() const
Modified: trunk/Source/WebCore/editing/FrameSelection.cpp (218450 => 218451)
--- trunk/Source/WebCore/editing/FrameSelection.cpp 2017-06-17 12:31:13 UTC (rev 218450)
+++ trunk/Source/WebCore/editing/FrameSelection.cpp 2017-06-17 13:02:08 UTC (rev 218451)
@@ -111,6 +111,7 @@
, m_xPosForVerticalArrowNavigation(NoXPosForVerticalArrowNavigation())
, m_granularity(CharacterGranularity)
, m_caretBlinkTimer(*this, &FrameSelection::caretBlinkTimerFired)
+ , m_appearanceUpdateTimer(*this, &FrameSelection::appearanceUpdateTimerFired)
, m_caretInsidePositionFixed(false)
, m_absCaretBoundsDirty(true)
, m_caretPaint(true)
@@ -2397,6 +2398,22 @@
void FrameSelection::updateAppearanceAfterLayout()
{
+ m_appearanceUpdateTimer.stop();
+ updateAppearanceAfterLayoutOrStyleChange();
+}
+
+void FrameSelection::scheduleAppearanceUpdateAfterStyleChange()
+{
+ m_appearanceUpdateTimer.startOneShot(0_s);
+}
+
+void FrameSelection::appearanceUpdateTimerFired()
+{
+ updateAppearanceAfterLayoutOrStyleChange();
+}
+
+void FrameSelection::updateAppearanceAfterLayoutOrStyleChange()
+{
if (auto* client = m_frame->editor().client())
client->updateEditorStateAfterLayoutIfEditabilityChanged();
Modified: trunk/Source/WebCore/editing/FrameSelection.h (218450 => 218451)
--- trunk/Source/WebCore/editing/FrameSelection.h 2017-06-17 12:31:13 UTC (rev 218450)
+++ trunk/Source/WebCore/editing/FrameSelection.h 2017-06-17 13:02:08 UTC (rev 218451)
@@ -151,6 +151,7 @@
void prepareForDestruction();
void updateAppearanceAfterLayout();
+ void scheduleAppearanceUpdateAfterStyleChange();
void setNeedsSelectionUpdate();
bool contains(const LayoutPoint&) const;
@@ -315,6 +316,9 @@
void caretBlinkTimerFired();
+ void updateAppearanceAfterLayoutOrStyleChange();
+ void appearanceUpdateTimerFired();
+
void setCaretVisibility(CaretVisibility);
bool recomputeCaretRect();
void invalidateCaretRect();
@@ -334,6 +338,7 @@
RefPtr<EditingStyle> m_typingStyle;
Timer m_caretBlinkTimer;
+ Timer m_appearanceUpdateTimer;
// The painted bounds of the caret in absolute coordinates
IntRect m_absCaretBounds;
bool m_caretInsidePositionFixed : 1;
