The changes in r260081 began enforcing NSSecureCoding best practices, triggering a bug in InjectedBundleMac.mm, which is used by iOS as well. This patch does the following: 1. It delays parameter decoding until the appropriate platform initialization function has been found and used. 2. It renames InjectedBundleMac.mm to InjectedBundleCocoa.mm. 3. It removes a temporary workaround needed to get PLT running on iOS again.
<rdar://problem/61838584>
Temporary workaround landed here: Committed r260159: <https://trac.webkit.org/changeset/260159>
Created attachment 396597 [details] Patch
(In reply to Brent Fulgham from comment #3) > Created attachment 396597 [details] > Patch Would be REALLY helpful to post a diff of the code changes without the file rename. The InjectedBundle::initialize() method is too long to reasonably compare. Maybe try using Tools/Scripts/svn-create-patch to create the patch? I think it does this automatically.
Yes, we can’t review a patch that doesn’t show the diff. I am surprised, because usually patches I make that include a rename do include the diff. Another option is to simply do the rename separately in a second step. Seems likely there’s no urgency to the name change.
I’ll do the small patch without the rename. I also see some bot redness which might be rename related, too.
Volumes/Data/worker/macOS-Mojave-Debug-Build-EWS/build/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource54-mm.mm:7:10: fatal error: 'WebProcess/InjectedBundle/mac/InjectedBundleMac.mm' file not found So maybe Unified Sources don’t understand renames. I probably need to update another file. I’ll separate the rename.
Created attachment 396623 [details] Patch
Comment on attachment 396623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396623&action=review I’d love to say review+ here but I don’t really understand the philosophy behind the changes. > Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:105 > ASSERT([dictionary isKindOfClass:[NSDictionary class]]); Should this be a runtime check that returns false rather than just an assertion? Why can we assert it's a dictionary? We should assert only things we know to be true. > Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:175 > + ASSERT(!initializeFunction); > + ASSERT(!additionalClassesForParameterCoderFunction); What makes these things true on iOS family platforms? I don’t understand why we would compile code to find these things and then assert (only in debug builds) that we don’t have them. Why not compile out the code that looks for them instead? Or something like that? > Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:190 > + ASSERT(!initializeFunction); Same comment.
Comment on attachment 396623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396623&action=review >> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:105 >> ASSERT([dictionary isKindOfClass:[NSDictionary class]]); > > Should this be a runtime check that returns false rather than just an assertion? Why can we assert it's a dictionary? We should assert only things we know to be true. I think this is part of the API contract between the injected bundle and the UIProcess. The design assumes we are getting an NSDictionary with the relevant parameters encoded in it. If this not an NSDictionary, it seems like the UIProcess is violating the contract and maybe we should just exit. I'm not sure what the threat model would be here -- possibly a WKWebView client that is attempting to manipulate WebContent in ways that our APIs do not allow. >> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:175 >> + ASSERT(!additionalClassesForParameterCoderFunction); > > What makes these things true on iOS family platforms? I don’t understand why we would compile code to find these things and then assert (only in debug builds) that we don’t have them. Why not compile out the code that looks for them instead? Or something like that? I didn't compile things out because the existing code allows both paths on iOS (the C API style, and the newer ObjC plugin class). Because I was not sure if the C API support on iOS was intentional I left it in, but added these assertions to see if I could catch any test cases on any of our many platforms that hit this. I'm basically trying to make sure that either the C API 'WKBundleAdditionalClassesForParameterCoder' or the Objective C method 'additionalClassesForParameterCoderFunction' are used to seed the valid classes for the NSKeyedUnarchiver prior to attempting to decode the parameters.
Created attachment 396669 [details] Patch
Comment on attachment 396623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396623&action=review I’d love to say review+ here but I don’t really understand the philosophy behind the changes. >>> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:105 >>> ASSERT([dictionary isKindOfClass:[NSDictionary class]]); >> >> Should this be a runtime check that returns false rather than just an assertion? Why can we assert it's a dictionary? We should assert only things we know to be true. > > I think this is part of the API contract between the injected bundle and the UIProcess. The design assumes we are getting an NSDictionary with the relevant parameters encoded in it. > > If this not an NSDictionary, it seems like the UIProcess is violating the contract and maybe we should just exit. I'm not sure what the threat model would be here -- possibly a WKWebView client that is attempting to manipulate WebContent in ways that our APIs do not allow. If so, the same is true of the failure to *decode* the bundle parameter data. I am not sure what the threat model is there either. Why does this function have a return value at all? Why not just assert there is no exception? >>> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:175 >>> + ASSERT(!initializeFunction); >>> + ASSERT(!additionalClassesForParameterCoderFunction); >> >> What makes these things true on iOS family platforms? I don’t understand why we would compile code to find these things and then assert (only in debug builds) that we don’t have them. Why not compile out the code that looks for them instead? Or something like that? > > I didn't compile things out because the existing code allows both paths on iOS (the C API style, and the newer ObjC plugin class). Because I was not sure if the C API support on iOS was intentional I left it in, but added these assertions to see if I could catch any test cases on any of our many platforms that hit this. > > I'm basically trying to make sure that either the C API 'WKBundleAdditionalClassesForParameterCoder' or the Objective C method 'additionalClassesForParameterCoderFunction' are used to seed the valid classes for the NSKeyedUnarchiver prior to attempting to decode the parameters. If that’s the goal maybe we can write the code explicitly that way. Make sure that either one or another is used, and do the appropriate thing if they are not (crash, don’t load the bundle, whatever). The platform conditionals make it *harder* to get that right.
Comment on attachment 396669 [details] Patch And you did that!
(In reply to Darin Adler from comment #12) > I’d love to say review+ here but I don’t really understand the philosophy > behind the changes. Sorry, that was re-posted because my locally built Safari crashed!
(In reply to Darin Adler from comment #13) > Comment on attachment 396669 [details] > Patch > > And you did that! :-D
Committed r260215: <https://trac.webkit.org/changeset/260215> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396669 [details].