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+
Patch (4.45 KB, patch)
2020-05-27 11:17 PDT, Per Arne Vollan
ews-feeder: commit-queue-
Patch (4.39 KB, patch)
2020-06-04 10:02 PDT, Per Arne Vollan
no flags
Patch (2.62 KB, patch)
2020-06-04 13:48 PDT, Per Arne Vollan
no flags
Per Arne Vollan
Comment 1 2020-05-27 09:54:02 PDT
Per Arne Vollan
Comment 2 2020-05-27 09:55:09 PDT
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
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
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
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
Note You need to log in before you can comment on or make changes to this bug.