New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 677596 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

EventHandlerRegistry asserts on circular HTML import and Polymer on-tap.

Project Member Reported by calamity@chromium.org, Dec 30 2016

Issue description

Version: 57.0.2967.0

Repro:

The easiest way to trigger this as a Chromium developer is to go to chrome/browser/resources/md_history/app.html:2 and add <link rel="import" href="chrome://history/history.html">. This imports the top level page for history which imports app.html.

If you then go to chrome://history with DCHECKs/ASSERTs enabled, the renderer will Aw Snap at EventHandlerRegistry:316.

A minimized test case would be setting up a Polymer project thus:

index.html:
<!doctype html>
<head>
  <link rel="import" href="app.html">
</head>
<body>
  <test-app></test-app>
</body>
</html>

app.html:
<link rel="import" href="polymer.html">
<link rel="import" href="index.html">

<dom-module id="test-app">
  <template>
    <div on-tap="tapped">
  </template>
  <script>
    Polymer({ is: 'test-app' });
  </script>
</dom-module>


 
Cc: wjmaclean@chromium.org hayato@chromium.org dominicc@chromium.org
Owner: kochi@chromium.org
Status: Assigned (was: Untriaged)
Thank you for the great bug report; I can reproduce a check here at r444673.

kochi, could you take a look at this since there's an import at play?

CC wjmaclean FYI I think you were looking at tree scope adoption and event handler stuff recently.

[1:1:0119/170140.028485:FATAL:EventHandlerRegistry.cpp(308)] Check failed: node->document().frameHost(). 
#0 0x7f45db13bb1e base::debug::StackTrace::StackTrace()
#1 0x7f45db1a99ff logging::LogMessage::~LogMessage()
#2 0x7f45c15ae310 blink::EventHandlerRegistry::checkConsistency()
#3 0x7f45c15ae9bc blink::EventHandlerRegistry::hasEventHandlers()
#4 0x7f45c15af037 blink::EventHandlerRegistry::notifyHasHandlersChanged()
#5 0x7f45c15aee7c blink::EventHandlerRegistry::updateEventHandlerInternal()
#6 0x7f45c15af529 blink::EventHandlerRegistry::didRemoveAllEventHandlers()
#7 0x7f45c15af4cd blink::EventHandlerRegistry::didMoveOutOfFrameHost()
#8 0x7f45c1242b0c blink::Node::willMoveToNewDocument()
#9 0x7f45c12eb766 blink::TreeScopeAdopter::moveNodeToNewDocument()
#10 0x7f45c12eabc7 blink::TreeScopeAdopter::moveTreeToNewScope()
#11 0x7f45c12ea94b blink::TreeScopeAdopter::execute()
#12 0x7f45c12e4f5c blink::TreeScope::adoptIfNeeded()
#13 0x7f45c10cd6f0 blink::ContainerNode::AdoptAndAppendChild::operator()()
#14 0x7f45c10ccf50 blink::ContainerNode::insertNodeVector<>()
#15 0x7f45c10c514f blink::ContainerNode::appendChild()
#16 0x7f45c123b73a blink::Node::appendChild()
#17 0x7f45c249eb3e blink::NodeV8Internal::appendChildMethodForMainWorld()
#18 0x7f45c249e7f2 blink::NodeV8Internal::appendChildMethodCallbackForMainWorld()
#19 0x7f45cf015afb v8::internal::FunctionCallbackArguments::Call()
#20 0x7f45cf0e4713 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#21 0x7f45cf0e3250 v8::internal::Builtin_Impl_HandleApiCall()
#22 0x06c35498426e <unknown>

