Bug 187565 – Add a FrameLoaderClient willInjectUserScriptForFrame callback

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Add a FrameLoaderClient willInjectUserScriptForFrame callback

[youenn fablet]

Created attachment 344783 [details]
WIP

[youenn fablet]

Created attachment 344786 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/loader/FrameLoaderClient.h:335:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 25 files


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

[Radar WebKit Bug Importer]

<rdar://problem/42095701>

[Sam Weinig]

What's the use case for this new API callback?

[youenn fablet]

(In reply to Sam Weinig from comment #5)
> What's the use case for this new API callback?

Apps on top of WebKit can currently rely on globalObjectIsAvailableForFrame to properly initialize worlds.
globalObjectIsAvailableForFrame is called at the time of receiving the first page byte.
This works well if the world is created before that, it fails otherwise and may lead to executing scripts in worlds that are not properly set up.

This new callback allows to properly set up worlds just before executing a user script.

[Brian Weinstein]

It looks like this doesn’t apply cleanly after https://trac.webkit.org/changeset/233752/webkit.

[youenn fablet]

(In reply to Brian Weinstein from comment #7)
> It looks like this doesn’t apply cleanly after
> https://trac.webkit.org/changeset/233752/webkit.

Yep, the call to this->loader().client().willInjectUserScript(world); probably needs to be rebased.

[Brian Weinstein]

(In reply to youenn fablet from comment #8)
> (In reply to Brian Weinstein from comment #7)
> > It looks like this doesn’t apply cleanly after
> > https://trac.webkit.org/changeset/233752/webkit.
> 
> Yep, the call to this->loader().client().willInjectUserScript(world);
> probably needs to be rebased.

It should go in Frame::injectUserScriptImmediately, correct? Just for my testing, I have:

void Frame::injectUserScriptImmediately(DOMWrapperWorld& world, const UserScript& script)
{
    auto* document = this->document();
    if (!document)
        return;
    if (script.injectedFrames() == InjectInTopFrameOnly && !isMainFrame())
        return;
    if (!UserContentURLPattern::matchesPatterns(document->url(), script.whitelist(), script.blacklist()))
        return;
    if (m_page)
        m_page->setAsRunningUserScripts();

    this->loader().client().willInjectUserScript(world);
    m_script->evaluateInWorld(ScriptSourceCode(script.source(), script.url()), world);
}

[youenn fablet]

Yes, I ll reupload a new version shortly after linch

[Alex Christensen]

(In reply to Sam Weinig from comment #5)
> What's the use case for this new API callback?

We are currently using globalObjectIsAvailableForFrame to set up things if we have a user script, but if we don't have a user script then use _addUserScriptImmediately (introduced in r233752) then things won't be set up when a user script is injected.  Moving the setup to this new callback fixes the problem without compatibility issues.

[youenn fablet]

Created attachment 344874 [details]
Rebasing

[EWS Watchlist]

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


ERROR: Source/WebCore/loader/FrameLoaderClient.h:335:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 25 files


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

[WebKit Commit Bot]

Comment on attachment 344874 [details]
Rebasing

Clearing flags on attachment: 344874

Committed r233782: <https://trac.webkit.org/changeset/233782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like the new test http/tests/contentextensions/injected-script-callback.html
Is crashing constantly on Debug WK2 platforms. 

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcontentextensions%2Finjected-script-callback.html

Link to crash log:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233795%20(4095)/http/tests/contentextensions/injected-script-callback-crash-log.txt

[youenn fablet]

Thanks truitt, I’ll look at it today

[Chris Dumez]

Committed r233799: <https://trac.webkit.org/changeset/233799>