Bug 161350 – Make PendingScript as ref-counted

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yusuke Suzuki]

PendingScript is used as the cached resource client. So they have copy constructor to add / remove clients to the cached resource.
However, there is only one place where this functionality is used.

Making PendingScript as ref-counted and making PendingScript as noncopyable clean up code.
And later, I plan to use PendingScript as the abstraction over CachedScript & ScriptModuleGraph.
And I plan to make PendingScript observable (adding PendingScriptClient).
It is good that PendingScript is noncopyable for that use cases.

[Yusuke Suzuki]

Created attachment 287356 [details]
Patch

[Yusuke Suzuki]

Created attachment 287364 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 287364 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Furthermore, by changing PendingScript noncopyable & ref-counted, we easily make it observable.

I think "we make it observable easily" reads better.

> Source/WebCore/ChangeLog:22
> +        Later, we plan to abstract ScriptModuleGraph (used for ES6 modules) and CachedScript by using
> +        this PendingScript.

I'm not sure what you mean by "we plan to abstract".
Do you mean that this observability is necessary to add the support for ES6 modules?

> Source/WebCore/dom/PendingScript.cpp:71
> +void PendingScript::propagateFinished()

I'd call this notifyClientFinished to match the naming convention elsewhere.

> Source/WebCore/dom/PendingScript.cpp:92
> +    m_client = client;

We should assert that m_client is not already set here.

> Source/WebCore/dom/PendingScript.cpp:99
> +    m_client = nullptr;

We should assert that the client matches m_client here.

> Source/WebCore/dom/PendingScript.h:49
> +    static Ref<PendingScript> create(Element&, CachedScript*);

Can we make this CachedScript&?

> Source/WebCore/dom/PendingScript.h:65
> +    bool needsLoading() const
> +    {
> +        return cachedScript();
> +    }

Nit: we should just write this in a single line: bool needsLoading() const { return cachedScript(); }

> Source/WebCore/dom/PendingScript.h:73
> +    void addClient(PendingScriptClient*);
> +    void removeClient(PendingScriptClient*);
> +

It's misleading to call these functions addClient and removeClient since there could be at most one client.
Maybe setClient and clearClient?

> Source/WebCore/dom/PendingScriptClient.h:36
> +    virtual void notifyFinished(PendingScript&) { }

Can't we make this a pure virtual function?

> Source/WebCore/dom/ScriptRunner.cpp:112
> +    for (size_t i = 0, length = scripts.size(); i < length; ++i) {

Use modern for loop: for (auto& currentScript: scripts).

> Source/WebCore/dom/ScriptRunner.cpp:116
> +        // The code is changed. Now if m_pendingAsyncScripts.take cannot find the target, it fills nullptr instead of empty PendingScript.

"The code is changed" is not a useful comment once this patch is landed.
I don't understand what you mean by "if m_pendingAsyncScripts.take cannot find the target, it fills nullptr instead of empty PendingScript"
Could you clarify?

> Source/WebCore/dom/ScriptRunner.h:61
> +    HashMap<ScriptElement*, RefPtr<PendingScript>> m_pendingAsyncScripts;

I think we can use Ref<PendingScript> for HashMap.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-156
> -    pendingScript.setWatchingForLoad(true);

It's not clear to me why we can remove the call to setWatchingForLoad.
Could you clarify this in the change log entry?

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-163
> -    m_host.stopWatchingForLoad(pendingScript.cachedScript());
> -    pendingScript.setWatchingForLoad(false);

Ditto.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-273
> -    pendingScript.setCachedScript(cachedScript);
> -    return true;

Again, it would be useful to have a per-function change log entry explaining
how this function's semantics have changed.
Namely, where are we setting pendingScript's cachedScript now?

[Yusuke Suzuki]

Comment on attachment 287364 [details]
Patch

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

Thank you for your review! I'll add the clarifications to the ChangeLog's each function lines.

>> Source/WebCore/ChangeLog:22
>> +        this PendingScript.
> 
> I'm not sure what you mean by "we plan to abstract".
> Do you mean that this observability is necessary to add the support for ES6 modules?

I would like to make this PendingScript the container of <ScriptModuleGraph|CachedScript>.
And instead of touching CachedScript in PendingScript directly, I would like to perform operations onto this PendingScript to hide the details of ScriptModuleGraph & CachedScript differences.

>> Source/WebCore/dom/PendingScript.cpp:71
>> +void PendingScript::propagateFinished()
> 
> I'd call this notifyClientFinished to match the naming convention elsewhere.

Thanks, fixed.

>> Source/WebCore/dom/PendingScript.cpp:92
>> +    m_client = client;
> 
> We should assert that m_client is not already set here.

OK, inserted.

>> Source/WebCore/dom/PendingScript.cpp:99
>> +    m_client = nullptr;
> 
> We should assert that the client matches m_client here.

Fixed.

>> Source/WebCore/dom/PendingScript.h:49
>> +    static Ref<PendingScript> create(Element&, CachedScript*);
> 
> Can we make this CachedScript&?

We can do so. Fixed.

>> Source/WebCore/dom/PendingScript.h:65
>> +    }
> 
> Nit: we should just write this in a single line: bool needsLoading() const { return cachedScript(); }

