Bug 51723 – Avoid manual ref/deref in AsyncScriptRunner by using PendingScript

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Avoid manual ref/deref in AsyncScriptRunner by using PendingScript

[Tony Gentilcore]

Created attachment 77642 [details]
Patch

[Darin Adler]

Comment on attachment 77642 [details]
Patch

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

I’m going to say review+ but I do wonder why the PendingScript changes here were needed at all.

> WebCore/ChangeLog:23
> +        * dom/PendingScript.cpp:
> +        (WebCore::PendingScript::PendingScript):
> +        * dom/PendingScript.h:

No explanation of why you changed this class to no longer inline its constructors. It doesn’t seem required by the rest of the patch. And you didn’t touch the assignment operator. I’m a bit puzzled. Why?

> WebCore/dom/AsyncScriptRunner.cpp:48
>      for (size_t i = 0; i < m_scriptsToExecuteSoon.size(); ++i) {
> -        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
>          m_document->decrementLoadEventDelayCount();
>      }

No braces needed now that this is a 1-line for loop body.

> WebCore/dom/AsyncScriptRunner.cpp:61
> +    PendingScript pendingScript;
> +    pendingScript.adoptElement(element);

Should we make a suitable constructor. It seems it would be better to construct the PendingScript ready to go rather than empty-constructing it and then setting the two pieces separately.

Also, the function name adoptElement is not good! This function doesn’t “adopt” an element. In WebKit we call a function “adopt” when it takes ownership of something in a unusual way, such as when a String takes over storage formerly owned by a Vector or a smart pointer takes a reference in the function adoptRef.

> WebCore/dom/PendingScript.h:44
>  class PendingScript : public CachedResourceClient {

The dual nature of the PendingScript class is not great. We treat these as data objects that we can copy around, but they are also CachedResourceClient objects that can perform loading. It seems impossible to correctly copy one while its loading.

> WebCore/dom/PendingScript.h:47
> +    PendingScript(const PendingScript& other);

No need for the argument name “other” here.

[Eric Seidel (no email)]

I would suggest running WebCore/benchmarks/parser/ before landing just to make sure.

[Tony Gentilcore]

Created attachment 77908 [details]
Patch

[Tony Gentilcore]

> No explanation of why you changed this class to no longer inline its constructors. It doesn’t seem required by the rest of the patch. And you didn’t touch the assignment operator. I’m a bit puzzled. Why?
Reverted.

> No braces needed now that this is a 1-line for loop body.
Done.

> Should we make a suitable constructor. It seems it would be better to construct the PendingScript ready to go rather than empty-constructing it and then setting the two pieces separately.
Agreed. Done.

> Also, the function name adoptElement is not good! This function doesn’t “adopt” an element. In WebKit we call a function “adopt” when it takes ownership of something in a unusual way, such as when a String takes over storage formerly owned by a Vector or a smart pointer takes a reference in the function adoptRef.
Renamed to setElement().

> The dual nature of the PendingScript class is not great. We treat these as data objects that we can copy around, but they are also CachedResourceClient objects that can perform loading. It seems impossible to correctly copy one while its loading.
It is quite possible I am missing something, so please explain if I am. But the CachedResourceClient here is never actually used. It only exist to keep a reference so that the CachedResource's buffer isn't purged. This allows the owner of a PendingScript to not have to worry about the data being purged out from under it.

> No need for the argument name “other” here.
Moot now that it is inlined again.

> I would suggest running WebCore/benchmarks/parser/ before landing just to make sure.
I take it this is moot now that the inlining in PendingScript isn't changed?

[Ryosuke Niwa]

Comment on attachment 77908 [details]
Patch

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

> WebCore/dom/AsyncScriptRunner.cpp:87
> +        CachedScript* cachedScript = scripts[i].cachedScript();
> +        RefPtr<Element> element = scripts[i].releaseElementAndClear();
> +        toScriptElement(element.get())->execute(cachedScript);

releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient().  Are you sure we want to do this?

[Tony Gentilcore]

(In reply to comment #6)
> (From update of attachment 77908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77908&action=review
> 
> > WebCore/dom/AsyncScriptRunner.cpp:87
> > +        CachedScript* cachedScript = scripts[i].cachedScript();
> > +        RefPtr<Element> element = scripts[i].releaseElementAndClear();
> > +        toScriptElement(element.get())->execute(cachedScript);
> 
> releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient().  Are you sure we want to do this?

Yes, they are different clients. releaseElementAndClear removes the PendingScript client and execute removes the ScriptElement client.

[Ryosuke Niwa]

Comment on attachment 77908 [details]
Patch

(In reply to comment #7)
> > releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient().  Are you sure we want to do this?
> 
> Yes, they are different clients. releaseElementAndClear removes the PendingScript client and execute removes the ScriptElement client.

OK, now I see it. r=me.

[WebKit Commit Bot]

Comment on attachment 77908 [details]
Patch

Rejecting attachment 77908 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
hbc/WebCorePrefix.h -c /Projects/CommitQueue/WebCore/html/HTMLVideoElement.cpp -o /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLVideoElement.o

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLScriptRunner.o /Projects/CommitQueue/WebCore/html/parser/HTMLScriptRunner.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/7345060

[Build Bot]

Attachment 77908 [details] did not build on win:
Build output: http://queues.webkit.org/results/7266406

[Tony Gentilcore]

Created attachment 77924 [details]
Patch for landing

[Tony Gentilcore]

> ** BUILD FAILED **

Was a merge error with http://trac.webkit.org/changeset/74995. Just needed to rename a new call to setElement.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 77924 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 77924 [details]
Patch for landing

Clearing flags on attachment: 77924

Committed r75045: <http://trac.webkit.org/changeset/75045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75045 might have broken Leopard Intel Debug (Tests)