New issue
Advanced search Search tips

Issue 853709 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Reentrant appendChild causes crash in StyleEngine::MarkTreeScopeDirty

Project Member Reported by andruud@chromium.org, Jun 18 2018

Issue description

Split out from  issue 847570 .

When calling 'script.append('fail', style)', we seem to be evaluating 'fail' as a script synchronously before HTMLStyleElement::DidNotifySubtreeInsertionsToDocument happens. The onerror then modifies the HTMLStyleElement, which is in a weird state, because the previous ContainerNode::AppendChild has still not completed.

Related: https://chromiumcodereview.appspot.com/16425002

REPRO:

<script>
function start() {
  style = document.createElement('style');
  document.body.appendChild(style);

  let sr = shadowHost.createShadowRoot();
  let script = document.createElement('script');
  sr.appendChild(script);

  script.append('fail', style);
}

window.onerror = function() {
  style.appendChild(document.createTextNode(''));
}

</script>
<div id="shadowHost">
</div>
<body onload="start()"></body>
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bf635c28effa427a270f4fc45df84ed344f411c

commit 2bf635c28effa427a270f4fc45df84ed344f411c
Author: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue Jun 19 07:47:58 2018

Do not crash while reentrantly appending to style element.

When a node is inserted into a container, it is notified via
::InsertedInto. However, a node may request a second notification via
DidNotifySubtreeInsertionsToDocument, which occurs after all the children
have been notified as well. *StyleElement is currently using this
second notification.

This causes a problem, because *ScriptElement is using the same mechanism,
which in turn means that scripts can execute before the state of
*StyleElements are properly updated.

This patch avoids ::DidNotifySubtreeInsertionsToDocument, and instead
processes the stylesheet in ::InsertedInto. The original reason for using
::DidNotifySubtreeInsertionsToDocument in the first place appears to be
invalid now, as the test case is still passing.

R=futhark@chromium.org, hayato@chromium.org

Bug:  853709 ,  847570 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ic0b5fa611044c78c5745cf26870a747f88920a14
Reviewed-on: https://chromium-review.googlesource.com/1104347
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568368}
[add] https://crrev.com/2bf635c28effa427a270f4fc45df84ed344f411c/third_party/WebKit/LayoutTests/fast/dom/shadow/style-change-tree-scope.html
[modify] https://crrev.com/2bf635c28effa427a270f4fc45df84ed344f411c/third_party/blink/renderer/core/html/html_style_element.cc
[modify] https://crrev.com/2bf635c28effa427a270f4fc45df84ed344f411c/third_party/blink/renderer/core/html/html_style_element.h
[modify] https://crrev.com/2bf635c28effa427a270f4fc45df84ed344f411c/third_party/blink/renderer/core/svg/svg_style_element.cc
[modify] https://crrev.com/2bf635c28effa427a270f4fc45df84ed344f411c/third_party/blink/renderer/core/svg/svg_style_element.h

Status: Fixed (was: Started)

Sign in to add a comment