WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
209678
API::PageConfiguration may have conflicting preference values between WebPreferences and WebPreferencesStore::ValueMap instance variables
https://bugs.webkit.org/show_bug.cgi?id=209678
Summary
API::PageConfiguration may have conflicting preference values between WebPref...
David Kilzer (:ddkilzer)
Reported
2020-03-27 13:11:12 PDT
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())
Attachments
Patch v1 (check EWS)
(
deleted
)
2020-03-27 13:49 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Patch v2 (check EWS)
(2.22 KB, patch)
2020-03-27 13:52 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v3 (check EWS)
(5.96 KB, patch)
2020-03-27 18:59 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v4 (check EWS)
(22.88 KB, patch)
2020-03-27 21:14 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v5 (check EWS)
(22.88 KB, patch)
2020-03-27 21:21 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v6
(27.74 KB, patch)
2020-03-27 22:57 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v7
(36.90 KB, patch)
2020-03-31 10:39 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v5
(
deleted
)
2020-03-31 11:13 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Patch v8
(36.87 KB, patch)
2020-04-01 19:34 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-03-27 13:11:31 PDT
<
rdar://problem/60981271
>
David Kilzer (:ddkilzer)
Comment 2
2020-03-27 13:49:46 PDT
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)
Comment 3
2020-03-27 13:51:24 PDT
The content of
attachment 394754
[details]
has been deleted for the following reason: Wrong file uploaded
David Kilzer (:ddkilzer)
Comment 4
2020-03-27 13:52:34 PDT
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)
Comment 5
2020-03-27 18:46:18 PDT
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)
Comment 6
2020-03-27 18:59:33 PDT
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)
Comment 7
2020-03-27 20:29:12 PDT
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)
Comment 8
2020-03-27 21:14:22 PDT
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)
Comment 9
2020-03-27 21:19:22 PDT
(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)
Comment 10
2020-03-27 21:21:34 PDT
Created
attachment 394790
[details]
Patch v5 (check EWS) Crazy enough to work v2. Fixed iOS build error.
David Kilzer (:ddkilzer)
Comment 11
2020-03-27 22:57:30 PDT
Created
attachment 394791
[details]
Patch v6
David Kilzer (:ddkilzer)
Comment 12
2020-03-28 04:12:09 PDT
Comment on
attachment 394791
[details]
Patch v6 Need to investigate LayoutTests/editing/undo-manager/ test failures.
David Kilzer (:ddkilzer)
Comment 13
2020-03-28 10:41:30 PDT
(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)
Comment 14
2020-03-28 10:47:13 PDT
(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)
Comment 15
2020-03-28 10:50:19 PDT
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)
Comment 16
2020-03-30 18:48:08 PDT
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)
Comment 17
2020-03-31 10:39:12 PDT
Created
attachment 395063
[details]
Patch v7
David Kilzer (:ddkilzer)
Comment 18
2020-03-31 10:42:09 PDT
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)
Comment 19
2020-03-31 11:13:45 PDT
Created
attachment 395072
[details]
Patch v5
David Kilzer (:ddkilzer)
Comment 20
2020-03-31 11:14:23 PDT
Comment on
attachment 395072
[details]
Patch v5 Posted to wrong bug.
David Kilzer (:ddkilzer)
Comment 21
2020-03-31 11:14:54 PDT
The content of
attachment 395072
[details]
has been deleted for the following reason: Posted to wrong bug.
Darin Adler
Comment 22
2020-03-31 17:37:41 PDT
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 23
2020-03-31 17:44:54 PDT
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 24
2020-03-31 17:55:36 PDT
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)
Comment 25
2020-03-31 20:21:00 PDT
***
Bug 209844
has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 26
2020-03-31 21:33:11 PDT
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 27
2020-04-01 13:10:46 PDT
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 28
2020-04-01 18:38:24 PDT
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 29
2020-04-01 19:18:55 PDT
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)
Comment 30
2020-04-01 19:34:12 PDT
Created
attachment 395235
[details]
Patch v8
David Kilzer (:ddkilzer)
Comment 31
2020-04-01 19:34:55 PDT
(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 32
2020-04-02 11:00:09 PDT
Comment on
attachment 395235
[details]
Patch v8 Looks good. I don't have any remaining concerns. r=me
EWS
Comment 33
2020-04-02 11:37:12 PDT
Committed
r259399
: <
https://trac.webkit.org/changeset/259399
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 395235
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug