| Summary: | API::PageConfiguration may have conflicting preference values between WebPreferences and WebPreferencesStore::ValueMap instance variables | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||||||||
| Component: | WebKit2 | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, andersca, bfulgham, cdumez, darin, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Created attachment 394754 [details]
Patch v1 (check EWS)
This is literally the dumbest patch that would "fix" the issue. I want to see what fails.
The obvious failure point would be a crash if API::PageConfiguration::m_preferences isn't set before m_preferenceValues is used.
We probably need to require a WebPreferences object when creating an API::PageConfiguration object--or construct a default WebPreferences object--to prevent such the crash if it occurs.
The content of attachment 394754 [details] has been deleted for the following reason:
Wrong file uploaded
Created attachment 394755 [details]
Patch v2 (check EWS)
This is literally the dumbest patch that would "fix" the issue. I want to see what fails.
The obvious failure point would be a crash if API::PageConfiguration::m_preferences isn't set before m_preferenceValues is used.
We probably need to require a WebPreferences object when creating an API::PageConfiguration object--or construct a default WebPreferences object--to prevent such the crash if it occurs.
Sam, if you have insight about this topic (such as why API::PageConfiguration::preferenceValues() and API::PageConfiguration::preferences()::m_store aren't kept in sync), or potential future refactoring directions, I'd appreciate it. Created attachment 394777 [details]
Patch v3 (check EWS)
This is Patch v2 plus a required WebKit::WebPreferences argument to the API::PageConfiguration constructor.
I'm sure this isn't correct, but I want to see what fails with this change on EWS.
I think I have a better fix--remove API::PageConfiguration::preferenceValues() and then fix the compiler errors. There are only two source files where this needs to be done for the macOS port, and tests should tell me if the API::PageConfiguration::preferences() object is not set when the settings are changed. Created attachment 394789 [details]
Patch v4 (check EWS)
This is crazy enough it just might work.
It removes API::PageConfiguration::preferenceValues() and then fixes build errors by using setters on API::PageConfiguration::preferences() instead.
(In reply to David Kilzer (:ddkilzer) from comment #8) > Created attachment 394789 [details] > Patch v4 (check EWS) > > This is crazy enough it just might work. > > It removes API::PageConfiguration::preferenceValues() and then fixes build > errors by using setters on API::PageConfiguration::preferences() instead. Modulo some iOS build errors due to typos. Checking. Created attachment 394790 [details]
Patch v5 (check EWS)
Crazy enough to work v2. Fixed iOS build error.
Created attachment 394791 [details]
Patch v6
Comment on attachment 394791 [details]
Patch v6
Need to investigate LayoutTests/editing/undo-manager/ test failures.
(In reply to David Kilzer (:ddkilzer) from comment #12) > Comment on attachment 394791 [details] > Patch v6 > > Need to investigate LayoutTests/editing/undo-manager/ test failures. The tests that are failing all have this: <!DOCTYPE html> <!-- webkit-test-runner [ enableUndoManagerAPI=true ] --> Tim Horton was telling someone that there is a check (in WebKitTestRunner?) that determines whether a new WebContent process needs to be started based on whether configuration/preferences changed. I wonder if that's the issue here? I don't recall where that was. (In reply to David Kilzer (:ddkilzer) from comment #13) > (In reply to David Kilzer (:ddkilzer) from comment #12) > > Comment on attachment 394791 [details] > > Patch v6 > > > > Need to investigate LayoutTests/editing/undo-manager/ test failures. > > The tests that are failing all have this: > > <!DOCTYPE html> <!-- webkit-test-runner [ enableUndoManagerAPI=true ] --> > > Tim Horton was telling someone that there is a check (in WebKitTestRunner?) > that determines whether a new WebContent process needs to be started based > on whether configuration/preferences changed. I wonder if that's the issue > here? I don't recall where that was. I think this was struct TestOptions::hasSameInitializationOptions() in TestOptions.h, but `enableUndoManagerAPI` is checked there. Added some fprintf() debugging statements, and it looks like the WKWebViewConfiguration setting is not being propagated to the RuntimeEnabledFeatures setting while running LayoutTests/editing/undo-manager/undo-manager-add-item-exceptions.html. Time to print some stack traces. >>>>> TestController::platformCreateWebView(): options.enableUndoManagerAPI = YES >>>>> -[WKWebView initWithFrame:configuration:]: configuration._undoManagerAPIEnabled = YES >>>>> -[WKWebView _setupPageConfiguration:]: !![_configuration _undoManagerAPIEnabled] = YES _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. >>>>> RuntimeEnabledFeatures::setUndoManagerAPIEnabled(): was: NO now: NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO >>>>> RuntimeEnabledFeatures::undoManagerAPIEnabled(): NO So here's the fix for the LayoutTests/editing/undo-manager/ failures: -<!DOCTYPE html> <!-- webkit-test-runner [ enableUndoManagerAPI=true ] --> +<!DOCTYPE html> <!-- webkit-test-runner [ internal:UndoManagerAPIEnabled=true ] --> It turns out that for internal settings, the second way is the correct way to set this so that it gets propagated everywhere it needs to go. I can probably remove support for `enableUndoManagerAPI` from WebKitTestRunner/DRT and the TestOptions class. Created attachment 395063 [details]
Patch v7
Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-153 > - if (options.enableUndoManagerAPI) > - [copiedConfiguration _setUndoManagerAPIEnabled:YES]; Note that I did not remove WKWebViewConfiguration._undoManagerAPIEnabled because it was exposed as SPI in the original patch that added it, even though this call was the last user of it: @property (nonatomic, setter=_setUndoManagerAPIEnabled:) BOOL _undoManagerAPIEnabled WK_API_AVAILABLE(macos(10.15), ios(13.0)); Created attachment 395072 [details]
Patch v5
Comment on attachment 395072 [details]
Patch v5
Posted to wrong bug.
The content of attachment 395072 [details] has been deleted for the following reason:
Posted to wrong bug.
Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review >> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-153 >> - [copiedConfiguration _setUndoManagerAPIEnabled:YES]; > > Note that I did not remove WKWebViewConfiguration._undoManagerAPIEnabled because it was exposed as SPI in the original patch that added it, even though this call was the last user of it: > > @property (nonatomic, setter=_setUndoManagerAPIEnabled:) BOOL _undoManagerAPIEnabled WK_API_AVAILABLE(macos(10.15), ios(13.0)); Seems like you should at least mark it deprecated. Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review > Source/WebKit/ChangeLog:10 > + API::PageConfiguration::m_preferenceValues and I'm wondering why we had this in the first place. > Source/WebKit/UIProcess/API/mac/WKView.mm:1198 > + configuration->preferences()->setSystemLayoutDirection(static_cast<uint32_t>(toUserInterfaceLayoutDirection(self.userInterfaceLayoutDirection))); static_cast<TextDirection> > Tools/TestWebKitAPI/Tests/WebKit/mac/GetBackingScaleFactor.mm:71 > + WKRetainPtr<WKPageGroupRef> pageGroup = adoptWK(WKPageGroupCreateWithIdentifier(Util::toWK("GetBackingScaleFactorPageGroup").get())); auto >> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-153 >> - [copiedConfiguration _setUndoManagerAPIEnabled:YES]; > > Note that I did not remove WKWebViewConfiguration._undoManagerAPIEnabled because it was exposed as SPI in the original patch that added it, even though this call was the last user of it: > > @property (nonatomic, setter=_setUndoManagerAPIEnabled:) BOOL _undoManagerAPIEnabled WK_API_AVAILABLE(macos(10.15), ios(13.0)); Let's remove SPI if it's not used and hasn't been used by anything that needs it to stay around for binary compatibility. Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review >>>> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-153 >>>> - [copiedConfiguration _setUndoManagerAPIEnabled:YES]; >>> >>> Note that I did not remove WKWebViewConfiguration._undoManagerAPIEnabled because it was exposed as SPI in the original patch that added it, even though this call was the last user of it: >>> >>> @property (nonatomic, setter=_setUndoManagerAPIEnabled:) BOOL _undoManagerAPIEnabled WK_API_AVAILABLE(macos(10.15), ios(13.0)); >> >> Seems like you should at least mark it deprecated. > > Let's remove SPI if it's not used and hasn't been used by anything that needs it to stay around for binary compatibility. I'm going to break this part out out into a separate patch (switching from "enableUndoManagerAPI=true" to "internal:UndoManagerAPIEnabled=true" for layout tests. *** Bug 209844 has been marked as a duplicate of this bug. *** Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review >>>>> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-153 >>>>> - [copiedConfiguration _setUndoManagerAPIEnabled:YES]; >>>> >>>> Note that I did not remove WKWebViewConfiguration._undoManagerAPIEnabled because it was exposed as SPI in the original patch that added it, even though this call was the last user of it: >>>> >>>> @property (nonatomic, setter=_setUndoManagerAPIEnabled:) BOOL _undoManagerAPIEnabled WK_API_AVAILABLE(macos(10.15), ios(13.0)); >>> >>> Seems like you should at least mark it deprecated. >> >> Let's remove SPI if it's not used and hasn't been used by anything that needs it to stay around for binary compatibility. > > I'm going to break this part out out into a separate patch (switching from "enableUndoManagerAPI=true" to "internal:UndoManagerAPIEnabled=true" for layout tests. Turns out these changes to stop using "enableUndoManagerAPI" only work with the other part of this patch, so I'm leaving this here. Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:-2775 > - return store; Didn't this approach allow a given WebPageProxy to have settings that differed from the global preferences? Is this needed to support some form of per-page preference setting? Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review >> Source/WebKit/ChangeLog:10 >> + API::PageConfiguration::m_preferenceValues and > > I'm wondering why we had this in the first place. Well, because Chris Dumez thought in <https://bugs.webkit.org/attachment.cgi?id=394576&action=review> that this looked suspicious in WebPageProxy.cpp: preferencesStore().getBoolValueForKey(WebPreferencesKey::attachmentElementEnabledKey()) And that this looked more correct: m_preferences->attachmentElementEnabled() So in order to make the latter work in all cases, I eliminated API::PageConfiguration::m_preferenceValues and WebPageProxy::m_configurationPreferenceValues and forced all values to be stored on WebPreferences::store() instead, which is where they end up anyway the first time WebPageProxy::preferencesStore() is called. (As I mention in reply to Bren't comment below, WebPageProxy::preferencesStore() just looks like it's implementing lazy initialization in way that requires WebProxyPage::preferencesStore() to be called at least once before direct methods on m_preferences work as expected.) So I could have fixed this by somehow calling WebPageProxy::preferencesStore() somewhere in WKWebView's initialization code, but eliminating the lazy initialization seemed like a better solution. I didn't see what advantage the lazy initialization had, especially when WebPageProxy::preferencesStore() recopied every value in m_configurationPreferenceValues each time it was called. And as I mention a couple lines below, the only thing you have to do is to make sure that API::PageConfiguration::m_preferences is set to a valid WebPreferences object before setting values. (Maybe there was a reason that API::PageConfiguration also needed to lazily set its m_preferences instance variable, but I'm not sure what that was. In all but one case (in -[WKView initWithFrame:contextRef:pageGroupRef:relatedToPage:]), API::PageConfiguration::m_preferences already existed by the time it was needed. >> Source/WebKit/UIProcess/API/mac/WKView.mm:1198 >> + configuration->preferences()->setSystemLayoutDirection(static_cast<uint32_t>(toUserInterfaceLayoutDirection(self.userInterfaceLayoutDirection))); > > static_cast<TextDirection> Wil fix. >> Source/WebKit/UIProcess/WebPageProxy.cpp:-2775 >> - return store; > > Didn't this approach allow a given WebPageProxy to have settings that differed from the global preferences? > > Is this needed to support some form of per-page preference setting? No, I don't think it is. This method simply copies all the values from m_configurationPreferenceValues back into the m_preferences->store() (and does it _every_ time you call the method; -[WKWebView _setupPageConfiguration:] stores nearly 50 values in it!), so after the first time it's called, every page would have the what was in m_preferences->store() with values in m_configurationPreferenceValues overriding them. At best it was a way to override default preference settings with what was stored in m_configurationPreferenceValues, but using lazy initialization code with an implicit assumption that clients would _always_ call this: preferencesStore().getBoolValueForKey(WebPreferencesKey::attachmentElementEnabledKey()) Instead of this: m_preferences->attachmentElementEnabled() In practice, WebPageProxy::preferencesStore() would just have to be called once (to copy the values from m_configurationPreferenceValues into m_preferences->store()), then calling direct methods like m_preferences->attachmentElementEnabled() would just work after that. (This is undesirable because calling direct methods on m_preferences should always work the first time.) >> Tools/TestWebKitAPI/Tests/WebKit/mac/GetBackingScaleFactor.mm:71 >> + WKRetainPtr<WKPageGroupRef> pageGroup = adoptWK(WKPageGroupCreateWithIdentifier(Util::toWK("GetBackingScaleFactorPageGroup").get())); > > auto Wil fix this one and the line below it. Comment on attachment 395063 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review >>> Source/WebKit/UIProcess/API/mac/WKView.mm:1198 >>> + configuration->preferences()->setSystemLayoutDirection(static_cast<uint32_t>(toUserInterfaceLayoutDirection(self.userInterfaceLayoutDirection))); >> >> static_cast<TextDirection> > > Wil fix. Sorry, can't fix: In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource30-mm.mm:3: OpenSource/Source/WebKit/UIProcess/API/mac/WKView.mm:1198:60: error: reference to type 'const uint32_t' (aka 'const unsigned int') could not bind to an rvalue of type 'WebCore::TextDirection' configuration->preferences()->setSystemLayoutDirection(static_cast<WebCore::TextDirection>(toUserInterfaceLayoutDirection(self.userInterfaceLayoutDirection))); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource30-mm.mm:1: In file included from OpenSource/Source/WebKit/UIProcess/API/mac/WKContentViewMac.mm:32: In file included from OpenSource/Source/WebKit/UIProcess/WebPageProxy.h:70: OpenSource/Source/WebKit/UIProcess/WebPreferences.h:62:32: note: passing argument to parameter 'value' here FOR_EACH_WEBKIT_PREFERENCE(DECLARE_PREFERENCE_GETTER_AND_SETTERS) ^ 1 error generated. Created attachment 395235 [details]
Patch v8
(In reply to David Kilzer (:ddkilzer) from comment #30) > Created attachment 395235 [details] > Patch v8 This fixed Alex's review feedback that didn't cause compiler errors. Comment on attachment 395235 [details]
Patch v8
Looks good. I don't have any remaining concerns. r=me
Committed r259399: <https://trac.webkit.org/changeset/259399> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395235 [details]. |
API::PageConfiguration may have conflicting preference values between WebPreferences and WebPreferencesStore::ValueMap instance variables. In other words, it's possible for this invariant NOT to be true for any preference (not just attachmentElementEnabled) on an API::PageConfiguration object (named `pageConfiguration`): pageConfiguration.preferences()->attachmentElementEnabled() == pageConfiguration.preferencesStore().getBoolValueForKey(WebPreferencesKey::attachmentElementEnabledKey())