Bug 210582 – REGRESSION(r260081) Broke iOS PLT due to InjectedBundle initialization

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/61838584>

[Brent Fulgham]

Temporary workaround landed here:

Committed r260159: <https://trac.webkit.org/changeset/260159>

[Brent Fulgham]

Created attachment 396597 [details]
Patch

[David Kilzer (:ddkilzer)]

(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.

[Darin Adler]

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.

[Brent Fulgham]

I’ll do the small patch without the rename. I also see some bot redness which might be rename related, too.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 396623 [details]
Patch

[Darin Adler]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 396669 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 396669 [details]
Patch

And you did that!

[Darin Adler]

(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!

[Brent Fulgham]

(In reply to Darin Adler from comment #13)
> Comment on attachment 396669 [details]
> Patch
> 
> And you did that!

:-D

[EWS]

Committed r260215: <https://trac.webkit.org/changeset/260215>

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