Received signal 6
#0 0x7f45db13bb1e base::debug::StackTrace::StackTrace()
#1 0x7f45db13b65f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f45db5aa330 <unknown>
#3 0x7f45c7503c37 gsignal
#4 0x7f45c7507028 abort
#5 0x7f45db137f16 base::debug::(anonymous namespace)::DebugBreak()
#6 0x7f45db137ef8 base::debug::BreakDebugger()
#7 0x7f45db1a9da2 logging::LogMessage::~LogMessage()
#8 0x7f45c15ae310 blink::EventHandlerRegistry::checkConsistency()
#9 0x7f45c15ae9bc blink::EventHandlerRegistry::hasEventHandlers()
#10 0x7f45c15af037 blink::EventHandlerRegistry::notifyHasHandlersChanged()
#11 0x7f45c15aee7c blink::EventHandlerRegistry::updateEventHandlerInternal()
#12 0x7f45c15af529 blink::EventHandlerRegistry::didRemoveAllEventHandlers()
#13 0x7f45c15af4cd blink::EventHandlerRegistry::didMoveOutOfFrameHost()
#14 0x7f45c1242b0c blink::Node::willMoveToNewDocument()
#15 0x7f45c12eb766 blink::TreeScopeAdopter::moveNodeToNewDocument()
#16 0x7f45c12eabc7 blink::TreeScopeAdopter::moveTreeToNewScope()
#17 0x7f45c12ea94b blink::TreeScopeAdopter::execute()
#18 0x7f45c12e4f5c blink::TreeScope::adoptIfNeeded()
#19 0x7f45c10cd6f0 blink::ContainerNode::AdoptAndAppendChild::operator()()
#20 0x7f45c10ccf50 blink::ContainerNode::insertNodeVector<>()
#21 0x7f45c10c514f blink::ContainerNode::appendChild()
#22 0x7f45c123b73a blink::Node::appendChild()
#23 0x7f45c249eb3e blink::NodeV8Internal::appendChildMethodForMainWorld()
#24 0x7f45c249e7f2 blink::NodeV8Internal::appendChildMethodCallbackForMainWorld()
#25 0x7f45cf015afb v8::internal::FunctionCallbackArguments::Call()
#26 0x7f45cf0e4713 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#27 0x7f45cf0e3250 v8::internal::Builtin_Impl_HandleApiCall()
#28 0x06c35498426e <unknown>
  r8: 00007ffd338ba700  r9: 00007f45c761da00 r10: 0000000000000008 r11: 0000000000000202
 r12: 0000000000000000 r13: 00007ffd338bd238 r14: 000026f1f94f9020 r15: 00007f45c249e7c0
  di: 0000000000000001  si: 0000000000000001  bp: 00007ffd338baaf0  bx: 00007ffd338bd1b0
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp: 00007ffd338ba9b8
  ip: 00007f45c7503c37 efl: 0000000000000202 cgf: 0000000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]

Comment 2 by kochi@chromium.org, Jan 19 2017

will take a look soon.

Comment 3 by kochi@chromium.org, Jan 20 2017

First I tried bisecting, but it seems not a recent regression, at least M56 branch also fails with this assertion.  So probably wjmaclean@'s changes
are not the culprit.  Will take further look.
Yes, I was just adding wjmaclean as FYI since I know he's been staring at this code recently.
It seems like this bug also hits the consistency check:

https://bugs.chromium.org/p/chromium/issues/detail?id=683071

It's a ClusterFuzz test case ... again not sure if it's my CL or a legacy issue, but at least it's another repro case. I've made some comments on that bug in case it helps ...

Comment 6 by kochi@chromium.org, Jan 23 2017

Thanks!  I also looked the issue there and added a comment.
Really appreciate your help.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 25 2017

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

commit c4d67ffc673ac4dc72dc2f7f7bb4d4b1735d229d
Author: kochi <kochi@chromium.org>
Date: Wed Jan 25 04:34:43 2017

Fix assertion hit on debug build for EventHandlerRegistry

The assertion checks consistency during removing the event handlers
for all the handler classes one by one, and reports inconsistency
because the handlers are partially removed.  Such inconsistency can
happen while moving a node from detached document to a main document,
found by clusterfuzz et al.

This changes the consistency check to see only modified handler class.

R=skyostil@chromium.org
BUG= 683071 ,  677596 

Review-Url: https://codereview.chromium.org/2653463003
Cr-Commit-Position: refs/heads/master@{#445944}

[modify] https://crrev.com/c4d67ffc673ac4dc72dc2f7f7bb4d4b1735d229d/third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp
[modify] https://crrev.com/c4d67ffc673ac4dc72dc2f7f7bb4d4b1735d229d/third_party/WebKit/Source/core/frame/EventHandlerRegistry.h

Comment 8 by kochi@chromium.org, Jan 25 2017

Status: Fixed (was: Assigned)
should be fixed now.

BTW, out of curiosity - calamity@, do you have any legitimate use case for importing the main HTML as sub import, or did you just find the repro accidentally?
Stumbled on it accidentally, didn't think it should crash the renderer lol.

Comment 10 by kochi@chromium.org, Jan 27 2017

Thanks, I now feel assured that this is not a viable use case :)

Comment 11 by tkent@chromium.org, Mar 15 2017

Components: -Blink>DOM>Events Blink>DOM
Remove Blink>DOM>Events

Sign in to add a comment