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>
Doh! Seems like it should be a pretty easy fix.
There are 7 other notImplemented()s in HTMLTreeBuilder, we should probably go through and assess whether any need to be taken care of.
Created attachment 96799 [details] Patch
you might have to rebaseline the test that i added - svg/dom/use-style-recalc-script-execute-crash.html
Created attachment 96804 [details] Patch
(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.
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()...
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.)
Created attachment 96807 [details] Patch
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...
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?
(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.
Created attachment 96814 [details] Patch
I guess the asserts are already covered if you follow the existing code paths, so this patch just fixes the order.
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
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
(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.
Tom wrote the test.
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. :)
Created attachment 96829 [details] Patch
Comment on attachment 96829 [details] Patch LGTM.
Comment on attachment 96829 [details] Patch Clearing flags on attachment: 96829 Committed r88584: <http://trac.webkit.org/changeset/88584>
All reviewed patches have been landed. Closing bug.
Committed r88586: <http://trac.webkit.org/changeset/88586>
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
I thought this patch was going to let us remove the new branch in ScriptElement::prepareScript added here: http://trac.webkit.org/changeset/88549
(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.
<rdar://problem/9681133>
(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? :)
(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. :)