Bug 31569 – Initial implementation of WebKitSharedScript and SharedScriptContext

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

This is another step of implementing GlobalScript feature, as described in bug 31317.

Includes initial IDL files for both, loading of initial script, firing of 'load' event and GlobalScriptContext lifetime.

It compiles with or without ENABLE_GLOBAL_SCRIPT, but JS bindings, tests and other port's build files will come separately (too avoid a huge patch).

The WebKitGlobalScript is the DOM object that returns, wrapped, from a WebKitGlobalScript constructor. It has a property 'context' that exposes inner global context.

The WebKitGlobalScriptRepository contains a list of all running GlobalScriptContexts and connects document to them using (origin, url, name) identification. Each GlobalScript holds a list of all documents that are connected to it. When Documents get 'detached', they get removed from that list, and once the document list of a GloballScriptContext becomes empty, it starts self-destruction timer which deletes it in a few seconds until there is a new Document that connects to it.

When GlobalScriptContext is initially created, the script indentified by URL is loaded and executed, after which all WebKitGlobalScripts that requested the same GlobalScriptContext will get the 'load' event and their 'context' property will return the GlobalScriptContext.

[Dmitry Titov]

Created attachment 43331 [details]
Proposed patch.

[Dmitry Titov]

Updating the title because of the name change. See bug 31317 for related discussion.

[Dmitry Titov]

Created attachment 43737 [details]
Patch updated for the name change.

[David Levin]

fwiw, I'm starting to review this. I should finish by tomorrow.

[David Levin]

Comment on attachment 43737 [details]
Patch updated for the name change.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        Initial implementation of WebKitSharedScript and SharedScriptContext
> +        https://bugs.webkit.org/show_bug.cgi?id=31569
> +
> +        No new tests since there is no bindings yet (soon to come).

s/is/are/

> +        * workers/WorkerScriptLoader.cpp:
> +        (WebCore::WorkerScriptLoader::loadSynchronously): Changed ASSERT because this loader can work with SharedScript as well.

Then the class is now misnamed.

> diff --git a/WebCore/shared-script/SharedScriptContext.cpp b/WebCore/shared-script/SharedScriptContext.cpp

regarding "shared-script" -- There seem to be a mix of directory names either NamedLikeThis or like-this (e.g. WebCore, WebKitTools), but in my cursory examination NamedLikeThis seems more common.

> +#include "config.h"
> +
> +#if ENABLE(SHARED_SCRIPT)
> +
> +#include "SharedScriptContext.h"

> +#include "SharedScriptController.h"

This one is out of place.

> +#include "Event.h"
> +#include "EventException.h"
> +#include "NotImplemented.h"
> +#include "ScriptSourceCode.h"
> +#include "scriptValue.h"

Should be ScriptValue.h

