Automation: scripts are executed in the wrong js context after a history navigation

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

I noticed this while working on bug #204151. After implementing the promises based execute script, tests imported/w3c/webdriver/tests/back/user_prompts.py and imported/w3c/webdriver/tests/forward/user_prompts.py started to fail, but only when the page cache was enabled. After a lot of debugging I realized the problem was that we wee using the script object from the previous frame js context after loading a page from the cache, because didClearWindowObjectForFrame() is not called in that case. We are caching the script object for every frame ID, and after a history navigation the frame ID is the same, but the frame js context isn't. That also mean we might be leaking the script objects in those cases, because we end up calling JSValueUnprotect with the wrong context. It would be easier to set the script object as a property of the global object and let JSC handle the lifetime. Instead of caching the script object and protect/unprotect it, we just check if the global object of the current js context has the property or not to get or create it.

[Carlos Garcia Campos]

Created attachment 384891 [details]
Patch

[BJ Burg]

Comment on attachment 384891 [details]
Patch

The overall approach seems fine to me, but I'm asking for a second review since I'm not super familiar with JSOjectRef API. What tests progress with this change?

[Devin Rousso]

Comment on attachment 384891 [details]
Patch

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

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:175
> +    auto webkitAutomationName = OpaqueJSString::tryCreate("WebKitAutomation"_s);

What about using `JSStringCreateWithUTF8CString` instead?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:179
> +        return const_cast<JSObjectRef>(JSObjectGetProperty(context, const_cast<JSObjectRef>(webkitAutomation), OpaqueJSString::tryCreate("automationSessionProxy"_s).get(), &exception));

Ditto (175)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:183
>      String script = StringImpl::createWithoutCopying(WebAutomationSessionProxyScriptSource, sizeof(WebAutomationSessionProxyScriptSource));
> +    JSEvaluateScript(context, OpaqueJSString::tryCreate(script).get(), nullptr, nullptr, 0, &exception);

Ditto (175) but with `JSStringCreateWithCharactersNoCopy` instead.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:192
> +    JSValueRef automationSessionProxyConstructor = JSObjectGetProperty(context, const_cast<JSObjectRef>(webkitAutomation), OpaqueJSString::tryCreate("AutomationSessionProxy"_s).get(), &exception);

Ditto (175)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:195
> +    JSObjectRef automationSessionProxy = JSObjectCallAsConstructor(context, const_cast<JSObjectRef>(automationSessionProxyConstructor), WTF_ARRAY_LENGTH(arguments), arguments, &exception);

Why not do this in the script itself and avoid having to call all of this API?  That way, you could invert this entire function and make it such that if `if (!JSObjectHasProperty(context, globalObject, webkitAutomationName.get()))` all you have to do is just evaluate the script, and then the rest of the logic to get the `automationSessionProxy` instance could be the same.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:198
> +    JSObjectSetProperty(context, const_cast<JSObjectRef>(webkitAutomation), OpaqueJSString::tryCreate("automationSessionProxy"_s).get(), automationSessionProxy, kJSPropertyAttributeNone, &exception);

Ditto (175)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:209
> +    auto webkitAutomationName = OpaqueJSString::tryCreate("WebKitAutomation"_s);

Ditto (175)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:215
> +    auto* scriptObject = const_cast<JSObjectRef>(JSObjectGetProperty(context, const_cast<JSObjectRef>(webkitAutomation), OpaqueJSString::tryCreate("automationSessionProxy"_s).get(), nullptr));

Ditto (175)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:28
> +var WebKitAutomation = {};

Does this mean that `WebKitAutomation` is now visible as `window.WebKitAutomation` in the page?

[Carlos Garcia Campos]

Comment on attachment 384891 [details]
Patch

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

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:175
>> +    auto webkitAutomationName = OpaqueJSString::tryCreate("WebKitAutomation"_s);
> 
> What about using `JSStringCreateWithUTF8CString` instead?

