Bug 161919 – Automatic Text Replacement Testing in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Automatic text replacement settings are not properly implemented in a testing environment in WebKit2.  This causes a number of tests to fail and, in some cases, cause the failure of other tests.

[Jonathan Bedard]

Created attachment 288711 [details]
Patch

[Jonathan Bedard]

Comment on attachment 288711 [details]
Patch

Note that the uploaded patch will almost certainly fail EWS and is still a work in progress.

[Build Bot]

Comment on attachment 288711 [details]
Patch

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

New failing tests:
editing/inserting/typing-space-to-trigger-smart-link.html

[Build Bot]

Created attachment 288721 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288711 [details]
Patch

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

New failing tests:
editing/spelling/spelling-unified-emulation.html
editing/spelling/spellcheck-input-search-crash.html
editing/spelling/centering-misspelling-dots.html
editing/spelling/context-menu-suggestions.html
editing/spelling/misspelling-dots-dont-extend-beyond-words.html
editing/spelling/spelling.html

[Build Bot]

Created attachment 288722 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Ryosuke Niwa]

Just enabling the feature wouldn't work.  WebKitTestRunner needs to create its own NSSpellChecker with its own dictionary with a specific list of words, etc... see code in DumpRenderTree.

Also, there is a reason many of these tests are skipped / failing expectations even on WK1. NSSpellChecker learn the user behavior so running these tests repeatedly would result in the test results changing over time.

[Jonathan Bedard]

(In reply to comment #7)
> ...

Good to know.  I was searching for the tests which used the newly fixed functionality and attempting to run them.  Will keep skipping the spelling tests for the time being.

[Jonathan Bedard]

Created attachment 289072 [details]
Patch

[Jonathan Bedard]

Created attachment 289075 [details]
Patch

[Build Bot]

Comment on attachment 289075 [details]
Patch

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

New failing tests:
editing/inserting/typing-space-to-trigger-smart-link.html

[Build Bot]

Created attachment 289083 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Jonathan Bedard]

Created attachment 289084 [details]
Patch

[Alexey Proskuryakov]

> NSSpellChecker learn the user behavior so running these tests repeatedly would result in the test results changing over time.

Do we have a bug asking for a way to use an empty dictionary for tests?

[Build Bot]

Comment on attachment 289084 [details]
Patch

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

New failing tests:
editing/inserting/typing-space-to-trigger-smart-link.html

[Build Bot]

Created attachment 289096 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Michael Catanzaro]

Comment on attachment 289084 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:46
> +const TextCheckerState& TextChecker::mutableState()

Be sure to remove const from the return value here, since you've currently got mutableState() returning a non-mutable state!

[Jonathan Bedard]

Created attachment 289110 [details]
Patch

[Jonathan Bedard]

Created attachment 289118 [details]
Patch

[Jonathan Bedard]

Created attachment 289132 [details]
Patch

[Jonathan Bedard]

Comment on attachment 289132 [details]
Patch

Investigation needs to be done on the WebKit 1 test environment to see if the sandboxing precautions taken in this patch are needed.

[Jonathan Bedard]

Created attachment 289257 [details]
Patch

[Jonathan Bedard]

Created attachment 289266 [details]
Patch

[Build Bot]

Comment on attachment 289266 [details]
Patch

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

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html

[Build Bot]

Created attachment 289270 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289266 [details]
Patch

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

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html

[Build Bot]

Created attachment 289277 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jonathan Bedard]

Created attachment 289279 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 289279 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1624
>      if (!contextDocument() || !contextDocument()->frame())
> -        return;
> +        ASSERT_NOT_REACHED();

This change is not right.  It's totally possible for a context document to not have a frame.
e.g. the document just got detached from an iframe.
r- because of this change.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5981
> +    TextChecker::setTestingMode(true);
> +    TextChecker::setSmartInsertDeleteEnabled(!TextChecker::isSmartInsertDeleteEnabled());

This creates a new potential security vulnerability surface that any comprised web content process can now force UIProcess to override user defaults related to spellchecking features.
r- because of this.

I think we need some mechanism for TestRunner to opt-into using such a hook instead of trusting WebContent process to be only sending this IPC while running tests.

[Jonathan Bedard]

Comment on attachment 289279 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:5981
>> +    TextChecker::setSmartInsertDeleteEnabled(!TextChecker::isSmartInsertDeleteEnabled());
> 
> This creates a new potential security vulnerability surface that any comprised web content process can now force UIProcess to override user defaults related to spellchecking features.
> r- because of this.
> 
> I think we need some mechanism for TestRunner to opt-into using such a hook instead of trusting WebContent process to be only sending this IPC while running tests.

