Closed Bug 528444 Opened 10 years ago Closed 8 years ago

Empty xlink:href treated like absent xlink:href on SVG <script>

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: hsivonen, Assigned: longsonr)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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.
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>
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #565793 - Flags: review?(dholbert)
Attached patch replace string override too (obsolete) — Splinter Review
Attachment #565793 - Attachment is obsolete: true
Attachment #565879 - Flags: review?(dholbert)
Attachment #565793 - Flags: review?(dholbert)
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).
Attachment #565879 - Attachment is obsolete: true
Attachment #565879 - Flags: review?(dholbert)
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/c49fd40bbd2b

I'll create a new bug for the AfterSetAttr change.
Flags: in-testsuite+
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
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)
After this patch xlink:href="" means reload this file as javascript and run it, that's where we get the security error now.
(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?
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.
https://hg.mozilla.org/mozilla-central/rev/15d3df58e1e4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.