AX: .js dialogs shown in unload while AX is running crash WebKit.

[Samuel White]

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.

[Samuel White]

<radar://problem/15160412>

[Samuel White]

Created attachment 216112 [details]
Patch.

Added manual test as this issue is not reproducible in DRT.

[Babak Shafiei]

<rdar://problem/15160412>

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Samuel White]

Created attachment 216211 [details]
Updated patch.

[Build Bot]

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

[Build Bot]

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

[Samuel White]

(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?

[Brady Eidson]

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.

[Alexey Proskuryakov]

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.

[Samuel White]

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

[Samuel White]

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.

[Samuel White]

(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.

[Samuel White]

(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.

[chris fleizach]

(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.

[chris fleizach]

(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

[Samuel White]

(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.

[chris fleizach]

Created attachment 218541 [details]
patch

[chris fleizach]

(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

[WebKit Commit Bot]

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.

[chris fleizach]

(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.

[EFL EWS Bot]

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

[chris fleizach]

Created attachment 218549 [details]
patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

See also: bug 126021.

[chris fleizach]

Created attachment 230408 [details]
patch

We still need the WKSI part to land

[chris fleizach]

Created attachment 230422 [details]
patch

[chris fleizach]

Created attachment 230426 [details]
patch

[Alexey Proskuryakov]

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?

[chris fleizach]

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

[chris fleizach]

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;
}

[WebKit Commit Bot]

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.

[chris fleizach]

Created attachment 230436 [details]
patch

[chris fleizach]

Created attachment 230438 [details]
patch

[chris fleizach]

http://trac.webkit.org/changeset/168653