Bug 48966 – Script runs more than once after a clone

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 72894 [details]
demo

WebKit runs script more than once when the script element is moved by editing commands.

[Ryosuke Niwa]

Created attachment 72895 [details]
work in progress

I've attempted to override cloneElementWithoutChildren for script element to copy m_data.m_evaluated but it seems like we're not even setting the flag.  With the following stack trace, what I see is that none of m_data's flags are set true:

#0	0x1019745ff in WebCore::HTMLScriptElement::cloneElementWithoutChildren at HTMLScriptElement.cpp:200
#1	0x1017f4930 in WebCore::Element::cloneElementWithChildren at Element.cpp:155
#2	0x1017f49a8 in WebCore::Element::cloneNode at Element.cpp:150
#3	0x101591ebd in WebCore::CompositeEditCommand::cloneParagraphUnderNewElement at CompositeEditCommand.cpp:784
#4	0x101592675 in WebCore::CompositeEditCommand::moveParagraphWithClones at CompositeEditCommand.cpp:847
#5	0x1019d1060 in WebCore::IndentOutdentCommand::indentIntoBlockquote at IndentOutdentCommand.cpp:114
#6	0x1019d138e in WebCore::IndentOutdentCommand::formatRange at IndentOutdentCommand.cpp:230
#7	0x1014e9d3b in WebCore::ApplyBlockElementCommand::formatSelection at ApplyBlockElementCommand.cpp:127
#8	0x1019d0e4f in WebCore::IndentOutdentCommand::formatSelection at IndentOutdentCommand.cpp:220
#9	0x1014ea310 in WebCore::ApplyBlockElementCommand::doApply at ApplyBlockElementCommand.cpp:85
#10	0x1017d56fb in WebCore::EditCommand::apply at EditCommand.cpp:91
#11	0x1017d5776 in WebCore::applyCommand at EditCommand.cpp:212
#12	0x1017e87b5 in WebCore::executeIndent at EditorCommand.cpp:473
#13	0x1017e7512 in WebCore::Editor::Command::execute at EditorCommand.cpp:1610
#14	0x1016d3501 in WebCore::Document::execCommand at Document.cpp:3924
#15	0x101aedebf in WebCore::jsDocumentPrototypeFunctionExecCommand at JSDocument.cpp:2163
#16	0x2d6890c001b8 in ??
#17	0x1007ea939 in JSC::JITCode::execute at JITCode.h:77
#18	0x1007e69c4 in JSC::Interpreter::execute at Interpreter.cpp:759
#19	0x1007b6354 in JSC::evaluate at Completion.cpp:62
#20	0x101bd25d4 in WebCore::JSMainThreadExecState::evaluate at JSMainThreadExecState.h:54
#21	0x101f992e6 in WebCore::ScriptController::evaluateInWorld at ScriptController.cpp:148
#22	0x101f994a0 in WebCore::ScriptController::evaluate at ScriptController.cpp:171
#23	0x101f9ee1e in WebCore::ScriptController::executeScript at ScriptControllerBase.cpp:60
#24	0x101975c37 in WebCore::HTMLScriptRunner::executeScript at HTMLScriptRunner.cpp:156
#25	0x101975ec8 in WebCore::HTMLScriptRunner::runScript at HTMLScriptRunner.cpp:321
#26	0x1019766c9 in WebCore::HTMLScriptRunner::execute at HTMLScriptRunner.cpp:181
#27	0x101921881 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder at HTMLDocumentParser.cpp:199
#28	0x101921dab in WebCore::HTMLDocumentParser::pumpTokenizer at HTMLDocumentParser.cpp:235
#29	0x101922074 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible at HTMLDocumentParser.cpp:169
#30	0x101922516 in WebCore::HTMLDocumentParser::append at HTMLDocumentParser.cpp:311
#31	0x1016b6336 in WebCore::DecodedDataDocumentParser::appendBytes at DecodedDataDocumentParser.cpp:54
#32	0x1017163a6 in WebCore::DocumentWriter::addData at DocumentWriter.cpp:200
#33	0x101716428 in WebCore::DocumentWriter::endIfNotLoadingMainResource at DocumentWriter.cpp:220
#34	0x101716471 in WebCore::DocumentWriter::end at DocumentWriter.cpp:206
#35	0x101706e84 in WebCore::DocumentLoader::finishedLoading at DocumentLoader.cpp:276
#36	0x101869ea5 in WebCore::FrameLoader::finishedLoading at FrameLoader.cpp:2165
#37	0x101d6e66c in WebCore::MainResourceLoader::didFinishLoading at MainResourceLoader.cpp:456
#38	0x101f7a448 in WebCore::ResourceLoader::didFinishLoading at ResourceLoader.cpp:421