Fixed.

>> Source/WebCore/dom/PendingScript.h:73
>> +
> 
> It's misleading to call these functions addClient and removeClient since there could be at most one client.
> Maybe setClient and clearClient?

Right! We should do that. Fixed.

>> Source/WebCore/dom/PendingScriptClient.h:36
>> +    virtual void notifyFinished(PendingScript&) { }
> 
> Can't we make this a pure virtual function?

Yeah, currently, all the derived class implements it. Made it pure virtual function.

>> Source/WebCore/dom/ScriptRunner.cpp:112
>> +    for (size_t i = 0, length = scripts.size(); i < length; ++i) {
> 
> Use modern for loop: for (auto& currentScript: scripts).

Changed to use modern for loop. And apply the std::exchange onto the reference to deref early (in per-iteration, we deref the each script.).

>> Source/WebCore/dom/ScriptRunner.cpp:116
>> +        // The code is changed. Now if m_pendingAsyncScripts.take cannot find the target, it fills nullptr instead of empty PendingScript.
> 
> "The code is changed" is not a useful comment once this patch is landed.
> I don't understand what you mean by "if m_pendingAsyncScripts.take cannot find the target, it fills nullptr instead of empty PendingScript"
> Could you clarify?

As the bug https://bugs.webkit.org/show_bug.cgi?id=144050 describes, we don't know the reason why this element becomes nullptr...
Before this change, we see this `element` becomes nullptr.
This is considered because the m_pendingAsyncScripts.take may fail to find the entry.
Before this patch, m_pendingAsyncScripts.take returns PendingScript() when it fails. And PendingScript().element is nullptr.

After this patch, m_pendingAsyncScripts.take returns RefPtr<PendingScript>() when it fails, this should be nullptr.
So if the assumption done in the bug is correct, instead of checking `!element`, now we should check `!script`.

Maybe, the original description, `// Paper over https://bugs.webkit.org/show_bug.cgi?id=144050` is enough.
I'll drop this comment.

>> Source/WebCore/dom/ScriptRunner.h:61
>> +    HashMap<ScriptElement*, RefPtr<PendingScript>> m_pendingAsyncScripts;
> 
> I think we can use Ref<PendingScript> for HashMap.

Wow, MappedTakeType is great. Fixed.

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-156
>> -    pendingScript.setWatchingForLoad(true);
> 
> It's not clear to me why we can remove the call to setWatchingForLoad.
> Could you clarify this in the change log entry?

Since we use addClient for pendingScript.cachedScript() directly in the m_host.watchForLoad, we don't have a way to query whether the pending script is watched.
So, in the old implementation, we have the m_watchingForLoad : bool flag in PendingScript.
And here, we change it to true. That's the implementation of `pendingScript.setWatchingForLoad(true)`.

But now, we don't use CachedScript's addClient directly. Instead, we have the PendingScriptClient and setClient / clearClient.
So what this flag manages is now replaced by `m_client != nullptr`. So we don't need this `pendingScript.setWatchingForLoad(true);` call.
See the implementation of PendingScript::watchingForLoad().

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-163
>> -    pendingScript.setWatchingForLoad(false);
> 
> Ditto.

I've just noted the reason. And I added this clarification to the ChangeLog.

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:-273
>> -    return true;
> 
> Again, it would be useful to have a per-function change log entry explaining
> how this function's semantics have changed.
> Namely, where are we setting pendingScript's cachedScript now?

PendingScript's cachedScript is initialized in the constructor. So we dropped the setters like `pendingScript.setCachedScript`.
`PendingScript::create(*script, cachedScript)` in the below is the example.

[Yusuke Suzuki]

Created attachment 287411 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 287411 [details]
Patch

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

Add some notes.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:270
> +    return nullptr;

I doubt this `!cachedScript` case is effective...
As the original comment said, when the invalid src value is set, cachedScript becomes nullptr.
But in that case, we return early in ScriptElement::prepareScript, and we don't set m_willBeParserExecuted.
So this requestPendingScript won't be called at that time.

One case we can consider is the case that cachedScript is correct but already cleared at that time.
But this is also unlikely since m_cachedScript = nullptr is executed when (1) m_willBeParserExecuted is false or (2) ScriptElement is destroyed.
(1) is not related since requestPendingScript is executed for ScriptElement that m_willBeParserExecuted is true. And (2) should not be done since we hold the element.

[Yusuke Suzuki]

Comment on attachment 287411 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:270
>> +    return nullptr;
> 
> I doubt this `!cachedScript` case is effective...
> As the original comment said, when the invalid src value is set, cachedScript becomes nullptr.
> But in that case, we return early in ScriptElement::prepareScript, and we don't set m_willBeParserExecuted.
> So this requestPendingScript won't be called at that time.
> 
> One case we can consider is the case that cachedScript is correct but already cleared at that time.
> But this is also unlikely since m_cachedScript = nullptr is executed when (1) m_willBeParserExecuted is false or (2) ScriptElement is destroyed.
> (1) is not related since requestPendingScript is executed for ScriptElement that m_willBeParserExecuted is true. And (2) should not be done since we hold the element.

Another case we can consider is the case that this script element has inlined code and does not have the src attribute.
But this is also unlikely since there is no path to reach here.

This requestPendingScript is called from requestDeferredScript and requestParsingBlockingScript.
requestDeferredScript() is never called for the inlined script since it is only called when m_willExecuteWhenDocumentFinishedParsing = true and it is not set for the inlined script.
And requestParsingBlockingScript() is also never called for the inlined script reaching here (m_willBeParserExecuted = true case) is always  `m_readyToBeParserExecuted = true`, and in that case, requestParsingBlockingScript is never called.


I'll drop this case and insert assertion.

[Ryosuke Niwa]

Comment on attachment 287411 [details]
Patch

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

> Source/WebCore/ChangeLog:31
> +        (WebCore::PendingScript::isLoaded): When introducing ScriptModuleGraph, this will query to either CachedScript or ScriptModuleGraph. PendingScript will become the container for the both types.

This is a really long line. Can we wrap the line earlier?

> Source/WebCore/ChangeLog:45
> +        (WebCore::ScriptRunner::timerFired): We use `std::exchange` to retrieve the RefPtr<PendingScript> and make the original vector element nullptr. Without this, all the PendingScript is held until the iteration finishes.

Ditto about the long line.

> Source/WebCore/dom/PendingScript.cpp:53
> +Ref<PendingScript> PendingScript::create(Element& element, CachedScript& cachedScript)

We usually put the ::create function before the constructor for clarity.

> Source/WebCore/dom/PendingScript.cpp:60
> +Ref<PendingScript> PendingScript::create(Element& element, TextPosition scriptStartPosition)

Ditto.

> Source/WebCore/dom/PendingScript.cpp:86
> +    if (m_cachedScript)
> +        return m_cachedScript->isLoaded();
> +    return false;

It seems cleaner to do: return m_cachedScript && m_cachedScript->isLoaded() instead.

> Source/WebCore/dom/PendingScript.h:47
> +WTF_MAKE_NONCOPYABLE(PendingScript);
> +WTF_MAKE_FAST_ALLOCATED;

RefCounted puts both WTF_MAKE_NONCOPYABLE and WTF_MAKE_FAST_ALLOCATED so there's no need to do that here.

> Source/WebCore/dom/ScriptRunner.cpp:114
> +    for (auto& currentScript : scripts) {
> +        RefPtr<PendingScript> script = std::exchange(currentScript, nullptr);
> +        ASSERT(script);

Why do we need a local variable here?  Vector keeps script alive, right?

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:62
> +        if (m_parserBlockingScript->needsLoading() && m_parserBlockingScript->watchingForLoad())

It looks like we're always checking needsLoading and watchingForLoad together.
Why don't we add a helper function to check both?
In fact, maybe watchingForLoad should be checking both.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:67
> +        RefPtr<PendingScript> pendingScript = m_scriptsToExecuteAfterParsing.takeFirst();

We can just use auto here.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:240
> +    if (!(m_parserBlockingScript = requestPendingScript(element)))

I think it's cleaner to do the assignment in a line before if.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:255
> +    RefPtr<PendingScript> pendingScript;
> +    if (!(pendingScript = requestPendingScript(element)))

We can use auto. It's probably cleaner to do the assignment when you declare pendingScript and do: if (!pendingScript).

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:262
> -bool HTMLScriptRunner::requestPendingScript(PendingScript& pendingScript, Element* script) const
> +RefPtr<PendingScript> HTMLScriptRunner::requestPendingScript(Element* script) const

It looks like this function can now be a static local function instead of a member function.

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:309
> +                m_parserBlockingScript = PendingScript::create(*script, scriptStartPosition);

Nice! Looks a lot cleaner.  Can we also modify the line below so that both of them are one line statements to get rid of curly braces?

[Yusuke Suzuki]

Created attachment 287447 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 287447 [details]
Patch

Oops, I uploaded the patch before finding Ryosuke's second comment. I'll reflect it.

[Yusuke Suzuki]

Created attachment 287464 [details]
Patch

[Yusuke Suzuki]

rebasing...

[Yusuke Suzuki]

Created attachment 287470 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 287470 [details]
Patch

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

> Source/WebCore/dom/ScriptRunner.cpp:89
> +        m_scriptsToExecuteSoon.append(&m_pendingAsyncScripts.take(scriptElement)->get());

Ref::ptr().

> Source/WebCore/dom/ScriptRunner.cpp:113
> +        auto script = std::exchange(currentScript, nullptr);

Nit: Why don't we use WTFMove(currentScript) instead?

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:124
> -    executePendingScriptAndDispatchEvent(m_parserBlockingScript);
> +    RefPtr<PendingScript> protector = std::exchange(m_parserBlockingScript, nullptr);
> +    executePendingScriptAndDispatchEvent(*protector);

Can we make executePendingScriptAndDispatchEvent take RefPtr<>&& so that we can just do:
executePendingScriptAndDispatchEvent(WTFMove(m_parserBlockingScript));

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:242
> +    auto* scriptElement = toScriptElementIfPossible(script);
> +    ASSERT(scriptElement->willBeParserExecuted());
> +    auto* cachedScript = scriptElement->cachedScript().get();
> +    ASSERT(cachedScript);

You should have used reference instead but it's fine either way.

[Yusuke Suzuki]

Comment on attachment 287470 [details]
Patch

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

Thanks!

>> Source/WebCore/dom/ScriptRunner.cpp:89
>> +        m_scriptsToExecuteSoon.append(&m_pendingAsyncScripts.take(scriptElement)->get());
> 
> Ref::ptr().

Fixed.

>> Source/WebCore/dom/ScriptRunner.cpp:113
>> +        auto script = std::exchange(currentScript, nullptr);
> 
> Nit: Why don't we use WTFMove(currentScript) instead?

Looks better. Fixed.

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:124
>> +    executePendingScriptAndDispatchEvent(*protector);
> 
> Can we make executePendingScriptAndDispatchEvent take RefPtr<>&& so that we can just do:
> executePendingScriptAndDispatchEvent(WTFMove(m_parserBlockingScript));

Looks nice. Fixed.
To invoke the move constructor, I've changed the executePendingScriptAndDispatchEvent signature to executePendingScriptAndDispatchEvent(RefPtr<PendingScript>) and use WTFMove to move value from here.

>> Source/WebCore/html/parser/HTMLScriptRunner.cpp:242
>> +    ASSERT(cachedScript);
> 
> You should have used reference instead but it's fine either way.

OK, I'll use auto& scriptElement. And for cachedScript, we insert the assertion `assert(scriptElement.cachedScript())` and use `*scriptElement.cachedScript()` directly in PendingScript::create.

[Yusuke Suzuki]

Committed r205218: <http://trac.webkit.org/changeset/205218>