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

Issue 683071 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

node->document().frameHost() in EventHandlerRegistry.cpp

Project Member Reported by ClusterFuzz, Jan 20 2017

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5457387516592128

Fuzzer: inferno_twister
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  node->document().frameHost() in EventHandlerRegistry.cpp
  blink::EventHandlerRegistry::checkConsistency
  blink::EventHandlerRegistry::hasEventHandlers
  
Sanitizer: address (ASAN)

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=306806:306813

Minimized Testcase (0.63 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94f4WFmK7wAfuvZGWxWWHNCFTtmkkLY6B1NMk30OIWBrNUs-UkHUalJwJVOymM_3VLenX-awkAupxr4DOYf8iFkLfg9-FOt5RjW72gtwPE-T3ZOKhXnZUrFD8f0f8J5b3frGm2R-A1E8WvWfzE1PFOvAbtD_wMEZCH1HCp8f5gI2RpbgbDWQTtARN6lImXyg27DCg54c8VmXfSgB4ikItlLPOCKsLEMGLWew-rU_VO-izr9VvawtlOuUGOMQdKRTaENgcFOOP1XnKW1s_xmyo0n8V761YBHLrxg6Sx2gleU3rxTwUuRKVWpBNN0PaMPd1b2r8a_VCsPyxerPhB-jf_s2kxcCkxFh6k9QwGOxl0iiamTz24?testcase_id=5457387516592128

Additional requirements: Requires HTTP

Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: msrchandra@chromium.org
Labels: Test-Predator-Wrong
Owner: wjmaclean@chromium.org
Status: Assigned (was: Untriaged)
Find it and CL did not provide any possible suspects.
Using Code Search for the file, "EventHandlerRegistry.cpp" assigning to the concern owner.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/95a1418e25fe0ef90aa7eced41f73e3e7db1f22c

@wjmaclean -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.
Cc: dominicc@chromium.org
I saw these kind of errors when developing the patch ... they seemed to be related to V8 accessing the registry when the node is not attached to a frame:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp?rcl=1484900213&l=308

I thought I had solved this problem through placing the changes to the registry in close enough proximity that there wasn't any opportunity for JS to run in between, but perhaps in the test case (which I haven't tried manually yet) JS does get to run.

That would suggest somewhere between these lines of code we're seeing JS attempt to access the event registry:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp?rcl=0&l=144

and

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp?rcl=0&l=156

But it seems weird, as presumably the event handler has been moved off the node already, unless some other target node is in play here.

I'll take a look and see if I can figure out what's happening.

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

Cc: kochi@chromium.org

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

Unlike  issue 677596 , this doesn't use HTMLImports (but uses another Document).
I took a look at the minimized repro case, and manually reduced further:

---------
<div id=x></div>
<script>
x.addEventListener("touchstart", ()=>{});
x.addEventListener("touchend", ()=>{});
document.implementation.createHTMLDocument().adoptNode(x);
</script>
---------

The condition for the crash looks like a node has more than one
event handlers, which have different classes according to
EventHandlerRegistry::eventTypeToClass().  E.g. touchstart and
touchend are classified to different classes, and reproduces
the crash, while touchstart and touchmove doesn't.

At least for the repro case above, JS doesn't run between what
you quoted in #2.

From what seems from the stack trace:
#0 0x7f2c5d6fe31e base::debug::StackTrace::StackTrace()
#1 0x7f2c5d76c1ff logging::LogMessage::~LogMessage()
#2 0x7f2c5447ba5a blink::EventHandlerRegistry::checkConsistency()
#3 0x7f2c5447c10c blink::EventHandlerRegistry::hasEventHandlers()
#4 0x7f2c5447c893 blink::EventHandlerRegistry::notifyHasHandlersChanged()
#5 0x7f2c5447c725 blink::EventHandlerRegistry::updateEventHandlerInternal()
#6 0x7f2c5447ce49 blink::EventHandlerRegistry::didRemoveAllEventHandlers()
#7 0x7f2c5447cded blink::EventHandlerRegistry::didMoveOutOfFrameHost()
#8 0x7f2c540ffc0c blink::Node::willMoveToNewDocument()
#9 0x7f2c541b87e5 blink::TreeScopeAdopter::moveNodeToNewDocument()
#10 0x7f2c541b7c17 blink::TreeScopeAdopter::moveTreeToNewScope()
#11 0x7f2c541b799b blink::TreeScopeAdopter::execute()
#12 0x7f2c541b1fac blink::TreeScope::adoptIfNeeded()
#13 0x7f2c53fc89ec blink::Document::adoptNode()
#14 0x7f2c55315bae blink::DocumentV8Internal::adoptNodeMethod()

didRemoveAllEventHandlers() loops through all EventHandlerClass enums,
and each descendant calls would accept a class from above, but
checkConsistency() also loops through all the EventHandlerClass,
so it sees an inconsistent state while trying to remove all the
classes one by one?

The other bug may not be the same issue, assuming my understanding above
is correct.

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

I uploaded my tentative fix at https://codereview.chromium.org/2653463003
which I used to check my theory above.  With the change this issue or
the other  issue 677596  does not happen.

Basically the change narrows down the consistency check to a specified
handler class only, so it may not be what the original check was intended to.

Cc: skyos...@chromium.org
+ skyostil@ who wrote the for-loop in the consistency check.

Can you comment on the intention of the check w.r.t. the CL in #5 ?
I see, we're doing the consistency check in the middle of an atomic handler removal operation, and it's blowing up because of the intermediate inconsistent state. I think it's fine to change to verification to only happen for the class of handler that is being queried.
kochi@ - would you like to proceed with your patch then?
Cc: -kochi@chromium.org wjmaclean@chromium.org
Owner: kochi@chromium.org

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

Thanks skyostil@ for reviewing.
As the check looks okay to you, I'll land the CL to fix this.
Project Member

Comment 11 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 12 by kochi@chromium.org, Jan 25 2017

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

Comment 13 by ClusterFuzz, Jan 25 2017

ClusterFuzz has detected this issue as fixed in range 445914:445945.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5457387516592128

Fuzzer: inferno_twister
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  node->document().frameHost() in EventHandlerRegistry.cpp
  blink::EventHandlerRegistry::checkConsistency
  blink::EventHandlerRegistry::hasEventHandlers
  
Sanitizer: address (ASAN)

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=306806:306813
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=445914:445945

Minimized Testcase (0.63 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94f4WFmK7wAfuvZGWxWWHNCFTtmkkLY6B1NMk30OIWBrNUs-UkHUalJwJVOymM_3VLenX-awkAupxr4DOYf8iFkLfg9-FOt5RjW72gtwPE-T3ZOKhXnZUrFD8f0f8J5b3frGm2R-A1E8WvWfzE1PFOvAbtD_wMEZCH1HCp8f5gI2RpbgbDWQTtARN6lImXyg27DCg54c8VmXfSgB4ikItlLPOCKsLEMGLWew-rU_VO-izr9VvawtlOuUGOMQdKRTaENgcFOOP1XnKW1s_xmyo0n8V761YBHLrxg6Sx2gleU3rxTwUuRKVWpBNN0PaMPd1b2r8a_VCsPyxerPhB-jf_s2kxcCkxFh6k9QwGOxl0iiamTz24?testcase_id=5457387516592128

Additional requirements: Requires HTTP

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