Bug 204880 - Automation: scripts are executed in the wrong js context after a history navigation
Summary: Automation: scripts are executed in the wrong js context after a history navi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 204151
  Show dependency treegraph
 
Reported: 2019-12-05 02:28 PST by Carlos Garcia Campos
Modified: 2020-01-10 00:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (12.36 KB, patch)
2019-12-05 02:41 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (9.13 KB, patch)
2020-01-08 02:31 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix apple builds (9.43 KB, patch)
2020-01-09 01:56 PST, Carlos Garcia Campos
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-12-05 02:28:49 PST
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.
Comment 1 Carlos Garcia Campos 2019-12-05 02:41:03 PST
Created attachment 384891 [details]
Patch
Comment 2 Brian Burg 2020-01-06 17:21:36 PST
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?
Comment 3 Devin Rousso 2020-01-06 20:16:21 PST
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?
Comment 4 Carlos Garcia Campos 2020-01-07 04:01:57 PST
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.
Comment 5 Carlos Garcia Campos 2020-01-07 04:03:57 PST
(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.
Comment 6 Carlos Garcia Campos 2020-01-07 08:35:49 PST
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.
Comment 7 Carlos Garcia Campos 2020-01-07 08:49:57 PST
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.
Comment 8 Joseph Pecoraro 2020-01-07 10:02:19 PST
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.
Comment 9 Brian Burg 2020-01-07 16:09:28 PST
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?
Comment 10 Carlos Garcia Campos 2020-01-08 02:22:45 PST
(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!
Comment 11 Carlos Garcia Campos 2020-01-08 02:31:56 PST
Created attachment 387088 [details]
Updated patch
Comment 12 Carlos Garcia Campos 2020-01-08 02:47:37 PST
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?
Comment 13 Radar WebKit Bug Importer 2020-01-08 10:47:49 PST
<rdar://problem/58413615>
Comment 14 Brian Burg 2020-01-08 10:58:30 PST
(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.
Comment 15 Brian Burg 2020-01-08 14:14:47 PST
(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.
Comment 16 Brian Burg 2020-01-08 14:40:30 PST
The patch builds for me locally using macOS 10.15 SDK (Catalina).
Comment 17 Keith Miller 2020-01-08 14:51:33 PST
(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.
Comment 18 Carlos Garcia Campos 2020-01-09 01:07:13 PST
(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.
Comment 19 Carlos Garcia Campos 2020-01-09 01:56:27 PST
Created attachment 387202 [details]
Try to fix apple builds
Comment 20 Brian Burg 2020-01-09 11:19:48 PST
Comment on attachment 387202 [details]
Try to fix apple builds

r=me (with EWS)
Comment 21 Keith Miller 2020-01-09 11:23:59 PST
(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.
Comment 22 Carlos Garcia Campos 2020-01-10 00:42:18 PST
Committed r254328: <https://trac.webkit.org/changeset/254328>