| Summary: | AX: .js dialogs shown in unload while AX is running crash WebKit. | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Samuel White <samuel_white> | ||||||||||||||||||||||||||||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, andersca, apinheiro, ap, beidson, buildbot, cfleizach, commit-queue, dmazzoni, eflews.bot, gyuyoung.kim, japhet, jcraig, jdiggs, mario, rniwa, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||||||||||||||
| OS: | OS X 10.9 | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
Created attachment 216112 [details]
Patch.
Added manual test as this issue is not reproducible in DRT.
Comment on attachment 216112 [details] Patch. Attachment 216112 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/21668005 New failing tests: platform/mac/fast/loader/webarchive-encoding-respected.html webarchive/loading/javascript-url-iframe-crash.html webarchive/loading/video-in-webarchive.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html webarchive/loading/test-loading-archive.html webarchive/loading/cache-expired-subresource.html Created attachment 216129 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216112 [details] Patch. Attachment 216112 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/21608015 New failing tests: platform/mac/fast/loader/webarchive-encoding-respected.html webarchive/loading/javascript-url-iframe-crash.html webarchive/loading/video-in-webarchive.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html webarchive/loading/test-loading-archive.html webarchive/loading/cache-expired-subresource.html Created attachment 216131 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216112 [details] Patch. Attachment 216112 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/21738002 New failing tests: platform/mac/fast/loader/webarchive-encoding-respected.html webarchive/loading/javascript-url-iframe-crash.html webarchive/loading/video-in-webarchive.html fast/encoding/char-encoding.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html webarchive/loading/test-loading-archive.html webarchive/loading/cache-expired-subresource.html fast/encoding/char-encoding-mac.html Created attachment 216136 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 216211 [details]
Updated patch.
Comment on attachment 216211 [details] Updated patch. Attachment 216211 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22758011 New failing tests: fast/encoding/char-encoding.html fast/encoding/char-encoding-mac.html Created attachment 216228 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #11) > (From update of attachment 216211 [details]) > Attachment 216211 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/22758011 > > New failing tests: > fast/encoding/char-encoding.html > fast/encoding/char-encoding-mac.html These both run as expected on Mavericks for me. Thoughts? Comment on attachment 216211 [details]
Updated patch.
Do we not have accessibility layout tests?
This seems like extraordinarily subtle behavior, and a manual test is not sufficient to prevent us from re-regressing.
Comment on attachment 216211 [details] Updated patch. The tests are likely genuinely broken by this patch, they aren't flaky. So, r- for breaking the tests. More generally speaking, adding more state to DocumentLoader would be super unfortunate, it already has too much state. Could you please explain in more detail what is going wrong when the crash occurs? Is there a stack trace for it? If this is a regression, what patch did it regress with? I'm not even sure if <rdar://problem/15160412> is the correct bug, its description is quite different. Trace: 1 0x10e7de450 WTFCrash 2 0x10fabfc25 WebCore::DocumentLoader::commitData(char const*, unsigned long) 3 0x10ccb3200 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) 4 0x10fac1940 WebCore::DocumentLoader::commitLoad(char const*, int) 5 0x10fac1f1b WebCore::DocumentLoader::dataReceived(WebCore::CachedResource*, char const*, int) 6 0x10f72b1b1 WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) 7 0x10f72b09e WebCore::CachedRawResource::addDataBuffer(WebCore::ResourceBuffer*) 8 0x110de485e WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) 9 0x110de4666 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, WebCore::DataPayloadType) 10 0x10ceb4bef WebKit::WebResourceLoader::didReceiveData(CoreIPC::DataReference const&, long long) 11 0x10ceb7fd5 void CoreIPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(CoreIPC::DataReference const&, long long), CoreIPC::DataReference, long long>(std::__1::tuple<CoreIPC::DataReference, long long>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(CoreIPC::DataReference const&, long long)) 12 0x10ceb7433 void CoreIPC::handleMessage<Messages::WebResourceLoader::DidReceiveData, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(CoreIPC::DataReference const&, long long)>(CoreIPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(CoreIPC::DataReference const&, long long)) 13 0x10ceb6bd3 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) 14 0x10ca878fc WebKit::NetworkProcessConnection::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) 15 0x10c975a03 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&) 16 0x10c96e0b0 CoreIPC::Connection::dispatchMessage(std::__1::unique_ptr<CoreIPC::MessageDecoder, std::__1::default_delete<CoreIPC::MessageDecoder> >) 17 0x10c975791 CoreIPC::Connection::dispatchOneMessage() 18 0x10c982112 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*) 19 0x10c982095 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()() 20 0x10c989bb2 WTF::Function<void ()>::operator()() const 21 0x10c989b2c std::__1::__function::__func<WTF::Function<void ()>, std::__1::allocator<WTF::Function<void ()> >, void ()>::operator()() 22 0x110c272fa std::__1::function<void ()>::operator()() const 23 0x110c26e94 WebCore::RunLoop::performWork() 24 0x110c284c4 WebCore::RunLoop::performWork(void*) 25 0x7fff935ab8f1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 26 0x7fff9359d062 __CFRunLoopDoSources0 27 0x7fff9359c7ef __CFRunLoopRun 28 0x7fff9359c275 CFRunLoopRunSpecific 29 0x7fff8dfdff0d RunCurrentEventLoopInMode 30 0x7fff8dfdfcb7 ReceiveNextEventCommon 31 0x7fff8dfdfabc _BlockUntilNextEventMatchingListInModeWithFilter The issue that I'm seeing is that the WebResourceLoader gets DidFinishResourceLoad BETWEEN DocumentLoader::commitLoad and DocumentLoader::committedLoad when accessibility is running. When this happens all kinds of ASSERTS are hit. If accessibility is off, DidFinishResourceLoad comes in after as expected. Therefore, the proposed solution bubbles the calls triggered by DidFinishResourceLoad to after the commit completion. (In reply to comment #14) > (From update of attachment 216211 [details]) > Do we not have accessibility layout tests? > > This seems like extraordinarily subtle behavior, and a manual test is not sufficient to prevent us from re-regressing. Unfortunately, when the attached test case is turned into a LayoutTest is passes without issue. I suspected that real javascript dialogs were required to trigger the issue. (In reply to comment #15) > (From update of attachment 216211 [details]) > The tests are likely genuinely broken by this patch, they aren't flaky. So, r- for breaking the tests. > > More generally speaking, adding more state to DocumentLoader would be super unfortunate, it already has too much state. Agreed. I tried using isCommitted but that check was too broad and resulted in a race state being introduced. > > Could you please explain in more detail what is going wrong when the crash occurs? Is there a stack trace for it? I've elaborated in comment #17. > > If this is a regression, what patch did it regress with? I'm not even sure if <rdar://problem/15160412> is the correct bug, its description is quite different. I'm not sure actually. This failure is not detectable in DRT (see comment #18) so I'm at a loss when looking for a date of introduction. Thanks. (In reply to comment #19) > (In reply to comment #15) > > (From update of attachment 216211 [details] [details]) > > The tests are likely genuinely broken by this patch, they aren't flaky. So, r- for breaking the tests. > > > > More generally speaking, adding more state to DocumentLoader would be super unfortunate, it already has too much state. > > Agreed. I tried using isCommitted but that check was too broad and resulted in a race state being introduced. > > > > > Could you please explain in more detail what is going wrong when the crash occurs? Is there a stack trace for it? > > I've elaborated in comment #17. > > > > > If this is a regression, what patch did it regress with? I'm not even sure if <rdar://problem/15160412> is the correct bug, its description is quite different. > > I'm not sure actually. This failure is not detectable in DRT (see comment #18) so I'm at a loss when looking for a date of introduction. My guess is that this has been around for awhile since we allowed javascript dialogs to spin the run-loop while waiting for an answer, so that VoiceOver wouldn't hang. That was done probably a year ago or more. > > Thanks. (In reply to comment #17) > The issue that I'm seeing is that the WebResourceLoader gets DidFinishResourceLoad BETWEEN DocumentLoader::commitLoad and DocumentLoader::committedLoad when accessibility is running. When this happens all kinds of ASSERTS are hit. > > If accessibility is off, DidFinishResourceLoad comes in after as expected. Therefore, the proposed solution bubbles the calls triggered by DidFinishResourceLoad to after the commit completion. Can you describe the user situation that results in this as well (In reply to comment #21) > (In reply to comment #17) > > The issue that I'm seeing is that the WebResourceLoader gets DidFinishResourceLoad BETWEEN DocumentLoader::commitLoad and DocumentLoader::committedLoad when accessibility is running. When this happens all kinds of ASSERTS are hit. > > > > If accessibility is off, DidFinishResourceLoad comes in after as expected. Therefore, the proposed solution bubbles the calls triggered by DidFinishResourceLoad to after the commit completion. > > Can you describe the user situation that results in this as well Good idea. Firstly, the quick and dirty reproduction steps are: 1. Open the test case attachment above 2. Turn VoiceOver on 3. Reload the page and attempt to interact with the dialog We see this happening in the wild on sites that prompt a user during the unload (or load) event with a javascript dialog (and VoiceOver running). For example, Facebook prompts you during unload if you try to close your window while composing a comment. Again, if you're a VoiceOver user this results in an unusable state as WebCore becomes unable to respond to any accessibility API calls. Created attachment 218541 [details]
patch
(In reply to comment #23) > Created an attachment (id=218541) [details] > patch Updated patch that 1) does not add state 2) adds a layout test that reproduces 3) presumably does not cause any regressions -- buildbots will tell us Attachment 218541 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash-expected.txt', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src2.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/FrameLoader.h', u'Source/WebCore/loader/SubresourceLoader.cpp', u'Source/WebCore/loader/SubresourceLoader.h', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.h', u'Tools/WebKitTestRunner/TestController.cpp', u'Tools/WebKitTestRunner/TestController.h', u'Tools/WebKitTestRunner/TestInvocation.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/loader/SubresourceLoader.cpp:69: Comma should be at the beggining of the line in a member initialization list. [whitespace/init] [4]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #25) > Attachment 218541 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash-expected.txt', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src2.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/FrameLoader.h', u'Source/WebCore/loader/SubresourceLoader.cpp', u'Source/WebCore/loader/SubresourceLoader.h', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.h', u'Tools/WebKitTestRunner/TestController.cpp', u'Tools/WebKitTestRunner/TestController.h', u'Tools/WebKitTestRunner/TestInvocation.cpp', '--commit-queue']" exit_code: 1 > ERROR: Source/WebCore/loader/SubresourceLoader.cpp:69: Comma should be at the beggining of the line in a member initialization list. [whitespace/init] [4] > Total errors found: 1 in 18 files > I am inclined to say this is a false positive > > If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 218541 [details] patch Attachment 218541 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/44208046 Created attachment 218549 [details]
patch
Attachment 218549 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash-expected.txt', u'LayoutTests/platform/mac-wk2/accessibility/javascript-alert-unload-handler-crash.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src.html', u'LayoutTests/platform/mac-wk2/accessibility/resources/javascript-alert-unload-handler-crash-subframe-src2.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/FrameLoader.h', u'Source/WebCore/loader/SubresourceLoader.cpp', u'Source/WebCore/loader/SubresourceLoader.h', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.h', u'Tools/WebKitTestRunner/TestController.cpp', u'Tools/WebKitTestRunner/TestController.h', u'Tools/WebKitTestRunner/TestInvocation.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/loader/SubresourceLoader.cpp:69: Comma should be at the beggining of the line in a member initialization list. [whitespace/init] [4]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 218549 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=218549&action=review I’m not sure this concept is going to work. The code in SubresourceLoader::didFinishLoading seems to only scratch the surface in terms of the number of bad things that could happen during the “stall for JavaScript alerts” state; we’d need something more complete and robust, maybe at a lower level? And adding a separate timer to each subresource also seems wrong. This is an overall state that I think is browser-context-wide, so this deferral mechanism should probably be tied to the entire browsing context, not a single sub resource. It needs to be a lot more like the existing setDefersLoading mechanism, which has a similar (maybe identical?) goal. > Source/WebCore/loader/SubresourceLoader.cpp:83 > + m_loadingCompleteRetryTimer.stop(); This line of code is not needed or helpful. A timer that is destroyed stops itself, and since this is the destructor the timer that is a data member will be destroyed. > Source/WebCore/loader/SubresourceLoader.cpp:277 > + if (m_documentLoader->frameLoader()->pageDismissalEventBeingDispatched() == FrameLoader::UnloadDismissal && !m_documentLoader->frameLoader()->unloadEventEmitted()) { This is a fragile rule, but it makes it even worse to have to check these things directly. I think we need a function on frame loader that encapsulates this into a single boolean. But what about when this is one frame, and the unload event is being processed in another frame elsewhere? It seems to me that we shouldn’t do this processing on any frame if any frame is in this state, since JavaScript could reach over to other frames and interact with them. I think this check is too narrow, and potentially needed in *many* other places. > Source/WebCore/loader/SubresourceLoader.cpp:279 > + m_loadingCompleteRetryTimer.startOneShot(0.2); Where does this 0.2 second duration come from? I’d prefer a named constant at the top of the file with a brief comment explaining the rational. See also: bug 126021. Created attachment 230408 [details]
patch
We still need the WKSI part to land
Created attachment 230422 [details]
patch
Created attachment 230426 [details]
patch
Comment on attachment 230426 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=230426&action=review > Source/WebKit2/Platform/IPC/mac/ConnectionMac.cpp:50 > +extern "C" AXError _AXUIElementNotifyProcessSuspendStatus(AXSuspendStatus); I think that you also need to redefine AXSuspendStatus enum for open source build to not break. > Source/WebKit2/Platform/IPC/mac/ConnectionMac.cpp:549 > + if ((flags & InformPlatformProcessWillSuspend) && WebCore::AXObjectCache::accessibilityEnabled()) I don't know if it's OK to use WebCore in CoreIPC. Anders? > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:384 > - // FIXME (126021): It is not good to change IPC behavior conditionally, and SpinRunLoopWhileWaitingForReply was known to cause trouble in other similar cases. > - unsigned syncSendFlags = WebPage::synchronousMessagesShouldSpinRunLoop() ? IPC::SpinRunLoopWhileWaitingForReply : 0; > + unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend; > + if (WebPage::synchronousMessagesShouldSpinRunLoop()) > + syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply; I don't understand how this change addresses the FIXME. Should IPC::SpinRunLoopWhileWaitingForReply be under an OS X version check now? Comment on attachment 230426 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=230426&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:384 >> + syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply; > > I don't understand how this change addresses the FIXME. Should IPC::SpinRunLoopWhileWaitingForReply be under an OS X version check now? yes we can do that Created attachment 230435 [details]
patch
Using the run loop spinning is now guarded by this method
bool WebPage::synchronousMessagesShouldSpinRunLoop()
{
#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101000
return WebCore::AXObjectCache::accessibilityEnabled();
#endif
return false;
}
Attachment 230435 [details] did not pass style-queue:
ERROR: Source/WebKit2/Platform/IPC/Connection.cpp:555: An else should appear on the same line as the preceding } [whitespace/newline] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 230436 [details]
patch
Created attachment 230438 [details]
patch
|
Created attachment 216075 [details] Reduced case. Attached is a reduced test case provided by James Craig. To reproduce load case with VO on and refresh page.