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
Alert saying FAIL.
Expected the document to load self as JS and JS parsing to fail.
This might be desired behavior. See bug 531327. Though of course we shouldn't execute any inline scripts, for example for:
Created attachment 565793 [details] [diff] [review]
Created attachment 565879 [details] [diff] [review]
replace string override too
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?
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.
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).
I'll create a new bug for the AfterSetAttr change.
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
Backed out for comment 8 orange:
The patch exposed a bug in the mochitest.
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/15d3df58e1e4 with the mochitest fixed.
(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)
(In reply to Robert Longson from comment #12)
Ah, ok -- and that's because href="" resolves to the URI of the host document?
it does now. As per comment 0 in fact.
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.)
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.
(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.