Bug 213556 – JavaScript cannot be injected into iframes

• Component: WebKit API
• Status: RESOLVED FIXED

[Ali Juma]

WKWebView now allows injecting JavaScript into isolated worlds using evaluateJavaScript, but doesn't expose a way to inject WKUserScripts into isolated worlds.

WKUserScripts have important functionality that is missing from evaluateJavaScript. In particular, WKUserScripts can be injected into *all frames* rather than just the main frame, and can be automatically injected when the frame is loaded.

We'd love to move most of our JS injection into isolated worlds, but we need to be able to inject into non-main frames.

One approach to fixing this would be to add a public WKUserScript constructor that takes a WKContentWorld argument (there's already such a constructor in WKUserScriptPrivate.h).

[Ali Juma]

I've also filed FB7772359 for this.

[Alexey Proskuryakov]

rdar://problem/64700590

[Alex Christensen]

I'm wondering if these SPIs would meet your needs?

- (void)_evaluateJavaScript:(NSString *)javaScriptString inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld completionHandler:(void (^)(id, NSError * error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
...
- (void)_callAsyncJavaScript:(NSString *)functionBody arguments:(NSDictionary<NSString *, id> *)arguments inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld completionHandler:(void (^)(id, NSError *error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
...
- (void)_frames:(void (^)(_WKFrameTreeNode *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

[Ali Juma]

(In reply to Alex Christensen from comment #3)
> I'm wondering if these SPIs would meet your needs?
> 
> - (void)_evaluateJavaScript:(NSString *)javaScriptString
> inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld
> completionHandler:(void (^)(id, NSError * error))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> ...
> - (void)_callAsyncJavaScript:(NSString *)functionBody
> arguments:(NSDictionary<NSString *, id> *)arguments inFrame:(WKFrameInfo
> *)frame inContentWorld:(WKContentWorld *)contentWorld
> completionHandler:(void (^)(id, NSError *error))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> ...
> - (void)_frames:(void (^)(_WKFrameTreeNode *))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

My main concerns with this approach are how we would find out about new frames appearing in _frames, and how early we would be able to inject script into a new frame. We don't currently get much by way of navigation callbacks for subframes in public API -- decidePolicyForNavigationAction has a targetFrame, but it's presumably too early to inject at that point, and decidePolicyForNavigationResponse tells us whether the response is for a subframe, but doesn't include a WKFrameInfo. Periodically polling _frames doesn't seem like a great solution.

As a concrete example, we inject logic for form autofill. This code adds event listeners on |document| for events that could be related to a form being edited (like 'focus', 'blur', 'change'). It's important that this gets reliably injected before the user interacts with the page, so using a WKUserScript with WKUserScriptInjectionTimeAtDocumentStart works well.

[Brady Eidson]

Retitling to a more general "JavaScript cannot be injected into iframes"

[Brady Eidson]

Created attachment 403128 [details]
Patch

[Alex Christensen]

Comment on attachment 403128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403128&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:44
> +    API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { source, API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, contentWorld ? *contentWorld->_contentWorld : API::ContentWorld::pageContentWorld());

contentWorld is non-null, so this check and fallback should be unused.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:876
> +    if (details.type == WebCore::ExceptionDetails::Type::InvalidTargetFrame)

This should probably be a switch statement.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:882
> +    [userInfo setObject:static_cast<NSString *>(details.message) forKey:_WKJavaScriptExceptionMessageErrorKey];

I think this static_cast is syntactically unnecessary because of WTF::String::operator NSString *

[Darin Adler]

Comment on attachment 403128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403128&action=review

Should wait for EWS until I set review+, but looks good

> Source/WebCore/bindings/js/ExceptionDetails.h:46
>      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
>      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
>      // Once we stop building on those platforms we can remove this.

I don’t think we need this any more.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:32
> +#include <WebCore/ExceptionDetails.h>

This seems quite unfortunate. Why do we need to include this here now, instead of in the places that invoke the encode/decode for ExceptionDetails?

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:78
> +        return WEB_UI_STRING("JavaScript execution was targetted at a frame that can no longer be targetted", "WKErrorJavaScriptMissingFrameTarget description");

Looked up the spelling to be sure: it's "targeted" with only the two "t"s.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:824
>      [self _evaluateJavaScript:javaScriptString asAsyncFunction:NO withSourceURL:nil withArguments:nil forceUserGesture:YES inFrame:nil inWorld:contentWorld completionHandler:completionHandler];

Why aren’t we removing this? It’s no longer mentioned in the header.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:873
> +    RetainPtr<NSMutableDictionary> userInfo = adoptNS([[NSMutableDictionary alloc] init]);

I suggest using auto here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:887
> +    [userInfo setObject:localizedDescriptionForErrorCode(errorCode) forKey:NSLocalizedDescriptionKey];
> +    [userInfo setObject:static_cast<NSString *>(details.message) forKey:_WKJavaScriptExceptionMessageErrorKey];
> +    [userInfo setObject:@(details.lineNumber) forKey:_WKJavaScriptExceptionLineNumberErrorKey];
> +    [userInfo setObject:@(details.columnNumber) forKey:_WKJavaScriptExceptionColumnNumberErrorKey];
> +
> +    if (!details.sourceURL.isEmpty())
> +        [userInfo setObject:[NSURL _web_URLWithWTFString:details.sourceURL] forKey:_WKJavaScriptExceptionSourceURLErrorKey];

Things like this are so much nicer when they use dictionary literals; easier to read and more efficient. But the sourceURL part of this would be hard to do that way; it would have to be two literals. Or a mutation of a copy of the literal.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:959
> +            auto error = nsErrorFromExceptionDetails(*details);
> +            rawHandler(nil, error.get());

I think this would read better as a one-liner without the local variable.

[Geoffrey Garen]

Comment on attachment 403128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403128&action=review

r=me

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:77
> +    case WKErrorJavaScriptMissingFrameTarget:

Let's call this WKErrorJavaScriptInvalidFrameTarget since "invalid" is the word you used in headerdoc.

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:78
> +        return WEB_UI_STRING("JavaScript execution was targetted at a frame that can no longer be targetted", "WKErrorJavaScriptMissingFrameTarget description");

targetted => targeted

But also, maybe we can be clearer: JavaScript execution targeted an invalid frame.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:237
> + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.

=> Pass nil to target the main frame.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:287
> + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.

=> Pass nil to target the main frame.

[Brady Eidson]

Oh boy, do I have news for you all about the use of the misspelled "targetted" in WebKit... :)

(Will still fix it here)

[Brady Eidson]

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:237
> > + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.
> 
> => Pass nil to target the main frame.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:287
> > + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.
> 
> => Pass nil to target the main frame.

Not quite accurate, since you can also target the main frame with a valid WKFrameInfo targeting the main frame.

But I changed it to "Passing nil is equivalent to targeting the main frame."

[Brady Eidson]

Created attachment 403149 [details]
Patch

[Brady Eidson]

(In reply to Darin Adler from comment #8)
> Comment on attachment 403128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403128&action=review
> 
> Should wait for EWS until I set review+, but looks good
> 
> > Source/WebCore/bindings/js/ExceptionDetails.h:46
> >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> >      // Once we stop building on those platforms we can remove this.
> 
> I don’t think we need this any more.

The build bot errors with this patch are because I removed this 😭

[Brady Eidson]

Created attachment 403158 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #13)
> (In reply to Darin Adler from comment #8)
> > Comment on attachment 403128 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403128&action=review
> > 
> > Should wait for EWS until I set review+, but looks good
> > 
> > > Source/WebCore/bindings/js/ExceptionDetails.h:46
> > >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> > >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> > >      // Once we stop building on those platforms we can remove this.
> > 
> > I don’t think we need this any more.
> 
> The build bot errors with this patch are because I removed this 😭

E.g. https://ews-build.webkit.org/#/builders/22/builds/21940/steps/7/logs/errors

[Brady Eidson]

*** Bug 204557 has been marked as a duplicate of this bug. ***

[EWS]

Committed r263727: <https://trac.webkit.org/changeset/263727>

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

[Darin Adler]

(In reply to Brady Eidson from comment #15)
> (In reply to Brady Eidson from comment #13)
> > (In reply to Darin Adler from comment #8)
> > > > Source/WebCore/bindings/js/ExceptionDetails.h:46
> > > >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> > > >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> > > >      // Once we stop building on those platforms we can remove this.
> > > 
> > > I don’t think we need this any more.
> > 
> > The build bot errors with this patch are because I removed this 😭
> 
> E.g.
> https://ews-build.webkit.org/#/builders/22/builds/21940/steps/7/logs/errors

Seems like the comment is inaccurate because whatever the problem is, it's not about "older compilers".