Bug 62412 – <script> inside <svg> should be executed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[James Simonsen]

According to the HTML5 spec, SVG <script> elements inside foreign content mode should be executed. Right now, we just have a notImplemented() in HTMLTreeBuilder there. We should fix that.

Here's an example that doesn't work:

<svg>
    <g>
        <rect id="test">
            <script>
                alert(1);
                document.body.innerHTML = "PASS";
            </script>
        </rect>
    </g>
</svg>

[Eric Seidel (no email)]

Doh!  Seems like it should be a pretty easy fix.

[Tony Gentilcore]

There are 7 other notImplemented()s in HTMLTreeBuilder, we should probably go through and assess whether any need to be taken care of.

[James Simonsen]

Created attachment 96799 [details]
Patch

[Abhishek Arya]

you might have to rebaseline the test that i added - svg/dom/use-style-recalc-script-execute-crash.html

[James Simonsen]

Created attachment 96804 [details]
Patch

[James Simonsen]

(In reply to comment #4)
> you might have to rebaseline the test that i added - svg/dom/use-style-recalc-script-execute-crash.html

Good point. I was running out of a chrome tree and we haven't rolled to the revision that has that. I'll sync with head, rebaseline, and upload a third patch.

[Adam Barth]

Comment on attachment 96804 [details]
Patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2319
> +            m_tree.openElements()->pop();
> +            m_isPaused = true;
> +            m_scriptToProcess = m_tree.currentElement();

I would have expected the m_tree.openElements()->pop() to come after m_scriptToProcess = m_tree.currentElement()...

[Eric Seidel (no email)]

The relevant spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#scriptForeignEndTag

An end tag whose tag name is "script", if the current node is a script element in the SVG namespace
Pop the current node off the stack of open elements.

Let the old insertion point have the same value as the current insertion point. Let the insertion point be just before the next input character.

Increment the parser's script nesting level by one. Set the parser pause flag to true.

Process the script element according to the SVG rules, if the user agent supports SVG. [SVG]

Even if this causes new characters to be inserted into the tokenizer, the parser will not be executed reentrantly, since the parser pause flag is true.

Decrement the parser's script nesting level by one. If the parser's script nesting level is zero, then set the parser pause flag to false.

Let the insertion point have the value of the old insertion point. (In other words, restore the insertion point to its previous value. This value might be the "undefined" value.)

[James Simonsen]

Created attachment 96807 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96804 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2319
>> +            m_scriptToProcess = m_tree.currentElement();
> 
> I would have expected the m_tree.openElements()->pop() to come after m_scriptToProcess = m_tree.currentElement()...

I'm surprised this works, actually...

[Eric Seidel (no email)]

Comment on attachment 96807 [details]
Patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2319
> +            m_scriptToProcess = m_tree.currentElement();

Are we sure this ends up as the script tag?

[James Simonsen]

(In reply to comment #7)
> (From update of attachment 96804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96804&action=review
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2319
> > +            m_tree.openElements()->pop();
> > +            m_isPaused = true;
> > +            m_scriptToProcess = m_tree.currentElement();
> 
> I would have expected the m_tree.openElements()->pop() to come after m_scriptToProcess = m_tree.currentElement()...

Hrm. Yeah. I wonder how it worked while baselining these tests. Anyway, once my build is done, I'll upload a new one (and include an ASSERT).

(In reply to comment #8)
> The relevant spec:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#scriptForeignEndTag
> 
> An end tag whose tag name is "script", if the current node is a script element in the SVG namespace
> Pop the current node off the stack of open elements.
> 
> Let the old insertion point have the same value as the current insertion point. Let the insertion point be just before the next input character.
> 
> Increment the parser's script nesting level by one. Set the parser pause flag to true.
> 
> Process the script element according to the SVG rules, if the user agent supports SVG. [SVG]
> 
> Even if this causes new characters to be inserted into the tokenizer, the parser will not be executed reentrantly, since the parser pause flag is true.
> 
> Decrement the parser's script nesting level by one. If the parser's script nesting level is zero, then set the parser pause flag to false.
> 
> Let the insertion point have the value of the old insertion point. (In other words, restore the insertion point to its previous value. This value might be the "undefined" value.)

FYI, most of this is done by HTMLDocumentParser and HTMLScriptRunner.

[James Simonsen]

Created attachment 96814 [details]
Patch

[James Simonsen]

I guess the asserts are already covered if you follow the existing code paths, so this patch just fixes the order.

[WebKit Review Bot]

Comment on attachment 96814 [details]
Patch

Attachment 96814 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8826538

New failing tests:
svg/dom/range-delete.html
html5lib/runner.html

[WebKit Review Bot]

Created attachment 96820 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Simonsen]

(In reply to comment #15)
> (From update of attachment 96814 [details])
> Attachment 96814 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8826538
> 
> New failing tests:
> svg/dom/range-delete.html

This test wasn't doing what was expected. It was the cloned node that executed and deleted everything. Now the original executes and destroys everything before it can be cloned.

Do you think it's still relevant? If so, I can rewrite it without SVG <script> and (hopefully) have it exercise the same code path.

> html5lib/runner.html

This just needs to be rebaselined. We pass another test. Yay.

[Eric Seidel (no email)]

Tom wrote the test.

[Eric Seidel (no email)]

I suspect we can just delete the test from the security bug, as invalid.  And update the html-parser test results.  Please upload a new patch. :)

[James Simonsen]

Created attachment 96829 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96829 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 96829 [details]
Patch

Clearing flags on attachment: 96829

Committed r88584: <http://trac.webkit.org/changeset/88584>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Simonsen]

Committed r88586: <http://trac.webkit.org/changeset/88586>

[Ryosuke Niwa]

This patch may have caused crashes on Snow Leopard Debug builds:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html

[Tony Gentilcore]

I thought this patch was going to let us remove the new branch in ScriptElement::prepareScript added here:
http://trac.webkit.org/changeset/88549

[James Simonsen]

(In reply to comment #25)
> This patch may have caused crashes on Snow Leopard Debug builds:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html

It's not crashing, it just failed that one test. I already submitted the new baseline on Friday. It's passing now.

(In reply to comment #26)
> I thought this patch was going to let us remove the new branch in ScriptElement::prepareScript added here:
> http://trac.webkit.org/changeset/88549

Yeah, I'll upload a separate patch for that. I wanted this one to bake for a bit.

[David Kilzer (:ddkilzer)]

<rdar://problem/9681133>

[David Kilzer (:ddkilzer)]

(In reply to comment #27)
> (In reply to comment #25)
> > This patch may have caused crashes on Snow Leopard Debug builds:
> > http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html
> 
> It's not crashing, it just failed that one test. I already submitted the new baseline on Friday. It's passing now.
> 
> (In reply to comment #26)
> > I thought this patch was going to let us remove the new branch in ScriptElement::prepareScript added here:
> > http://trac.webkit.org/changeset/88549
> 
> Yeah, I'll upload a separate patch for that. I wanted this one to bake for a bit.

Is it done baking yet?  :)

[James Simonsen]

(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > This patch may have caused crashes on Snow Leopard Debug builds:
> > > http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r88585%20(583)/results.html
> > 
> > It's not crashing, it just failed that one test. I already submitted the new baseline on Friday. It's passing now.
> > 
> > (In reply to comment #26)
> > > I thought this patch was going to let us remove the new branch in ScriptElement::prepareScript added here:
> > > http://trac.webkit.org/changeset/88549
> > 
> > Yeah, I'll upload a separate patch for that. I wanted this one to bake for a bit.
> 
> Is it done baking yet?  :)

Yep. Thanks for the reminder. :)