Actually, the whole point of TextChecker::setTestingMode(true) is that it prevents the setSmartInsertDeleteEnabled from setting user defaults.  In testing mode, user defaults are stored locally and propagated between WebKit2 processes, but not actually saved to user defaults.

Now, your point still stands.  Currently, if a compromised WebContent process were to send this IPC, it would place the UIProcess into a mode where it could never save user defaults, any changes would only effect the current set of WebKit2 processes.  I don't think this is a security risk, but I wouldn't describe it as desired behavior either.

[Ryosuke Niwa]

Comment on attachment 289279 [details]
Patch

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

>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:5981
>>> +    TextChecker::setSmartInsertDeleteEnabled(!TextChecker::isSmartInsertDeleteEnabled());
>> 
>> This creates a new potential security vulnerability surface that any comprised web content process can now force UIProcess to override user defaults related to spellchecking features.
>> r- because of this.
>> 
>> I think we need some mechanism for TestRunner to opt-into using such a hook instead of trusting WebContent process to be only sending this IPC while running tests.
> 
> Actually, the whole point of TextChecker::setTestingMode(true) is that it prevents the setSmartInsertDeleteEnabled from setting user defaults.  In testing mode, user defaults are stored locally and propagated between WebKit2 processes, but not actually saved to user defaults.
> 
> Now, your point still stands.  Currently, if a compromised WebContent process were to send this IPC, it would place the UIProcess into a mode where it could never save user defaults, any changes would only effect the current set of WebKit2 processes.  I don't think this is a security risk, but I wouldn't describe it as desired behavior either.

Oh, I see, that's a good point. That's probably less of an issue then. Please do fix the other issue in internals I pointed out though.

[Alexey Proskuryakov]

> This change is not right.  It's totally possible for a context document to not have a frame.
> e.g. the document just got detached from an iframe.
> r- because of this change.

But do we want the Internals method to be called in this scenario? Given that we don't have any way to implement it when there is no frame, I think that it should crash (but not only in debug - release should crash too).

[Ryosuke Niwa]

