Last Comment Bug 528444 - Empty xlink:href treated like absent xlink:href on SVG <script>
: Empty xlink:href treated like absent xlink:href on SVG <script>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
http://hsivonen.iki.fi/test/moz/svg-s...
Depends on:
Blocks: 694670
  Show dependency treegraph
 
Reported: 2009-11-13 01:38 PST by Henri Sivonen (:hsivonen)
Modified: 2011-10-15 05:22 PDT (History)
4 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.41 KB, patch)
2011-10-09 04:40 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
replace string override too (5.35 KB, patch)
2011-10-10 01:44 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description User image Henri Sivonen (:hsivonen) 2009-11-13 01:38:45 PST
Build id: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091105 Minefield/3.7a1pre

Steps to reproduce:
 1) Load http://hsivonen.iki.fi/test/moz/svg-script-empty-xlink-href.svg

Actual result:
Alert saying FAIL.

Expected results:
Expected the document to load self as JS and JS parsing to fail.
Comment 1 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-28 14:32:09 PDT
This might be desired behavior. See bug 531327. Though of course we shouldn't execute any inline scripts, for example for:

<svg:script xlink:href="">
thisShouldNotExecute();
</svg:script>
Comment 2 User image Robert Longson 2011-10-09 04:40:50 PDT
Created attachment 565793 [details] [diff] [review]
patch
Comment 3 User image Robert Longson 2011-10-10 01:44:21 PDT
Created attachment 565879 [details] [diff] [review]
replace string override too
Comment 4 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-10 15:36:52 PDT
The chunks from the original patch look good.

I don't understand the addition in the new patch (DidChangeString --> AfterSetAttr).  Could you explain what that's for?
Comment 5 User image Robert Longson 2011-10-11 01:58:03 PDT
I'm trying to get rid of all DidChangeXXX overrides. When I do so I can make all the 20 or so methods non-virtual and save 128 bytes per class (not per instance unfortunately). We'll get performance improvements too, over 1% on tSVG which isn't much but it all helps.
Comment 6 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-11 10:22:25 PDT
Ok -- I think I'd prefer if that change were in a separate patch, then (or ideally a separate bug, but I don't care too much on that part), since it's a nontrivial change that's unrelated to the actual fix here.  If we have to back it out for some reason, or if we trace a regression back to it, we'd want to back out just that part without the meat of this bug's fix.

Marking r+ on the original attachment, with just the fix for this bug (and pending r=me on the AfterSetAttr chunk as a separate patch, either on this bug or in a new one).
Comment 7 User image Robert Longson 2011-10-14 06:51:42 PDT
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/c49fd40bbd2b

I'll create a new bug for the AfterSetAttr change.
Comment 8 User image Ed Morley [:emorley] 2011-10-14 08:05:45 PDT
Robert, I couldn't see you on IRC (or more likely you are using something other than longsonr now).

This has caused a new M1 orange:
{
96930 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_nonAnimStrings.xhtml | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: XML can't be the whole program at http://mochi.test:8888/tests/content/svg/content/test/test_nonAnimStrings.xhtml:79
}
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c49fd40bbd2b
https://tbpl.mozilla.org/php/getParsedLog.php?id=6853293&tree=Mozilla-Inbound
Comment 9 User image Ed Morley [:emorley] 2011-10-14 08:11:44 PDT
Backed out for comment 8 orange:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4166640d1b5c
Comment 10 User image Robert Longson 2011-10-14 11:29:10 PDT
The patch exposed a bug in the mochitest. 

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/15d3df58e1e4 with the mochitest fixed.
Comment 11 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-14 11:49:28 PDT
(In reply to Robert Longson from comment #10)
> The patch exposed a bug in the mochitest.

Are you sure? I'm not convinced that the mochitest was buggy...

The mochitest did have <script id="script" xlink:href="">, but it didn't depend on any scripts *inside* of that <script> element running (that'd be the sort of test bug I'd expect this patch to expose).

Rather, it just tested that it was able to <animate> the xlink:href attribute (which apparently works, but we now trigger a security error for some reason).

Might merit a followup bug to investigate...  (I'm happy to file, I just want to make sure I'm not missing something)
Comment 12 User image Robert Longson 2011-10-14 12:32:28 PDT
After this patch xlink:href="" means reload this file as javascript and run it, that's where we get the security error now.
Comment 13 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-14 12:51:34 PDT
(In reply to Robert Longson from comment #12)
> After this patch xlink:href="" means reload this file as javascript and run
> it

Ah, ok -- and that's because href="" resolves to the URI of the host document?
Comment 14 User image Robert Longson 2011-10-14 12:53:17 PDT
it does now. As per comment 0 in fact.
Comment 15 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-14 14:16:01 PDT
Ah, of course, this exactly matches "expected results" from comment 0. :)

(I was recalling comment 1's "we shouldn't execute any inline scripts" as being the entirety of the expected results, and had forgotten about the other part.)
Comment 16 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-14 14:41:44 PDT
Please note that elsewhere we have moved towards letting <someelement src=""> not load anything. This because in the vast majority of cases the above markup intends to not actually load anything and is instead often a result of serverside code like:

response.write("<someelement src=\"" + somevalue + "\">");

So in this case I think that we should *not* execute the contents of a <svg:script xlink:href="">. But we also shouldn't attempt to resolve a url and load it.

I'm not 100% sure that the HTML5 spec has been updated to require this behavior (though I think so), but that was definitely the agreement on the lists.
Comment 17 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-14 14:59:51 PDT
(In reply to Jonas Sicking (:sicking) from comment #16)
> But we also shouldn't attempt to resolve a url and load it.

I've filed bug 694670 on that.
Comment 18 User image Ed Morley [:emorley] 2011-10-15 05:22:41 PDT
https://hg.mozilla.org/mozilla-central/rev/15d3df58e1e4

Note You need to log in before you can comment on or make changes to this bug.