Bug 173520

Summary: AX: Cannot call setValue() on contenteditable or ARIA text controls
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, buildbot, commit-queue, dmazzoni, jbedard, jcraig, jdiggs, jlewis3, rniwa, ryanhaddad, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
patch
none
patch
none
patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
patch
none
patch for landing none

Description chris fleizach 2017-06-17 21:06:08 PDT
Trying to call setAccessibilityValue on a content editable or aria text control fails. That support was never built in

<rdar://problem/31717256>
Comment 1 chris fleizach 2017-06-17 21:09:19 PDT
Created attachment 313218 [details]
patch
Comment 2 Build Bot 2017-06-17 22:08:21 PDT
Comment on attachment 313218 [details]
patch

Attachment 313218 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3950758

New failing tests:
inspector/canvas/create-canvas-contexts.html
Comment 3 Build Bot 2017-06-17 22:08:22 PDT
Created attachment 313221 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-06-17 22:16:51 PDT
Comment on attachment 313218 [details]
patch

Attachment 313218 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3950768

New failing tests:
accessibility/set-value-editable-types.html
Comment 5 Build Bot 2017-06-17 22:16:52 PDT
Created attachment 313222 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Joanmarie Diggs 2017-06-18 04:36:14 PDT
Comment on attachment 313218 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313218&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1754
> +    else if (isARIATextControl())

A noted limitation of ARIA 1.x is that it is "one way." ATs can get values, but not set them. Should we really allow setting the value for non-content-editable ARIA text controls? And if your answer is "yes," then what about setting the value of ARIA range controls (which I believe James indicated couldn't, or shouldn't, be done for <insert reason I forgot here>)?