(In reply to comment #32)
> > This change is not right.  It's totally possible for a context document to not have a frame.
> > e.g. the document just got detached from an iframe.
> > r- because of this change.
> 
> But do we want the Internals method to be called in this scenario?

I don't think it matters whether we want it to be called or not.

> Given that we don't have any way to implement it when there is no frame, I think
> that it should crash (but not only in debug - release should crash too).

That's not what we do in any other internals method so we want to change that, I think we should have a discussion on webkit-dev first.

[Jonathan Bedard]

(In reply to comment #33)
> (In reply to comment #32)
> ....

Perhaps these asserts should be in a different patch, then.  If this was a change we want, there are a few other places it should be made as well.

[Alexey Proskuryakov]

This is actually what we do normally. Misuse of many testRunner or internals methods results in an assertion failure or a release mode crash. It would be counter-productive to make tests fail silently and mysteriously when we can already tell that the tests are written incorrectly.

Do you have a better suggestion for how to handle it in this specific case?

[Jonathan Bedard]

Created attachment 289393 [details]
Patch

[Jonathan Bedard]

This patch should resolve any bad behavior from a mis-behaiving WebContent process.

It also removes the asserts.  I think these should be placed in, but I also think that they are conceptually unrelated to this patch and should be done in a later patch.

[Ryosuke Niwa]

(In reply to comment #35)
> This is actually what we do normally. Misuse of many testRunner or internals
> methods results in an assertion failure or a release mode crash. It would be
> counter-productive to make tests fail silently and mysteriously when we can
> already tell that the tests are written incorrectly.
> 
> Do you have a better suggestion for how to handle it in this specific case?

Just throw a JS exception.  That's a lot better than crashing.

[Ryosuke Niwa]

Comment on attachment 289393 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +                editing/inserting/smart-link-when-caret-is-moved-before-URL.html
> +                editing/inserting/typing-space-to-trigger-smart-link.html

Indentation here is weird.  We normally just do:

Tests: editing/inserting/smart-link-when-caret-is-moved-before-URL.html
           editing/inserting/typing-space-to-trigger-smart-link.html

If you're going with your current formatting, the test names should be indented by exactly 4 spaces.

> Source/WebCore/ChangeLog:12
> +        Implemented test hooks for text replacement for WebKit2.  Note that spell checking has not been implemented, so most tests which use text replacement still fail.

We use single space after .
Also, please split this into two hard lines.

> Source/WebKit2/ChangeLog:8
> +        Implemented test hooks for text replacement for WebKit2.  Note that spell checking has not been implemented, so most tests which use text replacement still fail.

Ditto about two spaces after . and splitting this into two hard lines.

> Source/WebKit2/UIProcess/API/C/WKTextChecker.cpp:30
> +#if !defined(__APPLE__)

I think we normally use #if PLATFORM(MAC) || PLATFORM(IOS) for these if-defs.

[Jonathan Bedard]

Comment on attachment 289393 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKTextChecker.cpp:30
>> +#if !defined(__APPLE__)
> 
> I think we normally use #if PLATFORM(MAC) || PLATFORM(IOS) for these if-defs.

We do.  But since this is the implementation of functions declared in WKTextChecker.h, which does not include the PLATFORM() macros (and shouldn't, since it is a C API header) I used defined(__APPLE__) instead.

Actually, the real issue here is that I am combining two files (which is rarely done in these C API headers).  Originally, this should have been named WKWebTextChecker, since other ports define the WebTextChecker and this file provides C API hooks for that header.  However, it was instead named WKTextChecker, and excluded from builds on iOS and Mac.   Rather than try and rename this header to WKWebTextChecker (which would violate naming conventions on all of the functions in the header and mess up anyone else who was using the API) I simply added the new function to the header with and #ifdef that requires no additional headers.

[Jonathan Bedard]

Created attachment 289563 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289563 [details]
Patch

Clearing flags on attachment: 289563

Committed r206261: <http://trac.webkit.org/changeset/206261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> > Do you have a better suggestion for how to handle it in this specific case?

> Just throw a JS exception.  That's a lot better than crashing.

You say that it's better, but I don't see in what way this is better. Can you elaborate?

[Ryosuke Niwa]

(In reply to comment #44)
> > > Do you have a better suggestion for how to handle it in this specific case?
> 
> > Just throw a JS exception.  That's a lot better than crashing.
> 
> You say that it's better, but I don't see in what way this is better. Can
> you elaborate?

It doesn't significantly increase the total runtime of layout tests (because crashing WebKitTestRunner and running CrashReporter is VERY slow), and we get the JS stack trace which tells us exactly where a bad internals method call was made.

[Alexey Proskuryakov]

Crashing is better because it will prevent getting the bad test in the tree in the first place.

We have a lot of tests that have never been confirmed to work, and fail to verify what they claim to. Failing early and forcefully is much better.

I used to think that this was a well known policy for tests, although the last time I remember we discussed that was about ten years ago.

> and we get the JS stack trace which tells us exactly where a bad internals method call was made.

Do we? I think that the observable effect is that the test doesn't run to completion, and if it gets landed that way, no one will ever notice.

[Ryosuke Niwa]

(In reply to comment #46)
> Crashing is better because it will prevent getting the bad test in the tree
> in the first place.

I disagree. Getting stack trace is a PITA and it increases the test runtime.

> We have a lot of tests that have never been confirmed to work, and fail to
> verify what they claim to. Failing early and forcefully is much better.

Unless you have try & catch, throwing an exception would definitively show that something went wrong in the test in CONSOLE: message

> I used to think that this was a well known policy for tests, although the
> last time I remember we discussed that was about ten years ago.
> 
> > and we get the JS stack trace which tells us exactly where a bad internals method call was made.
> 
> Do we? I think that the observable effect is that the test doesn't run to
> completion, and if it gets landed that way, no one will ever notice.

Exceptions are reported to CONSOLE. Unless someone is calling waitUntilDone and never calling notifyDone when an exception happens, the test will run 'til completion.

[Csaba Osztrogonác]

(In reply to comment #42)
> Comment on attachment 289563 [details]
> Patch
> 
> Clearing flags on attachment: 289563
> 
> Committed r206261: <http://trac.webkit.org/changeset/206261>

It broke the Apple Mac cmake build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/8570

cc-ing Alex

[Alexey Proskuryakov]

> I disagree. Getting stack trace is a PITA 

Can you elaborate? 

> and it increases the test runtime.

Why is this a problem when you are only running your new test locally?

> Unless you have try & catch, throwing an exception would definitively show that something went wrong in the test in CONSOLE: message

There is a console message, but not a stack trace. It's very easy to overlook.

I do think that Internals and TestRunner misuse should result in as strong a failure as possible. Asserting is what we do in a lot of places, and this is no different.

[Jonathan Bedard]

(In reply to comment #48)
> ...

https://bugs.webkit.org/show_bug.cgi?id=162493 has a fix for this issue.

[Jeremy Huddleston Sequoia]

Still seeing build failure fallout from this on current trunk.  The previously mentioned fix doesn't seem to have addressed all the issues:

Following up in https://bugs.webkit.org/show_bug.cgi?id=163346