[Ryosuke Niwa]

The way I looked at the code briefly, it seems like we need to set some flags true in runScriptsForPausedTreeBuilder or HTMLScriptRunner::execute but I don't know anything about these two functions so it'll be great if someone could tell me which one I should be fixing.

[Ryosuke Niwa]

Just to clarify, Firefox doesn't run the script twice and passes my test.

[Alexey Proskuryakov]

Script elements in conteneditable seem just wrong, and manipulating them with execCommand even more so. San we perhaps just bail out in such cases? That seems much safer than playing whack-a-mole with scripts, frames, plug-ins etc.

[Eric Seidel (no email)]

Comment on attachment 72895 [details]
work in progress

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

> WebCore/dom/Element.h:378
>      virtual PassRefPtr<Node> cloneNode(bool deep);

What's proper behavior for javascript scriptElement.cloneNode()?  If you insert the clone does it get evaluated again?  What if it's mid-evaluation when it's cloned?

> WebCore/dom/ScriptElement.h:71
> +    ScriptElementData(ScriptElement*, Element*, bool evaluated);

Seems this should simply default to false.  Also an Enum may be better than a bool here.

> WebCore/html/HTMLScriptElement.cpp:198
> +PassRefPtr<Element> HTMLScriptElement::cloneElementWithoutChildren()

I don't understand the new funtion or it's name.  If the purpose is to clone respecting the evaluated bit, it seems like an odd name.

[Ryosuke Niwa]

Created attachment 73546 [details]
simpler demo

[Ryosuke Niwa]

Created attachment 73558 [details]
work in progress 2

[Ryosuke Niwa]

Created attachment 73572 [details]
lacks SVG implementation and test

[Ryosuke Niwa]

Created attachment 73631 [details]
Patch

[Darin Adler]

Comment on attachment 73631 [details]
Patch

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

Looks like you are on the right track, but I’d like you to consider some changes.

> WebCore/dom/Element.cpp:160
> +// This function needs to be synced with HTMLScriptElement and SVGScriptElement

It’s not obvious to me what “synced with” means here. Do we really need a copy of the function? It’s really unfortunate to have a copied and pasted copy of this function body. Can’t we call the base class version of  cloneElementWithoutChildren and then set the evaluated flag after the fact?

Or, since we have to add virtual function overhead, how about making a new small function that covers just the part of the operation that has to be different, rather than making cloneElementWithoutChildren itself virtual? That would resolve the issue above too.

> WebCore/dom/ScriptElement.cpp:369
> +    ASSERT(element);
> +    if (element->isHTMLElement() && element->hasTagName(HTMLNames::scriptTag))
> +        static_cast<HTMLScriptElement*>(element)->m_data.executeScript(sourceCode);
> +#if ENABLE(SVG)
> +    else if (element->isSVGElement() && element->hasTagName(SVGNames::scriptTag))
> +        static_cast<SVGScriptElement*>(element)->m_data.executeScript(sourceCode);
> +#endif

The isHTMLElement and isSVGElement checks here are not needed for correctness. Are you doing them for performance reasons?

