WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
212411
[Cocoa] Adopt read-only mode for preferences in the WebContent process
https://bugs.webkit.org/show_bug.cgi?id=212411
Summary
[Cocoa] Adopt read-only mode for preferences in the WebContent process
Per Arne Vollan
Reported
2020-05-27 09:31:49 PDT
The WebContent process should never write preferences. Adopt read-only mode for preferences.
Attachments
Patch
(4.32 KB, patch)
2020-05-27 09:55 PDT
,
Per Arne Vollan
darin
: review+
Details
Formatted Diff
Diff
Patch
(4.45 KB, patch)
2020-05-27 11:17 PDT
,
Per Arne Vollan
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(4.39 KB, patch)
2020-06-04 10:02 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(2.62 KB, patch)
2020-06-04 13:48 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2020-05-27 09:54:02 PDT
rdar://problem/63640788
Per Arne Vollan
Comment 2
2020-05-27 09:55:09 PDT
Created
attachment 400343
[details]
Patch
Darin Adler
Comment 3
2020-05-27 11:00:11 PDT
Comment on
attachment 400343
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=400343&action=review
> Source/WTF/wtf/PlatformHave.h:640 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) > +#define HAVE_CFPREFS_READONLY_SPI 1 > +#endif
This has the common mistake that Tim Horton alerted us to: checking IOS_FAMILY and then only checking __IPHONE_OS_VERSION_MIN_REQUIRED, which is frozen at a lower value on watchOS and tVOS. If our intent is to include only iOS itself, then please use PLATFORM(IOS), or add the relevant version checking for watchOS and tvOS. HAVE_CFNETWORK_METRICS_CONNECTION_PROPERTIES is one example of doing this correctly.
Per Arne Vollan
Comment 4
2020-05-27 11:05:50 PDT
(In reply to Darin Adler from
comment #3
)
> Comment on
attachment 400343
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=400343&action=review
> > > Source/WTF/wtf/PlatformHave.h:640 > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) > > +#define HAVE_CFPREFS_READONLY_SPI 1 > > +#endif > > This has the common mistake that Tim Horton alerted us to: checking > IOS_FAMILY and then only checking __IPHONE_OS_VERSION_MIN_REQUIRED, which is > frozen at a lower value on watchOS and tVOS. If our intent is to include > only iOS itself, then please use PLATFORM(IOS), or add the relevant version > checking for watchOS and tvOS. HAVE_CFNETWORK_METRICS_CONNECTION_PROPERTIES > is one example of doing this correctly.
Ah, good point! I will update the patch. Thanks for reviewing!
Per Arne Vollan
Comment 5
2020-05-27 11:17:04 PDT
Created
attachment 400353
[details]
Patch
EWS
Comment 6
2020-06-04 09:51:24 PDT
ChangeLog entry in Source/WebCore/PAL/ChangeLog contains OOPS!.
Per Arne Vollan
Comment 7
2020-06-04 10:02:26 PDT
Created
attachment 401031
[details]
Patch
Darin Adler
Comment 8
2020-06-04 10:52:16 PDT
Comment on
attachment 401031
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=401031&action=review
> Source/WTF/wtf/PlatformHave.h:651 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) \ > + || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \ > + || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 70000) \ > + || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000) > +#define HAVE_CFPREFS_READONLY_SPI 1 > +#endif
Could we try to keep these in alphabetical order rather than adding new ones at the bottom? Maybe we’re not even close to that right now? Normally we name these after the function rather than coining a new term, so it would be something like HAVE(CF_PREFS_SET_READ_ONLY).
EWS
Comment 9
2020-06-04 11:04:44 PDT
Committed
r262546
: <
https://trac.webkit.org/changeset/262546
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 401031
[details]
.
Per Arne Vollan
Comment 10
2020-06-04 12:02:38 PDT
(In reply to Darin Adler from
comment #8
)
> Comment on
attachment 401031
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=401031&action=review
> > > Source/WTF/wtf/PlatformHave.h:651 > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) \ > > + || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \ > > + || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 70000) \ > > + || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000) > > +#define HAVE_CFPREFS_READONLY_SPI 1 > > +#endif > > Could we try to keep these in alphabetical order rather than adding new ones > at the bottom? Maybe we’re not even close to that right now? > > Normally we name these after the function rather than coining a new term, so > it would be something like HAVE(CF_PREFS_SET_READ_ONLY).
I will create a follow-up patch for this. Thanks for reviewing!
Per Arne Vollan
Comment 11
2020-06-04 13:48:12 PDT
Reopening to attach new patch.
Per Arne Vollan
Comment 12
2020-06-04 13:48:15 PDT
Created
attachment 401075
[details]
Patch
Jacob Uphoff
Comment 13
2020-06-04 14:30:40 PDT
Reverted
r262546
for reason: This commit caused internal build failures Committed
r262571
: <
https://trac.webkit.org/changeset/262571
>
EWS
Comment 14
2020-06-05 12:07:25 PDT
Committed
r262639
: <
https://trac.webkit.org/changeset/262639
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 401031
[details]
.
Per Arne Vollan
Comment 15
2020-06-08 11:46:35 PDT
Landed follow-up fix in <
https://trac.webkit.org/changeset/262730/webkit
>.
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