I'm just following what the existing code does, I guess we use the internal API to avoid having to call JSStringRelease. I don't mind to change it, but it's probably unrelated to this patch.

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:28
>> +var WebKitAutomation = {};
> 
> Does this mean that `WebKitAutomation` is now visible as `window.WebKitAutomation` in the page?

Yes. We could use a document property instead like chromium does. Or we could even try to use an isolated world for automation. The other option would be to keep the lifetime of the automation object from C++ like we currently do, but we need a reliable way to release the object when the js context of the frame changes.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #2)
> Comment on attachment 384891 [details]
> Patch
> 
> The overall approach seems fine to me, but I'm asking for a second review
> since I'm not super familiar with JSOjectRef API. What tests progress with
> this change?

Tests imported/w3c/webdriver/tests/back/user_prompts.py and imported/w3c/webdriver/tests/forward/user_prompts.py fail when using promises in script execution, see the bug description.

[Carlos Garcia Campos]

Comment on attachment 384891 [details]
Patch

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

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:195
>> +    JSObjectRef automationSessionProxy = JSObjectCallAsConstructor(context, const_cast<JSObjectRef>(automationSessionProxyConstructor), WTF_ARRAY_LENGTH(arguments), arguments, &exception);
> 
> Why not do this in the script itself and avoid having to call all of this API?  That way, you could invert this entire function and make it such that if `if (!JSObjectHasProperty(context, globalObject, webkitAutomationName.get()))` all you have to do is just evaluate the script, and then the rest of the logic to get the `automationSessionProxy` instance could be the same.

The thing is that we need to provide the constructor arguments from C++ implementation. If we always call the script we would be creating the js values every time, but they are only needed the first time.

[Carlos Garcia Campos]

Comment on attachment 384891 [details]
Patch

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

>>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:28
>>> +var WebKitAutomation = {};
>> 
>> Does this mean that `WebKitAutomation` is now visible as `window.WebKitAutomation` in the page?
> 
> Yes. We could use a document property instead like chromium does. Or we could even try to use an isolated world for automation. The other option would be to keep the lifetime of the automation object from C++ like we currently do, but we need a reliable way to release the object when the js context of the frame changes.

We can't use an isolated world, because we are expected to have access to global object properties from scripts.

[Joseph Pecoraro]

Comment on attachment 384891 [details]
Patch

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

>>>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:28
>>>> +var WebKitAutomation = {};
>>> 
>>> Does this mean that `WebKitAutomation` is now visible as `window.WebKitAutomation` in the page?
>> 
>> Yes. We could use a document property instead like chromium does. Or we could even try to use an isolated world for automation. The other option would be to keep the lifetime of the automation object from C++ like we currently do, but we need a reliable way to release the object when the js context of the frame changes.
> 
> We can't use an isolated world, because we are expected to have access to global object properties from scripts.

You could probably use PrivateNames, like JavaScriptCore does for its built-in scripts. It would never be observable from user script but would exist in their world.

There are two such facilities:

   1. <JavaScriptCore/JSObjectRefPrivate.h>
    JS_EXPORT bool JSObjectSetPrivateProperty(JSContextRef ctx, JSObjectRef object, JSStringRef propertyName, JSValueRef value);
    JS_EXPORT JSValueRef JSObjectGetPrivateProperty(JSContextRef ctx, JSObjectRef object, JSStringRef propertyName);

   2. <JavaScriptCore/PrivateName.h>
    These PrivateNames are like symbols.

[BJ Burg]

It definitely seems wrong for the automation property to be user visible. Carlos, can you try one of Joe's approaches which doesn't have this side-effect?

[Carlos Garcia Campos]

