Bug 210582

Summary: REGRESSION(r260081) Broke iOS PLT due to InjectedBundle initialization
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, darin, ddkilzer, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Brent Fulgham 2020-04-15 17:16:56 PDT
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.
Comment 1 Brent Fulgham 2020-04-15 17:17:35 PDT
<rdar://problem/61838584>
Comment 2 Brent Fulgham 2020-04-15 17:19:43 PDT
Temporary workaround landed here:

Committed r260159: <https://trac.webkit.org/changeset/260159>
Comment 3 Brent Fulgham 2020-04-15 17:35:17 PDT
Created attachment 396597 [details]
Patch
Comment 4 David Kilzer (:ddkilzer) 2020-04-15 17:40:49 PDT
(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.
Comment 5 Darin Adler 2020-04-15 18:49:24 PDT
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.
Comment 6 Brent Fulgham 2020-04-15 19:55:47 PDT
I’ll do the small patch without the rename. I also see some bot redness which might be rename related, too.
Comment 7 Brent Fulgham 2020-04-15 19:57:00 PDT
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.
Comment 8 Brent Fulgham 2020-04-15 22:40:36 PDT
Created attachment 396623 [details]
Patch
Comment 9 Darin Adler 2020-04-16 09:43:01 PDT
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 10 Brent Fulgham 2020-04-16 10:11:51 PDT
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.
Comment 11 Brent Fulgham 2020-04-16 10:40:38 PDT
Created attachment 396669 [details]
Patch
Comment 12 Darin Adler 2020-04-16 11:12:45 PDT
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 13 Darin Adler 2020-04-16 11:12:59 PDT
Comment on attachment 396669 [details]
Patch

And you did that!
Comment 14 Darin Adler 2020-04-16 11:20:01 PDT
(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!
Comment 15 Brent Fulgham 2020-04-16 12:07:30 PDT
(In reply to Darin Adler from comment #13)
> Comment on attachment 396669 [details]
> Patch
> 
> And you did that!

:-D
Comment 16 EWS 2020-04-16 13:02:47 PDT
Committed r260215: <https://trac.webkit.org/changeset/260215>

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