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.
Created attachment 288711 [details] Patch
Comment on attachment 288711 [details] Patch Note that the uploaded patch will almost certainly fail EWS and is still a work in progress.
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
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
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
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
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.
(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.
Created attachment 289072 [details] Patch
Created attachment 289075 [details] Patch
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
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
Created attachment 289084 [details] Patch
> 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?
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
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
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!
Created attachment 289110 [details] Patch
Created attachment 289118 [details] Patch
Created attachment 289132 [details] Patch
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.
Created attachment 289257 [details] Patch
Created attachment 289266 [details] Patch
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
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
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
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
Created attachment 289279 [details] Patch
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.
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.
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.
> 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).
(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.
(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.
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?
Created attachment 289393 [details] Patch
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.
(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.
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.
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.
Created attachment 289563 [details] Patch
Comment on attachment 289563 [details] Patch Clearing flags on attachment: 289563 Committed r206261: <http://trac.webkit.org/changeset/206261>
All reviewed patches have been landed. Closing bug.
> > 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?
(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.
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.
(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.
(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
> 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.
(In reply to comment #48) > ... https://bugs.webkit.org/show_bug.cgi?id=162493 has a fix for this issue.
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