Bug 187639

Summary: [GLIB] Add API to evaluate code using a given object to store global symbols
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
mcatanzaro: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews205 for win-future none

Description Carlos Garcia Campos 2018-07-13 05:42:59 PDT
Instead of adding symbols to the global object, they are added to a new object created in the context. This is similar to JS::Evaluate in spider monkey when a scopeChain parameter is passed.
Comment 1 Carlos Garcia Campos 2018-07-13 05:50:46 PDT
Created attachment 344938 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-07-13 05:56:55 PDT
Created attachment 344939 [details]
Patch
Comment 3 Carlos Garcia Campos 2018-07-13 06:25:44 PDT
Created attachment 344940 [details]
Patch

Extended the api to allow passing a custom class to be used for the object.
Comment 4 Carlos Garcia Campos 2018-07-14 04:41:54 PDT
Created attachment 345034 [details]
Patch

Updated the API again. I've realized that it doesn't make sense to limit the new object as property of the context global object, you might want to add the object to a namespace object, for example. Now that API returns the object as an out parameter so that you can add it to any other object.
Comment 5 Michael Catanzaro 2018-07-14 11:49:31 PDT
Comment on attachment 345034 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCContext.cpp:775
> + * in @uri; the value is one-based so the first line is 1. @uri and @line_number will be shown in exceptions,
> + * they don't affect the behavior of the script.

This is a comma splice (in the last sentence). You know by now your options for how to fix it.

> Source/JavaScriptCore/API/glib/JSCContext.cpp:793
> + * jsc_context_evaluate_in_object:

It's confusing that this function always creates the object that evaluates the JS. Would it not be useful to be able to pass an existing object?

> Source/JavaScriptCore/ChangeLog:9
> +        evaluated script are added as propertuies to the new object instead of to the context global object. This is

properties
Comment 6 EWS Watchlist 2018-07-14 12:24:52 PDT
Comment on attachment 345034 [details]
Patch

Attachment 345034 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8537661

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 7 EWS Watchlist 2018-07-14 12:25:04 PDT
Created attachment 345039 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Carlos Garcia Campos 2018-07-15 22:29:55 PDT
(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 345034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345034&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCContext.cpp:775
> > + * in @uri; the value is one-based so the first line is 1. @uri and @line_number will be shown in exceptions,
> > + * they don't affect the behavior of the script.
> 
> This is a comma splice (in the last sentence). You know by now your options
> for how to fix it.

Sure!

> > Source/JavaScriptCore/API/glib/JSCContext.cpp:793
> > + * jsc_context_evaluate_in_object:
> 
> It's confusing that this function always creates the object that evaluates
> the JS. Would it not be useful to be able to pass an existing object?

Indeed, that would be ideal and we wouldn't need the JSCClass parameter, but it's not possible without making a lot more changes to JSC. With current JSC, we need to use a new context to get its global object, so we can't pass an existing object. To use an existing object we would need a special scope mode that also "redirects" the assignments. Yusuke can explain much better than me. The good thing about this approach is that we can change the impl if JSC ever had this new scope keeping the existing API, so it can be optimized internally. Note that even if the function received an existing object, it would be an empty object in most of (if not all) the cases.

> > Source/JavaScriptCore/ChangeLog:9
> > +        evaluated script are added as propertuies to the new object instead of to the context global object. This is
> 
> properties

Oops.
Comment 9 Carlos Garcia Campos 2018-07-16 04:58:25 PDT
Committed r233844: <https://trac.webkit.org/changeset/233844>
Comment 10 Radar WebKit Bug Importer 2018-07-16 05:00:11 PDT
<rdar://problem/42236712>