Bug 209678 – API::PageConfiguration may have conflicting preference values between WebPreferences and WebPreferencesStore::ValueMap instance variables

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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

[Radar WebKit Bug Importer]

<rdar://problem/60981271>

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

The content of attachment 394754 [details] has been deleted for the following reason:

Wrong file uploaded

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 394790 [details]
Patch v5 (check EWS)

Crazy enough to work v2.  Fixed iOS build error.

[David Kilzer (:ddkilzer)]

Created attachment 394791 [details]
Patch v6

[David Kilzer (:ddkilzer)]

Comment on attachment 394791 [details]
Patch v6

Need to investigate LayoutTests/editing/undo-manager/ test failures.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 395063 [details]
Patch v7

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 395072 [details]
Patch v5

[David Kilzer (:ddkilzer)]

Comment on attachment 395072 [details]
Patch v5

Posted to wrong bug.

[David Kilzer (:ddkilzer)]

The content of attachment 395072 [details] has been deleted for the following reason:

Posted to wrong bug.

[Darin Adler]

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.

[Alex Christensen]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

*** Bug 209844 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

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.

[Brent Fulgham]

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?

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 395235 [details]
Patch v8

[David Kilzer (:ddkilzer)]

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

[Brent Fulgham]

Comment on attachment 395235 [details]
Patch v8

Looks good. I don't have any remaining concerns. r=me

[EWS]

Committed r259399: <https://trac.webkit.org/changeset/259399>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395235 [details].