Summary: | [ Mac wk1] ASSERTION FAILED: m_wrapper under WebCore::XMLHttpRequestUpload::dispatchProgressEvent | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Lawrence <Lawrence.j> | ||||||
Component: | Tools / Tests | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, cdumez, ggaren, rniwa, saam, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Mac | ||||||||
OS: | macOS 10.15 | ||||||||
Attachments: |
|
Description
Jason Lawrence
2020-03-25 13:43:29 PDT
I have marked this test as failing and crashing while this issue is investigated. https://trac.webkit.org/changeset/259007/webkit I was able to reproduce the failure with the command below. run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f -1 imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm' (In reply to Jason Lawrence from comment #3) > I was able to reproduce the failure with the command below. > run-webkit-tests --iterations 2000 --exit-after-n-failures 1 > --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force > --no-build -f -1 > imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm' This seems to reproduce a flaky timeout for me but not a crash. (In reply to Chris Dumez from comment #4) > (In reply to Jason Lawrence from comment #3) > > I was able to reproduce the failure with the command below. > > run-webkit-tests --iterations 2000 --exit-after-n-failures 1 > > --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force > > --no-build -f -1 > > imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm' > > This seems to reproduce a flaky timeout for me but not a crash. Crash might have been fixed by http://trac.webkit.org/r259009 or http://trac.webkit.org/r258959. (In reply to Chris Dumez from comment #5) > (In reply to Chris Dumez from comment #4) > > (In reply to Jason Lawrence from comment #3) > > > I was able to reproduce the failure with the command below. > > > run-webkit-tests --iterations 2000 --exit-after-n-failures 1 > > > --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force > > > --no-build -f -1 > > > imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm' > > > > This seems to reproduce a flaky timeout for me but not a crash. > > Crash might have been fixed by http://trac.webkit.org/r259009 or > http://trac.webkit.org/r258959. Not fixed, I can now reproduce the crash with aggressive GC on: run-webkit-tests --iterations 200 --no-retry --force --no-build -f -1 imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm --additional-env-var=JSC_slowPathAllocsBetweenGCs=10 (In reply to Chris Dumez from comment #6) > (In reply to Chris Dumez from comment #5) > > (In reply to Chris Dumez from comment #4) > > > (In reply to Jason Lawrence from comment #3) > > > > I was able to reproduce the failure with the command below. > > > > run-webkit-tests --iterations 2000 --exit-after-n-failures 1 > > > > --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force > > > > --no-build -f -1 > > > > imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm' > > > > > > This seems to reproduce a flaky timeout for me but not a crash. > > > > Crash might have been fixed by http://trac.webkit.org/r259009 or > > http://trac.webkit.org/r258959. > > Not fixed, I can now reproduce the crash with aggressive GC on: > run-webkit-tests --iterations 200 --no-retry --force --no-build -f -1 > imported/w3c/web-platform-tests/xhr/send-response-upload-event-progress.htm > --additional-env-var=JSC_slowPathAllocsBetweenGCs=10 It reproduces on WebKit2 as well. This seems to indicate that the JS wrapper for the XMLHttpRequestUpload object has been collected by the time we try and dispatch the JS event. Normally, the XMLHttpRequestUpload wrapper is kept alive by the XMLHttpRequest wrapper via this visit method: void JSXMLHttpRequest::visitAdditionalChildren(SlotVisitor& visitor) { if (auto* upload = wrapped().optionalUpload()) visitor.addOpaqueRoot(upload); if (auto* responseDocument = wrapped().optionalResponseXML()) visitor.addOpaqueRoot(responseDocument); } and this generated isReachable code for JSXMLHttpRequestUpload: bool JSXMLHttpRequestUploadOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason) { auto* jsXMLHttpRequestUpload = jsCast<JSXMLHttpRequestUpload*>(handle.slot()->asCell()); if (jsXMLHttpRequestUpload->wrapped().isFiringEventListeners()) { if (UNLIKELY(reason)) *reason = "EventTarget firing event listeners"; return true; } XMLHttpRequestUpload* root = &jsXMLHttpRequestUpload->wrapped(); if (UNLIKELY(reason)) *reason = "Reachable from XMLHttpRequestUpload"; return visitor.containsOpaqueRoot(root); } The generated bindings code for the upload.onprogress setter looks reasonable: static inline bool setJSXMLHttpRequestEventTargetOnprogressSetter(JSGlobalObject& lexicalGlobalObject, JSXMLHttpRequestEventTarget& thisObject, JSValue value, ThrowScope& throwScope) { UNUSED_PARAM(lexicalGlobalObject); UNUSED_PARAM(throwScope); setEventHandlerAttribute(lexicalGlobalObject, thisObject, thisObject.wrapped(), eventNames().progressEvent, value); VM& vm = JSC::getVM(&lexicalGlobalObject); vm.heap.writeBarrier(&thisObject, value); ensureStillAliveHere(value); return true; } Created attachment 394636 [details]
Patch
Comment on attachment 394636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394636&action=review r=me > Source/WebCore/ChangeLog:10 > + have any relevant event listeners. However, the XMLHttpRequestUpload's wrapper lifetime is tried tied > Source/WebCore/ChangeLog:13 > + relevant listener, even though XMLHttpRequestUpload may have a relevant event listeners. We would listener > Source/WebCore/xml/XMLHttpRequest.cpp:1188 > switch (readyState()) { Does the readyState change to DONE before or after we fire event listeners? (It might be a problem if the answer is 'before'.) Comment on attachment 394636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394636&action=review > LayoutTests/http/wpt/xhr/send-response-upload-event-progress-agressive-gc.htm:1 > +<!DOCTYPE html><!-- webkit-test-runner [ jscOptions=--slowPathAllocsBetweenGCs=10 ] --> Are wpt tests supposed to be upstreamed? This one obviously can't. I remain very skeptical about adding copies of tests with slowPathAllocsBetweenGCs. That's a high cost for catching issues that we would catch anyway post-commit. (In reply to Alexey Proskuryakov from comment #12) > Comment on attachment 394636 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394636&action=review > > > LayoutTests/http/wpt/xhr/send-response-upload-event-progress-agressive-gc.htm:1 > > +<!DOCTYPE html><!-- webkit-test-runner [ jscOptions=--slowPathAllocsBetweenGCs=10 ] --> > > Are wpt tests supposed to be upstreamed? This one obviously can't. > > I remain very skeptical about adding copies of tests with > slowPathAllocsBetweenGCs. That's a high cost for catching issues that we > would catch anyway post-commit. Ok, I can drop the slowPathAllocsBetweenGCs test. I think Saam was the one who suggested I add such test in a previous review. Created attachment 394652 [details]
Patch
Committed r259080: <https://trac.webkit.org/changeset/259080> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394652 [details]. |