Bug 188883 – [GTK][WPE] Add API to inject/register user content in isolated worlds

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are always using the main world.

[Carlos Garcia Campos]

Created attachment 347918 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[EWS Watchlist]

Attachment 347918 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitUserContentManager.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitUserContentManager.h"
ERROR: Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:44:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitUserContent.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitUserContent.h"
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 347918 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:44
> +    return *map.get().ensure(worldName, [&] { return API::UserContentWorld::worldWithName(String::fromUTF8(worldName)); }).iterator->value;

Nit: why [&]? It looks like the only thing you're capturing here is worldName. I would capture it explicitly.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:179
> + * webkit_user_style_sheet_new_for_world:

Hm... do we need it?

We definitely need webkit_user_script_new_for_world().

And it probably makes sense to add webkit_user_style_sheet_new_for_world() at the same time.

But would there be any real use case for it? The use case for running JS in isolated worlds is to prevent the JS from being accessible to the web page. Normally with a style you do want it to be accessible.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:304
> + * #WebKitUserContentManager::script-message-received signal,

Comma splice; use a semicolon

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:308
> + * See also webkit_user_content_manager_register_script_message_handler()

.

> Source/WebKit/WebProcess/UserContent/WebUserContentController.cpp:112
> -        worldMap().ensure(world.first, [&] { return std::make_pair(InjectedBundleScriptWorld::create(world.second), 1); });
> +        worldMap().ensure(world.first, [&] {
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +            // The GLib API doesn't allow to create script worlds from the UI process, and the
> +            // world name is used as the identifier. Use the existing world if found.
> +            if (auto* existingWorld = InjectedBundleScriptWorld::find(world.second))
> +                return std::make_pair(Ref<InjectedBundleScriptWorld>(*existingWorld), 1);
> +#endif
> +            return std::make_pair(InjectedBundleScriptWorld::create(world.second), 1);
> +        });

I don't understand your explanation; can you try explaining it again? It's expected for WebUserContentController::addUserContentWorlds to use existing worlds, rather than add new worlds, only on WPE/GTK?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:365
> +    // Check that the "window.webkit.messageHandlers" namespace doesn't exist in isolated worlds.

Is this comment correct? If it were, I would expect the JS to return 'n', correct? But instead it fails to execute, because you can't execute JS in a world that doesn't exist yet?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:384
> +    // Post message in nain world should fail.

main

If you want to test even more, you could proceed to hook up the msg handler for the main world, and then verify that the proper handler gets called when you post the message from WebExtensionTestScriptWorld and the main world.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 347918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347918&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:44
> > +    return *map.get().ensure(worldName, [&] { return API::UserContentWorld::worldWithName(String::fromUTF8(worldName)); }).iterator->value;
> 
> Nit: why [&]? It looks like the only thing you're capturing here is
> worldName. I would capture it explicitly.

You are right.

> > Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:179
> > + * webkit_user_style_sheet_new_for_world:
> 
> Hm... do we need it?

I don't know but we are also creating the user style sheets unconditionally in the main world. So, I just want to make sure the user content API is consistent and avoid having to add this in the future.

> We definitely need webkit_user_script_new_for_world().
> 
> And it probably makes sense to add webkit_user_style_sheet_new_for_world()
> at the same time.

Exactly.

> But would there be any real use case for it? The use case for running JS in
> isolated worlds is to prevent the JS from being accessible to the web page.
> Normally with a style you do want it to be accessible.

Yes, I don't know the use case to be honest.

> > Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:304
> > + * #WebKitUserContentManager::script-message-received signal,
> 
> Comma splice; use a semicolon
> 
> > Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:308
> > + * See also webkit_user_content_manager_register_script_message_handler()
> 
> .
> 
> > Source/WebKit/WebProcess/UserContent/WebUserContentController.cpp:112
> > -        worldMap().ensure(world.first, [&] { return std::make_pair(InjectedBundleScriptWorld::create(world.second), 1); });
> > +        worldMap().ensure(world.first, [&] {
> > +#if PLATFORM(GTK) || PLATFORM(WPE)
> > +            // The GLib API doesn't allow to create script worlds from the UI process, and the
> > +            // world name is used as the identifier. Use the existing world if found.
> > +            if (auto* existingWorld = InjectedBundleScriptWorld::find(world.second))
> > +                return std::make_pair(Ref<InjectedBundleScriptWorld>(*existingWorld), 1);
> > +#endif
> > +            return std::make_pair(InjectedBundleScriptWorld::create(world.second), 1);
> > +        });
> 
> I don't understand your explanation; can you try explaining it again? It's
> expected for WebUserContentController::addUserContentWorlds to use existing
> worlds, rather than add new worlds, only on WPE/GTK?

Right. We need to inject the scripts or message handers in the isolated world created by the web extension, but WebUserContentController always creates a new one.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:365
> > +    // Check that the "window.webkit.messageHandlers" namespace doesn't exist in isolated worlds.
> 
> Is this comment correct? If it were, I would expect the JS to return 'n',
> correct? But instead it fails to execute, because you can't execute JS in a
> world that doesn't exist yet?

No, it fails because it raises an exception. We could improve it by catching the exception and returning 'n', but I don't think it's worth it.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:384
> > +    // Post message in nain world should fail.
> 
> main
> 
> If you want to test even more, you could proceed to hook up the msg handler
> for the main world, and then verify that the proper handler gets called when
> you post the message from WebExtensionTestScriptWorld and the main world.

Tests can be improved, but I had to do this yesterday in a hurry because we are close to the release (and already frozen indeed).

[Carlos Garcia Campos]

Committed r235282: <https://trac.webkit.org/changeset/235282>