(In reply to Joseph Pecoraro from comment #8)
> Comment on attachment 384891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384891&action=review
> 
> >>>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:28
> >>>> +var WebKitAutomation = {};
> >>> 
> >>> Does this mean that `WebKitAutomation` is now visible as `window.WebKitAutomation` in the page?
> >> 
> >> Yes. We could use a document property instead like chromium does. Or we could even try to use an isolated world for automation. The other option would be to keep the lifetime of the automation object from C++ like we currently do, but we need a reliable way to release the object when the js context of the frame changes.
> > 
> > We can't use an isolated world, because we are expected to have access to global object properties from scripts.
> 
> You could probably use PrivateNames, like JavaScriptCore does for its
> built-in scripts. It would never be observable from user script but would
> exist in their world.

Great idea!

> There are two such facilities:
> 
>    1. <JavaScriptCore/JSObjectRefPrivate.h>
>     JS_EXPORT bool JSObjectSetPrivateProperty(JSContextRef ctx, JSObjectRef
> object, JSStringRef propertyName, JSValueRef value);
>     JS_EXPORT JSValueRef JSObjectGetPrivateProperty(JSContextRef ctx,
> JSObjectRef object, JSStringRef propertyName);

We can't use this, because the global object is not created with a JSClass definition.

>    2. <JavaScriptCore/PrivateName.h>
>     These PrivateNames are like symbols.

But we can definitely use this. Thanks!

[Carlos Garcia Campos]

Created attachment 387088 [details]
Updated patch

[Carlos Garcia Campos]

In file included from /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/Release/JavaScriptCore.framework/Headers/JSContextRef.h:29:
/Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/Release/JavaScriptCore.framework/Headers/JSObjectRef.h:588:22: note: 'JSObjectGetPropertyForKey' has been explicitly marked partial here
JS_EXPORT JSValueRef JSObjectGetPropertyForKey(JSContextRef ctx, JSObjectRef object, JSValueRef propertyKey, JSValueRef* exception) API_AVAILABLE(macos(10.15), ios(13.0));
                     ^
In file included from /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55.cpp:5:
/Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:227:40: note: enclose 'JSObjectGetPropertyForKey' in a __builtin_available check to silence this warning
        return const_cast<JSObjectRef>(JSObjectGetPropertyForKey(context, globalObject, scriptObjectSymbol, nullptr));
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~

Any idea how to fix this? I don't see any other use of __builtin_available in the code, so I guess there's another way?

[Radar WebKit Bug Importer]

<rdar://problem/58413615>

[BJ Burg]

(In reply to Carlos Garcia Campos from comment #12)
> In file included from
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/JavaScriptCore.framework/Headers/JSContextRef.h:29:
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/JavaScriptCore.framework/Headers/JSObjectRef.h:588:22: note:
> 'JSObjectGetPropertyForKey' has been explicitly marked partial here
> JS_EXPORT JSValueRef JSObjectGetPropertyForKey(JSContextRef ctx, JSObjectRef
> object, JSValueRef propertyKey, JSValueRef* exception)
> API_AVAILABLE(macos(10.15), ios(13.0));
>                      ^
> In file included from
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55.cpp:5:
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/Source/WebKit/
> WebProcess/Automation/WebAutomationSessionProxy.cpp:227:40: note: enclose
> 'JSObjectGetPropertyForKey' in a __builtin_available check to silence this
> warning
>         return const_cast<JSObjectRef>(JSObjectGetPropertyForKey(context,
> globalObject, scriptObjectSymbol, nullptr));
>                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Any idea how to fix this? I don't see any other use of __builtin_available
> in the code, so I guess there's another way?

I'll investigate.

[BJ Burg]

(In reply to Brian Burg from comment #14)
> (In reply to Carlos Garcia Campos from comment #12)
> > In file included from
> > /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> > Release/JavaScriptCore.framework/Headers/JSContextRef.h:29:
> > /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> > Release/JavaScriptCore.framework/Headers/JSObjectRef.h:588:22: note:
> > 'JSObjectGetPropertyForKey' has been explicitly marked partial here
> > JS_EXPORT JSValueRef JSObjectGetPropertyForKey(JSContextRef ctx, JSObjectRef
> > object, JSValueRef propertyKey, JSValueRef* exception)
> > API_AVAILABLE(macos(10.15), ios(13.0));
> >                      ^
> > In file included from
> > /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> > Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55.cpp:5:
> > /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/Source/WebKit/
> > WebProcess/Automation/WebAutomationSessionProxy.cpp:227:40: note: enclose
> > 'JSObjectGetPropertyForKey' in a __builtin_available check to silence this
> > warning
> >         return const_cast<JSObjectRef>(JSObjectGetPropertyForKey(context,
> > globalObject, scriptObjectSymbol, nullptr));
> >                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Any idea how to fix this? I don't see any other use of __builtin_available
> > in the code, so I guess there's another way?
> 
> I'll investigate.

That API is marked as not available because High Sierra is less than min version of 10.15.

[BJ Burg]

The patch builds for me locally using macOS 10.15 SDK (Catalina).

[Keith Miller]

(In reply to Carlos Garcia Campos from comment #12)
> In file included from
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/JavaScriptCore.framework/Headers/JSContextRef.h:29:
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/JavaScriptCore.framework/Headers/JSObjectRef.h:588:22: note:
> 'JSObjectGetPropertyForKey' has been explicitly marked partial here
> JS_EXPORT JSValueRef JSObjectGetPropertyForKey(JSContextRef ctx, JSObjectRef
> object, JSValueRef propertyKey, JSValueRef* exception)
> API_AVAILABLE(macos(10.15), ios(13.0));
>                      ^
> In file included from
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/WebKitBuild/
> Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55.cpp:5:
> /Volumes/Data/worker/macOS-High-Sierra-Release-Build-EWS/build/Source/WebKit/
> WebProcess/Automation/WebAutomationSessionProxy.cpp:227:40: note: enclose
> 'JSObjectGetPropertyForKey' in a __builtin_available check to silence this
> warning
>         return const_cast<JSObjectRef>(JSObjectGetPropertyForKey(context,
> globalObject, scriptObjectSymbol, nullptr));
>                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Any idea how to fix this? I don't see any other use of __builtin_available
> in the code, so I guess there's another way?

I think using __builtin_available is the way to go. JSC gets around this in testapi.cpp by #defining JSC_API_AVAILABLE away. But by this point we will have rewritten the JSC API header to use the system API availability attributes.

[Carlos Garcia Campos]

(In reply to Keith Miller from comment #17)
[...]
> I think using __builtin_available is the way to go. JSC gets around this in
> testapi.cpp by #defining JSC_API_AVAILABLE away. But by this point we will
> have rewritten the JSC API header to use the system API availability
> attributes.

If I understand __builtin_available correctly, that would use the API only when in newer version of macOS, but this is internal API that is available for sure when building WebKit, no? We don't have a fallback for previous versions. Another alternative would be to use the internal JSC API to get/set the property.

[Carlos Garcia Campos]

Created attachment 387202 [details]
Try to fix apple builds

[BJ Burg]

Comment on attachment 387202 [details]
Try to fix apple builds

r=me (with EWS)

[Keith Miller]

(In reply to Carlos Garcia Campos from comment #18)
> (In reply to Keith Miller from comment #17)
> [...]
> > I think using __builtin_available is the way to go. JSC gets around this in
> > testapi.cpp by #defining JSC_API_AVAILABLE away. But by this point we will
> > have rewritten the JSC API header to use the system API availability
> > attributes.
> 
> If I understand __builtin_available correctly, that would use the API only
> when in newer version of macOS, but this is internal API that is available
> for sure when building WebKit, no? We don't have a fallback for previous
> versions. Another alternative would be to use the internal JSC API to
> get/set the property.

Oh, ok, I misunderstood the warning. If you really want to use the public API there's probably a pragma to disable availability warnings. Using internal JSC API seems fine, though.

[Carlos Garcia Campos]

Committed r254328: <https://trac.webkit.org/changeset/254328>