node->document().frameHost() in EventHandlerRegistry.cpp |
||||||
Issue descriptionDetailed 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.
,
Jan 20 2017
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.
,
Jan 23 2017
,
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.
,
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.
,
Jan 23 2017
+ 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 ?
,
Jan 24 2017
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.
,
Jan 24 2017
kochi@ - would you like to proceed with your patch then?
,
Jan 24 2017
,
Jan 25 2017
Thanks skyostil@ for reviewing. As the check looks okay to you, I'll land the CL to fix this.
,
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
,
Jan 25 2017
should be fixed now.
,
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 |
||||||
Comment 1 by msrchandra@chromium.org
, Jan 20 2017Labels: Test-Predator-Wrong
Owner: wjmaclean@chromium.org
Status: Assigned (was: Untriaged)