Note that I'm not yet saying "Don't do this"; it's more of a "Really? Are you sure?"
Comment 7 Joanmarie Diggs 2017-06-18 04:41:44 PDT
(In reply to Joanmarie Diggs (irc: joanie) from comment #6)

> what about setting the value of ARIA range controls (which I believe James
> indicated couldn't, or shouldn't, be done for <insert reason I forgot here>)?
> 
> Note that I'm not yet saying "Don't do this"; it's more of a "Really? Are
> you sure?"

Not sure if the reason James cited was the author updating their stored value, which would subsequently get sent as part of the form submission, but.... If it's not a content-editable control, and it's not a native host language control, and we set the value, what mechanism are we going to use to notify the author of the new value? And what guarantee do we have that the author will in turn accept that new value?

(I'm afraid I'm starting to drift away from "Are you sure?" and towards "Don't do this.")
Comment 8 chris fleizach 2017-06-18 11:38:48 PDT
Comment on attachment 313218 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313218&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1755
> +        element.setInnerHTML(string);

I'm ok removing that part of this patch. I saw the FIXME in the code and realized it would be easy to change. I'm mostly concerned about the content editable part in this patch

We can wait for AOM or future ARIA versions to worry about the content setting parts
Comment 9 chris fleizach 2017-06-18 11:40:12 PDT
Created attachment 313232 [details]
patch

Updated patch to remove aria text control changes
Comment 10 Build Bot 2017-06-18 12:41:08 PDT
Comment on attachment 313232 [details]
patch

Attachment 313232 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3953542

New failing tests:
accessibility/set-value-editable-types.html
Comment 11 Build Bot 2017-06-18 12:41:09 PDT
Created attachment 313235 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-06-18 12:47:05 PDT
Comment on attachment 313232 [details]
patch

Attachment 313232 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3953550

New failing tests:
accessibility/set-value-editable-types.html
Comment 13 Build Bot 2017-06-18 12:47:07 PDT
Created attachment 313238 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-06-18 12:58:36 PDT
Comment on attachment 313232 [details]
patch

Attachment 313232 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3953564

New failing tests:
accessibility/set-value-editable-types.html
Comment 15 Build Bot 2017-06-18 12:58:37 PDT
Created attachment 313239 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 chris fleizach 2017-06-18 23:55:29 PDT
Created attachment 313267 [details]
patch
Comment 17 Build Bot 2017-06-19 00:37:36 PDT
Comment on attachment 313267 [details]
patch

Attachment 313267 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3956028

New failing tests:
accessibility/set-value-editable-types.html
Comment 18 Build Bot 2017-06-19 00:37:37 PDT
Created attachment 313269 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 chris fleizach 2017-06-19 01:48:31 PDT
Created attachment 313270 [details]
patch
Comment 20 Build Bot 2017-06-19 01:51:34 PDT
Attachment 313270 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:113:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 chris fleizach 2017-06-19 01:53:23 PDT
Created attachment 313271 [details]
patch
Comment 22 chris fleizach 2017-06-19 23:59:01 PDT
Joanmarie can you review again? thx
Comment 23 Ryosuke Niwa 2017-06-20 00:18:28 PDT
Comment on attachment 313271 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313271&action=review

> LayoutTests/accessibility/set-value-editable-types.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use modern doctype: <!DOCTYPE html>

> LayoutTests/accessibility/set-value-editable-types.html:5
> +<head>
> +<script src="../resources/js-test-pre.js"></script>
> +</head>

No need for head. Just put it in body.

> LayoutTests/accessibility/set-value-editable-types.html:20
> +    if (window.accessibilityController) {

You should add testFailed when window.accessibilityController is not defined with a description.

> LayoutTests/accessibility/set-value-editable-types.html:25
> +            debug("Role: " + axElement.role);
> +            debug("Value: " + axElement.stringValue);

Please use shouldBe instead.

> LayoutTests/accessibility/set-value-editable-types.html:28
> +            var writable = axElement.isAttributeSettable("AXValue");
> +            debug("Writable: " + writable);

Ditto.

> LayoutTests/accessibility/set-value-editable-types.html:36
> +            // setting attributes can be done after a dispatch.
> +            window.setTimeout(function() {
> +               debug("Updated value: " + axElement.stringValue);
> +               completion();
> +            }, 10);

There is no callback for this? 10ms may not be enough when the load is heavy on the machine.
Comment 24 chris fleizach 2017-06-20 01:05:59 PDT
(In reply to Ryosuke Niwa from comment #23)
> Comment on attachment 313271 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313271&action=review
> 
> > LayoutTests/accessibility/set-value-editable-types.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Please use modern doctype: <!DOCTYPE html>
> 
> > LayoutTests/accessibility/set-value-editable-types.html:5
> > +<head>
> > +<script src="../resources/js-test-pre.js"></script>
> > +</head>
> 
> No need for head. Just put it in body.
> 
> > LayoutTests/accessibility/set-value-editable-types.html:20
> > +    if (window.accessibilityController) {
> 
> You should add testFailed when window.accessibilityController is not defined
> with a description.
> 
> > LayoutTests/accessibility/set-value-editable-types.html:25
> > +            debug("Role: " + axElement.role);
> > +            debug("Value: " + axElement.stringValue);
> 
> Please use shouldBe instead.

That doesn't scale as well to all the platforms. When we use debug, its easier to add expectations files for each platform rather than writing new tests for each platform

> 
> > LayoutTests/accessibility/set-value-editable-types.html:28
> > +            var writable = axElement.isAttributeSettable("AXValue");
> > +            debug("Writable: " + writable);
> 
> Ditto.
> 
> > LayoutTests/accessibility/set-value-editable-types.html:36
> > +            // setting attributes can be done after a dispatch.
> > +            window.setTimeout(function() {
> > +               debug("Updated value: " + axElement.stringValue);
> > +               completion();
> > +            }, 10);
> 
> There is no callback for this? 10ms may not be enough when the load is heavy
> on the machine.

There's no callback for calling the accessibilitySetValue. It just happens on the next iteration of the run loop, so I think if we can delay our JS method to another run loop iteration, this will happen in order
Comment 25 chris fleizach 2017-06-20 11:44:38 PDT
Created attachment 313417 [details]
patch

Addressed relevant comments
Comment 26 Build Bot 2017-06-20 13:21:33 PDT
Comment on attachment 313417 [details]
patch

Attachment 313417 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3967368

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbobjectstore_clear2.html
Comment 27 Build Bot 2017-06-20 13:21:35 PDT
Created attachment 313431 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 28 Ryosuke Niwa 2017-06-21 01:35:21 PDT
Comment on attachment 313417 [details]
patch

Nice!
Comment 29 WebKit Commit Bot 2017-06-21 08:58:56 PDT
Comment on attachment 313417 [details]
patch

Clearing flags on attachment: 313417

Committed r218633: <http://trac.webkit.org/changeset/218633>
Comment 30 WebKit Commit Bot 2017-06-21 08:58:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Matt Lewis 2017-06-21 16:15:45 PDT
After this revision was landed, the test accessibility/set-value-editable-types.html started to consistently fail on Windows Release and Sierra WK2 Debug.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fset-value-editable-types.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r218643%20(1676)/results.html

Sierra Diff:
--- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/accessibility/set-value-editable-types-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/accessibility/set-value-editable-types-actual.txt
@@ -8,7 +8,7 @@
 Role: AXRole: AXTextArea
 Value: AXValue: current1
 Writable: true
-Updated value: AXValue: new value
+Updated value: AXValue: current1
 PASS successfullyParsed is true
 
 TEST COMPLETE

Windows Diff:
--- /home/buildbot/slave/win-release-tests/build/layout-test-results/accessibility/set-value-editable-types-expected.txt
+++ /home/buildbot/slave/win-release-tests/build/layout-test-results/accessibility/set-value-editable-types-actual.txt
@@ -5,10 +5,10 @@
 
 
 Contenteditable
-Role: AXRole: AXTextArea
+Role: AXRole: AXStaticText
 Value: AXValue: current1
-Writable: true
-Updated value: AXValue: new value
+Writable: false
+Updated value: AXValue: current1
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 32 Matt Lewis 2017-06-22 09:34:01 PDT
Reverted r218633 for reason:

The test is failing frequently on Sierra Debug and Windows

Committed r218702: <http://trac.webkit.org/changeset/218702>
Comment 33 chris fleizach 2017-06-22 09:39:59 PDT
(In reply to Matt Lewis from comment #32)
> Reverted r218633 for reason:
> 
> The test is failing frequently on Sierra Debug and Windows
> 
> Committed r218702: <http://trac.webkit.org/changeset/218702>

Completely unhelpful. Test on windows should just be skipped
Comment 34 Ryan Haddad 2017-06-22 10:35:14 PDT
(In reply to chris fleizach from comment #33)
> (In reply to Matt Lewis from comment #32)
> > Reverted r218633 for reason:
> > 
> > The test is failing frequently on Sierra Debug and Windows
> > 
> > Committed r218702: <http://trac.webkit.org/changeset/218702>
> 
> Completely unhelpful. Test on windows should just be skipped

We can skip the test on Windows, but it was still a flaky failure on Sierra Debug WK2.
Comment 35 chris fleizach 2017-06-22 10:36:07 PDT
(In reply to Ryan Haddad from comment #34)
> (In reply to chris fleizach from comment #33)
> > (In reply to Matt Lewis from comment #32)
> > > Reverted r218633 for reason:
> > > 
> > > The test is failing frequently on Sierra Debug and Windows
> > > 
> > > Committed r218702: <http://trac.webkit.org/changeset/218702>
> > 
> > Completely unhelpful. Test on windows should just be skipped
> 
> We can skip the test on Windows, but it was still a flaky failure on Sierra
> Debug WK2.

I'm working on a fix for both and will post a new patch for review

thanks
Comment 36 chris fleizach 2017-06-22 13:04:45 PDT
Created attachment 313652 [details]
patch

New patch
  - move the test to Mac only
  - rely on the value change callback before we check the updated value
Comment 37 chris fleizach 2017-06-23 15:21:33 PDT
Anyone available to review this again?
Comment 38 chris fleizach 2017-06-23 17:56:44 PDT
Comment on attachment 313652 [details]
patch

Thanks Ryosuke!
Comment 39 WebKit Commit Bot 2017-06-23 18:25:59 PDT
Comment on attachment 313652 [details]
patch

Rejecting attachment 313652 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 313652, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
22f M	Source
:040000 040000 5bf598c00c17d61e47dc225a4161bef0a48128e7 51df1e4d97692ef22e6c26445eba3e4fc4ba1f8f M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3987435
Comment 40 WebKit Commit Bot 2017-06-24 00:14:49 PDT
Comment on attachment 313652 [details]
patch

Clearing flags on attachment: 313652

Committed r218783: <http://trac.webkit.org/changeset/218783>
Comment 41 WebKit Commit Bot 2017-06-24 00:14:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Matt Lewis 2017-06-26 10:00:48 PDT
It looks like this patch caused test: accessibility/mac/setting-attributes-is-asynchronous.html
to crash every time on macOS WK2 Debug. It's starting to affect testing.

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218804%20(1939)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Fsetting-attributes-is-asynchronous.html

stderr:
1   0x11a79d930 WTFCrash
2   0x121d02de7 WTR::AccessibilityController::removeNotificationListener()
3   0x121d3c285 WTR::JSAccessibilityController::removeNotificationListener(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**)
4   0x11a0fcd69 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*)
5   0x11a2d75b3 JSC::LLInt::handleHostCall(JSC::ExecState*, JSC::Instruction*, JSC::JSValue, JSC::CodeSpecializationKind)
6   0x11a2d87c0 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*)
7   0x11a2d8396 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind)
8   0x11a2d47a9 llint_slow_path_call
9   0x11a2e36b0 llint_entry
10  0x11a2dbd6e vmEntryToJavaScript
11  0x11a09c341 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
12  0x11a0497d5 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
13  0x1197d2ebe JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
14  0x1197d30fa JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
15  0x11a1ce9ed JSObjectCallAsFunction
16  0x121d03df0 -[AccessibilityNotificationHandler _notificationReceived:]
17  0x7fff88554b0c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
18  0x7fff88554a9f ___CFXRegistrationPost_block_invoke
19  0x7fff88554a17 _CFXRegistrationPost
20  0x7fff88554782 ___CFXNotificationPost_block_invoke
21  0x7fff88511592 -[_CFXNotificationRegistrar find:object:observer:enumerator:]
22  0x7fff885107e5 _CFXNotificationPost
23  0x7fff91b2323a -[NSNotificationCenter postNotificationName:object:userInfo:]
24  0x10fdeb8ab -[WebAccessibilityObjectWrapperBase accessibilityPostedNotification:userInfo:]
25  0x10d36c525 WebCore::AXPostNotificationWithUserInfo(WebAccessibilityObjectWrapper*, NSString*, objc_object*)
26  0x10d36c415 WebCore::AXObjectCache::postPlatformNotification(WebCore::AccessibilityObject*, WebCore::AXObjectCache::AXNotification)
27  0x10d33ba4d WebCore::AXObjectCache::notificationPostTimerFired()
28  0x10d354bc7 WTF::Function<void ()>::CallableWrapper<std::__1::__bind<void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*> >::call()
29  0x10d1de093 WTF::Function<void ()>::operator()() const
30  0x10d1ddefc WebCore::Timer::fired()
31  0x10fcb21c2 WebCore::ThreadTimers::sharedTimerFiredInternal()
LEAK: 1 WebPageProxy
Comment 43 Jonathan Bedard 2017-06-26 11:51:24 PDT
Reverted r218783 for reason:

Causing accessibility/mac/setting-attributes-is-asynchronous.html to crash consistently on mac-wk2 Debug

Committed r218817: <http://trac.webkit.org/changeset/218817>
Comment 44 Jonathan Bedard 2017-06-26 11:53:09 PDT
I'm going to look into this failure a bit today.  Looking over the content of this patch briefly, I suspect something wrong with infrastructure, but in the mean time, rolling out this change since it's pretty consistently causing WebKit 2 to crash on Mac.
Comment 45 chris fleizach 2017-06-26 12:47:57 PDT
(In reply to Jonathan Bedard from comment #44)
> I'm going to look into this failure a bit today.  Looking over the content
> of this patch briefly, I suspect something wrong with infrastructure, but in
> the mean time, rolling out this change since it's pretty consistently
> causing WebKit 2 to crash on Mac.

can we not do that since this is just a test issue.
Comment 46 Ryosuke Niwa 2017-06-26 13:09:53 PDT
(In reply to chris fleizach from comment #45)
> (In reply to Jonathan Bedard from comment #44)
> > I'm going to look into this failure a bit today.  Looking over the content
> > of this patch briefly, I suspect something wrong with infrastructure, but in
> > the mean time, rolling out this change since it's pretty consistently
> > causing WebKit 2 to crash on Mac.
> 
> can we not do that since this is just a test issue.

Our policy is to rollout any patch that recesses our tests.
Comment 47 Jonathan Bedard 2017-06-26 13:15:36 PDT
(In reply to chris fleizach from comment #45)
> (In reply to Jonathan Bedard from comment #44)
> > I'm going to look into this failure a bit today.  Looking over the content
> > of this patch briefly, I suspect something wrong with infrastructure, but in
> > the mean time, rolling out this change since it's pretty consistently
> > causing WebKit 2 to crash on Mac.
> 
> can we not do that since this is just a test issue.

Just managed to get to the bottom of this.

You're calling finishJSTest() too early, it should be called after accessibilityController.removeNotificationListener(). Calling removeNotificationListener() will actually cause the next test to crash. If finishJSTest() is called after accessibilityController.removeNotificationListener(), I no longer see the test failure locally.

Ryosuke, do you have any other additional comments, or do you think this patch is ready to go with that change?
Comment 48 chris fleizach 2017-06-26 13:16:45 PDT
(In reply to Jonathan Bedard from comment #47)
> (In reply to chris fleizach from comment #45)
> > (In reply to Jonathan Bedard from comment #44)
> > > I'm going to look into this failure a bit today.  Looking over the content
> > > of this patch briefly, I suspect something wrong with infrastructure, but in
> > > the mean time, rolling out this change since it's pretty consistently
> > > causing WebKit 2 to crash on Mac.
> > 
> > can we not do that since this is just a test issue.
> 
> Just managed to get to the bottom of this.
> 
> You're calling finishJSTest() too early, it should be called after
> accessibilityController.removeNotificationListener(). Calling
> removeNotificationListener() will actually cause the next test to crash. If
> finishJSTest() is called after
> accessibilityController.removeNotificationListener(), I no longer see the
> test failure locally.
> 
> Ryosuke, do you have any other additional comments, or do you think this
> patch is ready to go with that change?

That makes sense. thanks for taking a second look.
Comment 49 Ryosuke Niwa 2017-06-26 13:38:46 PDT
My r=me stands. Please land the patch with the test fix.
Comment 50 chris fleizach 2017-06-28 01:20:45 PDT
(In reply to Jonathan Bedard from comment #47)
> (In reply to chris fleizach from comment #45)
> > (In reply to Jonathan Bedard from comment #44)
> > > I'm going to look into this failure a bit today.  Looking over the content
> > > of this patch briefly, I suspect something wrong with infrastructure, but in
> > > the mean time, rolling out this change since it's pretty consistently
> > > causing WebKit 2 to crash on Mac.
> > 
> > can we not do that since this is just a test issue.
> 
> Just managed to get to the bottom of this.
> 
> You're calling finishJSTest() too early, it should be called after
> accessibilityController.removeNotificationListener(). Calling
> removeNotificationListener() will actually cause the next test to crash. If
> finishJSTest() is called after
> accessibilityController.removeNotificationListener(), I no longer see the
> test failure locally.
> 
> Ryosuke, do you have any other additional comments, or do you think this
> patch is ready to go with that change?

Jonathan are you going to land this patch again with your change?
Comment 51 Jonathan Bedard 2017-06-28 08:15:08 PDT
(In reply to chris fleizach from comment #50)
> ...
> 
> Jonathan are you going to land this patch again with your change?

I was not going to, although I can.  Since it's your change, Chris, I think you should be the one to land it.
Comment 52 chris fleizach 2017-06-29 00:05:05 PDT
Created attachment 314121 [details]
patch for landing
Comment 53 WebKit Commit Bot 2017-06-29 20:43:29 PDT
Comment on attachment 314121 [details]
patch for landing

Clearing flags on attachment: 314121

Committed r218986: <http://trac.webkit.org/changeset/218986>
Comment 54 WebKit Commit Bot 2017-06-29 20:43:31 PDT
All reviewed patches have been landed.  Closing bug.