> +#include "SecurityOrigin.h"
> +#include "WebKitSharedScriptRepository.h"
> +
> +namespace WebCore {
> +
> +// Interval to allow orphaned SharedScriptContext to gain potential new owner.
> +// This can happen on navigation when both pages connect to the same SharedScript.
> +static const double destructionInterval = 5.0;

Is this part of the feature in dispute right now?

> +SharedScriptContext::SharedScriptContext(const String& name, const KURL& url, PassRefPtr<SecurityOrigin> origin)
> +    : m_name(name)
> +    , m_url(url)
> +    , m_script(new SharedScriptController(this))
> +    , m_destructionTimer(this, &SharedScriptContext::destructionTimerFired)
> +    , m_loaded(false)
> +{
> +    setSecurityOrigin(origin);
> +    ref(); // Matching deref is in destructionTimer callback.

s/destructionTimer/destructionTimerFired/ ?

> +void SharedScriptContext::addMessage(MessageDestination destination, MessageSource source, MessageType type, MessageLevel level, const String& message, unsigned lineNumber, const String& sourceURL)
> +{
> +    // FIXME: figure out console/incspector story for SharedScript. Maybe similar to SharedWorkers.

typo: incspector (throughout).

> +bool SharedScriptContext::matches(const String& name, PassRefPtr<SecurityOrigin> origin, const KURL& urlToMatch) const

Since this method doesn't take ownership of origin, it should be a SecurityOrigin* instead of a PassRefPtr<SecurityOrigin>.
Actually SecurityOrigin& would be even better since this method always wants it to be non-0.

> +{
...
> +    // If the names are both empty, compares the URLs instead per the Web Workers spec.

s/per/like/
I feel that you don't need to do something per the Web Workers spec since this isn't Web Workers but it is good to have a consistent pattern.
(It should be per the SharedScript spec.)

> +void SharedScriptContext::addToDocumentsList(ScriptExecutionContext* context)

Why does it say Document but take a ScriptExecutionContext?

I guess you don't need everything a document provides and the SEC is sufficient but it seems like a mixed message. I guess you could take a Document* but store it as an SEC.

> +void SharedScriptContext::removeFromDocumentList(ScriptExecutionContext* context)

Why not only start the timer when "if (!m_documentList.size())"?
> +    m_destructionTimer.startOneShot(destructionInterval);

> +void SharedScriptContext::postTask(PassOwnPtr<Task> task)
> +{
> +    // FIXME: Need to implement ScriptExecutionContext::postTaskToMainThread to share between Document and SharedScritpContext.

typo: SharedScritpContext.

> +ScriptExecutionContext* SharedScriptContext::scriptExecutionContext() const

Why is this const? It doesn't look like a const operation?

> +{
> +    return const_cast<SharedScriptContext*>(this);
> +}

> diff --git a/WebCore/shared-script/SharedScriptContext.h b/WebCore/shared-script/SharedScriptContext.h

> +namespace WebCore {
> +
> +class Document;
> +class SharedScriptController;
> +class KURL;

KURL is out of sort order.

> +class String;

> +class SharedScriptContext : public RefCounted<SharedScriptContext>, public ScriptExecutionContext, public EventTarget {
> +public:

> +    virtual ~SharedScriptContext();
Does the destructor need to be public? (Since this is ref counted, I expect that it should be either private or protected ).

> +    // JS wrapper and EventTaret support.

typo: EventTaret

> +    bool matches(const String& name, PassRefPtr<SecurityOrigin> origin, const KURL& urlToMatch) const;

remove param name "origin"


> diff --git a/WebCore/shared-script/WebKitSharedScript.cpp b/WebCore/shared-script/WebKitSharedScript.cpp

> +#include "Event.h"
> +#include "EventException.h"
> +#include "SharedScriptContext.h"

#include "Shared.. is out of order.

> +WebKitSharedScript::WebKitSharedScript(const String& url, const String& name, ScriptExecutionContext* context, ExceptionCode& ec)

> +    // FIXME: This should use the dynamic global scope (bug #27887)

Add "." to end of comment.

> diff --git a/WebCore/shared-script/WebKitSharedScript.h b/WebCore/shared-script/WebKitSharedScript.h

> +        void setContext(PassRefPtr<SharedScriptContext> context);
> +
> +        // When fired, will set the innerContext into this WebKitSharedScript and dispatch 'load' event.

s/will/this will/


> diff --git a/WebCore/shared-script/WebKitSharedScriptRepository.cpp b/WebCore/shared-script/WebKitSharedScriptRepository.cpp

> +#include "ActiveDOMObject.h"
> +#include "Document.h"
> +#include "Event.h"
> +#include "EventNames.h"
> +#include "ExceptionCode.h"
> +#include "SharedScriptContext.h"

Shared... is out of order.

> +#include "SecurityOrigin.h"

> +// Helper class to load the initial script on behalf of a SharedScript.
> +class ScriptLoader : public RefCounted<ScriptLoader>, public ActiveDOMObject, private WorkerScriptLoaderClient {

Should this be "SharedScriptLoader"? It doesn't load all scripts, so ScriptLoader seems a little too broad.

> +void ScriptLoader::notifyFinished()
> +{
> +    if (m_scriptLoader->failed())
> +        m_sharedScript->dispatchEvent(Event::create(eventNames().errorEvent, false, true));
> +    else {
> +        // If another loader have not yet initialized the SharedScriptContext - do so.

s/have/has/
s/-/,/

> +    m_sharedScript->unsetPendingActivity(m_sharedScript.get());
> +    unsetPendingActivity(this); // This frees this object - must be the last action in this function.

s/-/so it/

> +void WebKitSharedScriptRepository::documentDetached(Document* document)
> +{
> +    UNUSED_PARAM(document);

document is used.

> +    WebKitSharedScriptRepository& repository = instance();
> +    for (unsigned i = 0; i < repository.m_sharedScriptContexts.size(); i++)
> +        repository.m_sharedScriptContexts[i]->removeFromDocumentList(document);
> +}

> +void WebKitSharedScriptRepository::connectToSharedScript(PassRefPtr<WebKitSharedScript> sharedScript, const KURL& url, const String& name, ExceptionCode& ec)
> +{
> +    ASSERT(sharedScript->scriptExecutionContext()->securityOrigin()->canAccess(SecurityOrigin::create(url).get()));
> +    RefPtr<SharedScriptContext> innerContext = getSharedScriptContext(name, url);
> +
> +    innerContext->addToDocumentsList(sharedScript->scriptExecutionContext());

Should this document get added before the url check is done?

> +    if (innerContext->url() != url) {
> +        // SharedScript with same name but different URL already exists - return an error.
> +        ec = URL_MISMATCH_ERR;
> +        return;
> +    }
> +    // If SharedScriptContext is already running, just schedule a load event - otherwise, kick off a loader to load the script.
> +    if (innerContext->loaded())
> +        sharedScript->scheduleLoadEvent(innerContext);
> +    else {
> +        RefPtr<ScriptLoader> loader = adoptRef(new ScriptLoader(sharedScript, innerContext.release()));

Ideally there would be a create method (to match the general pattern followed for RefCounted objects).

> +        loader->load(url); // Pending activity will keep the loader alive.

This has the potential to do multiple loads of the same url for the same SharedScript. Then, it could fail for the first one and succeed later for another, which leaves me confused. A document gets some notification that the SharedScript failed to load but then it does load.

It would be better to only do one load. (You could track this by changing from a bool to track "loaded" to and enum to track when the laod starts as well or when it failed. It seems that if a shared script fails to load, then another document tries to get it, it shouldn't just appear.


> diff --git a/WebCore/shared-script/WebKitSharedScriptRepository.h b/WebCore/shared-script/WebKitSharedScriptRepository.h

> +namespace WebCore {
> +
> +typedef int ExceptionCode;
> +
> +class Document;
> +class KURL;
> +class SharedScriptContext;
> +class WebKitSharedScript;
> +class String;

Please sort.

[Dmitry Titov]

Created attachment 43806 [details]
Updated for David's comments.

[Dmitry Titov]

Created attachment 43809 [details]
Updated per David's comments.

[David Levin]

Comment on attachment 43809 [details]
Updated per David's comments.


> diff --git a/WebCore/sharedScript/SharedScriptContext.cpp b/WebCore/sharedScript/SharedScriptContext.cpp

> +void SharedScriptContext::addToDocumentsList(Document* context)

Would be nice to call it document instead of context.

> +void SharedScriptContext::removeFromDocumentList(Document* context)

Ditto.

> +void SharedScriptContext::removeFromDocumentList(Document* context)

Ditto.

It would be nice to not set-up the timer if it isn't needed.

> +    m_destructionTimer.startOneShot(destructionInterval);

[Dmitry Titov]

Thanks for review!

Capturing the details of the chat we had offline:

It makes sense to rename workers/WorkerScriptLoader.h,cpp into ScriptLoader and move it to WebCore/loader. It has no Worker semantics.

> > +static const double destructionInterval = 5.0;
> Is this part of the feature in dispute right now?

The "navigation hop" which would check the URL of the next document during navigation is indeed removed from the current implementation plan. However, I still need the final destruction of the SharedScriptContext to be an async operation, so even if I use 0 here there is still a possibility that a new document will come and hook to the still existing SharedScriptContext which is ok. It could be any random number here. It is basically 'non deterministic' delay.

> > +        loader->load(url); // Pending activity will keep the loader alive.
> 
> This has the potential to do multiple loads of the same url for the same
> SharedScript. Then, it could fail for the first one and succeed later for
> another, which leaves me confused. A document gets some notification that the
> SharedScript failed to load but then it does load.
> 
> It would be better to only do one load. (You could track this by changing from
> a bool to track "loaded" to and enum to track when the laod starts as well or
> when it failed. It seems that if a shared script fails to load, then another
> document tries to get it, it shouldn't just appear.

Unfortunately, at the moment we depend on a ThreadabkeLoader to load the script, which is using the owner Document. So if more then one page requests the same SharedScript and then one is destroyed while loading the script, the load may never happen so we start load on all documents is the script is not yet loaded and then the first one wins. It's the same algorithm as SharedWorkers do. It is another indication that we do need DocumentlessLoader (naming?) and I'lltry to address this later (XHR in SharedWorkers and SharedScript needs that too).

Will update with last comments and land.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/51374