Can we do this in a way that uses a virtual function in ScriptElement or perhaps even in Element for this rather than hasTagName checks for the two specific derived classes? It’s not good encapsulation for ScriptElement to have to know about all classes derived from it, so if we can avoid it that would be better.

> WebCore/dom/ScriptElement.h:81
> +    bool isEvaluated() const { return m_evaluated; }

Might be worthwhile to rename the data member too to add the “is”

> WebCore/dom/ScriptElement.h:91
> +    void executeScript(const ScriptSourceCode& sourceCode);

No need for the argument name here.

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #10)
> > WebCore/dom/Element.cpp:160
> > +// This function needs to be synced with HTMLScriptElement and SVGScriptElement
> 
> It’s not obvious to me what “synced with” means here. Do we really need a copy of the function? It’s really unfortunate to have a copied and pasted copy of this function body. Can’t we call the base class version of  cloneElementWithoutChildren and then set the evaluated flag after the fact?

Right, it's very unfortunate.  I considered setting the flag afterwards but then I have to add a public function that sets evaluated flag to ScriptElementData but that'll expose internals of ScriptElementData to its users.

> Or, since we have to add virtual function overhead, how about making a new small function that covers just the part of the operation that has to be different, rather than making cloneElementWithoutChildren itself virtual? That would resolve the issue above too.

I also considered adding a helper function that just copies attributes but that doesn't really reduce the amount of duplicated code either (at most one statement).

An alternatie approach I have in my mind right now is to add a virtual cloneElementWithoutAttributesAndChildren and call it inElement::cloneElementWithoutChildren.  This adds a virtual call but we can keep Element::cloneElementWithoutChildren non-virtual.  I'll try this & see what happens.

> > WebCore/dom/ScriptElement.cpp:369
> > +    ASSERT(element);
> > +    if (element->isHTMLElement() && element->hasTagName(HTMLNames::scriptTag))
> > +        static_cast<HTMLScriptElement*>(element)->m_data.executeScript(sourceCode);
> > +#if ENABLE(SVG)
> > +    else if (element->isSVGElement() && element->hasTagName(SVGNames::scriptTag))
> > +        static_cast<SVGScriptElement*>(element)->m_data.executeScript(sourceCode);
> > +#endif
> 
> The isHTMLElement and isSVGElement checks here are not needed for correctness. Are you doing them for performance reasons?

No, but I need to somehow access m_data of HTMLScriptElement and SVGScriptElement.

> Can we do this in a way that uses a virtual function in ScriptElement or perhaps even in Element for this rather than hasTagName checks for the two specific derived classes? It’s not good encapsulation for ScriptElement to have to know about all classes derived from it, so if we can avoid it that would be better.

I can move all of this to Element but we already have toScriptElement in ScriptElement.cpp, which has very similar checks so I'm not sure moving it to Element is a good idea. The problem with moving it to HTMLScriptElement or SVGScriptElement is that we still do need to cast them in HTMLScriptRunner and XMLParser in order to call executeScript. The only way to avoid this kind of check is if we added a virtual function to Element, and overrode it in HTMLScriptElement and SVGScriptElement.

The problem comes down to the fact ScriptElement does not inherit from Element and ScriptElementData's constructor requires a pointer to Element*, which means I can't move m_data to ScriptElement.  This is very similar situation to what we had with setSelectionRange in RenderTextControl, and it makes sense to put in ScriptElement.cpp if we were to follow the same convention.

> > WebCore/dom/ScriptElement.h:81
> > +    bool isEvaluated() const { return m_evaluated; }
> 
> Might be worthwhile to rename the data member too to add the “is”

Will do.

> > WebCore/dom/ScriptElement.h:91
> > +    void executeScript(const ScriptSourceCode& sourceCode);
> 
> No need for the argument name here.

Oops.  Will fix.

[Darin Adler]

(In reply to comment #11)
> An alternatie approach I have in my mind right now is to add a virtual cloneElementWithoutAttributesAndChildren and call it inElement::cloneElementWithoutChildren.  This adds a virtual call but we can keep Element::cloneElementWithoutChildren non-virtual. I'll try this & see what happens.

Seems like a good idea. Lets keep that virtual function as small as possible. Perhaps even less than cloneElementWithoutAttributesAndChildren.

> > The isHTMLElement and isSVGElement checks here are not needed for correctness. Are you doing them for performance reasons?
> 
> No, but I need to somehow access m_data of HTMLScriptElement and SVGScriptElement.

Then you should remove the isHTMLElement and isSVGElement parts here. The hasTagName checks are enough alone.

> > Can we do this in a way that uses a virtual function in ScriptElement or perhaps even in Element for this rather than hasTagName checks for the two specific derived classes? It’s not good encapsulation for ScriptElement to have to know about all classes derived from it, so if we can avoid it that would be better.
> 
> I can move all of this to Element but we already have toScriptElement in ScriptElement.cpp, which has very similar checks so I'm not sure moving it to Element is a good idea. The problem with moving it to HTMLScriptElement or SVGScriptElement is that we still do need to cast them in HTMLScriptRunner and XMLParser in order to call executeScript. The only way to avoid this kind of check is if we added a virtual function to Element, and overrode it in HTMLScriptElement and SVGScriptElement.

I’d like to see this list of classes concentrated as much as possible in one place. I’d like to see an isScriptElement function, whether Element member function or free function, then toScriptElement, then the rest be done with ScriptElement virtual functions.

I’d like to avoid writing functions one at a time that have separate branches for the two types of script element.

[Ryosuke Niwa]

(In reply to comment #12)
> I’d like to see this list of classes concentrated as much as possible in one place. I’d like to see an isScriptElement function, whether Element member function or free function, then toScriptElement, then the rest be done with ScriptElement virtual functions.
> 
> I’d like to avoid writing functions one at a time that have separate branches for the two types of script element.

The problem is that ScriptElement does have access to ScriptElementData, and HTMLScriptElement/SVGScriptElement separately has m_data.  So we'll end up doing that branch & cast in ScriptElement later anyways regardless of adding a public accessor to HTMLScriptElement/SVGScriptElement or making ScriptElement a friend of them.  And I think adding executeScript to Element is rather odd.

[Ryosuke Niwa]

Created attachment 73641 [details]
Fixed per Darin's comments

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> > An alternatie approach I have in my mind right now is to add a virtual cloneElementWithoutAttributesAndChildren and call it inElement::cloneElementWithoutChildren.  This adds a virtual call but we can keep Element::cloneElementWithoutChildren non-virtual. I'll try this & see what happens.
> 
> Seems like a good idea. Lets keep that virtual function as small as possible. Perhaps even less than cloneElementWithoutAttributesAndChildren.

This was indeed a good idea.  Each function is just one line now!

[Darin Adler]

(In reply to comment #13)
> The problem is that ScriptElement does have access to ScriptElementData, and HTMLScriptElement/SVGScriptElement separately has m_data.  So we'll end up doing that branch & cast in ScriptElement later anyways regardless of adding a public accessor to HTMLScriptElement/SVGScriptElement or making ScriptElement a friend of them.  And I think adding executeScript to Element is rather odd.

But ScriptElement can have a virtual function that does this.

[Ryosuke Niwa]

Created attachment 73659 [details]
Fixed more

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #13)
> > The problem is that ScriptElement does have access to ScriptElementData, and HTMLScriptElement/SVGScriptElement separately has m_data.  So we'll end up doing that branch & cast in ScriptElement later anyways regardless of adding a public accessor to HTMLScriptElement/SVGScriptElement or making ScriptElement a friend of them.  And I think adding executeScript to Element is rather odd.
> 
> But ScriptElement can have a virtual function that does this.

Ah, I finally understand what you meant.  Done.

[Darin Adler]

Comment on attachment 73659 [details]
Fixed more

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

> WebCore/dom/Element.cpp:177
> +    // This will catch HTML elements in the wrong namespace that are not correctly copied.
> +    // This is a sanity check as HTML overloads some of the DOM methods.
> +    ASSERT(isHTMLElement() == clone->isHTMLElement());

I think it would be better to leave the assertion behind in the non-virtual function rather than moving it into the virtual function.

> WebCore/dom/Element.h:202
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;

I think this function should be private. Nobody needs to call it except cloneElementWithoutChildren.

> WebCore/html/HTMLScriptElement.h:75
> +    void executeScript(const ScriptSourceCode& sourceCode);

No need to give the argument a name here.

> WebCore/html/parser/HTMLScriptRunner.cpp:138
> +    ScriptElement* scriptElement = toScriptElement(element.get());
> +    if (scriptElement) {

Could put this declaration inside the if.

[Ryosuke Niwa]

(In reply to comment #19)
> > WebCore/dom/Element.cpp:177
> > +    // This will catch HTML elements in the wrong namespace that are not correctly copied.
> > +    // This is a sanity check as HTML overloads some of the DOM methods.
> > +    ASSERT(isHTMLElement() == clone->isHTMLElement());
> 
> I think it would be better to leave the assertion behind in the non-virtual function rather than moving it into the virtual function.

Will do.

> > WebCore/dom/Element.h:202
> > +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren() const;
> 
> I think this function should be private. Nobody needs to call it except cloneElementWithoutChildren.

Will do.

> > WebCore/html/HTMLScriptElement.h:75
> > +    void executeScript(const ScriptSourceCode& sourceCode);
> 
> No need to give the argument a name here.

Will fix.

> > WebCore/html/parser/HTMLScriptRunner.cpp:138
> > +    ScriptElement* scriptElement = toScriptElement(element.get());
> > +    if (scriptElement) {
> 
> Could put this declaration inside the if.

I prefer to put it in a separate line.  Do you have a strong preference?

[Darin Adler]

(In reply to comment #20)
> > > WebCore/html/parser/HTMLScriptRunner.cpp:138
> > > +    ScriptElement* scriptElement = toScriptElement(element.get());
> > > +    if (scriptElement) {
> > 
> > Could put this declaration inside the if.
> 
> I prefer to put it in a separate line.  Do you have a strong preference?

When a definition can go inside an if I think it’s good to scope it only to the block of the if. It’s a moderately strong preference. Often, our use of early return precludes this idiom.

I think you should “develop a taste” for this, because it’s great to scope the variable and have it be a compile error to use it outside the if.

[Ryosuke Niwa]

(In reply to comment #21)
> When a definition can go inside an if I think it’s good to scope it only to the block of the if. It’s a moderately strong preference. Often, our use of early return precludes this idiom.

Ok.  Will do.  Maybe we should consider adding this to the style guideline?

[Darin Adler]

(In reply to comment #22)
> Maybe we should consider adding this to the style guideline?

Sure, I think that’s worth considering.

[Ryosuke Niwa]

(In reply to comment #23)
> Sure, I think that’s worth considering.

Would you mind posting it on webkit-dev?  I think scoping tightly as possible makes a lot of sense but I don't know enough about the existing code base to define a syntactical rule myself.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/71895.  But the SVG tests fail on qt platforms because qt has a separate XML parser.  We need to fix qt's parser as well.  I'll disable the tests on qt platforms for now.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71895 might have broken Qt Linux Release
The following tests are not passing:
svg/dom/SVGScriptElement/script-clone-rerun-self.svg
svg/dom/SVGScriptElement/script-clone-rerun.svg

[Ryosuke Niwa]

The follow up bug for the qt platform is filed as https://bugs.webkit.org/show_bug.cgi?id=49429.

[David Kilzer (:ddkilzer)]

<rdar://problem/8626165>