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>
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
: Jet Villegas (:jet)
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 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

Actual result:
Alert saying FAIL.

Expected results:
Expected the document to load self as JS and JS parsing to fail.
Comment 1 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="">
Comment 2 Robert Longson 2011-10-09 04:40:50 PDT
Created attachment 565793 [details] [diff] [review]
Comment 3 Robert Longson 2011-10-10 01:44:21 PDT
Created attachment 565879 [details] [diff] [review]
replace string override too
Comment 4 Daniel Holbert [:dholbert] 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 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 Daniel Holbert [:dholbert] 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 Robert Longson 2011-10-14 06:51:42 PDT

I'll create a new bug for the AfterSetAttr change.
Comment 8 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
Comment 9 Ed Morley [:emorley] 2011-10-14 08:11:44 PDT
Backed out for comment 8 orange:
Comment 10 Robert Longson 2011-10-14 11:29:10 PDT
The patch exposed a bug in the mochitest. 

pushed with the mochitest fixed.
Comment 11 Daniel Holbert [:dholbert] 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 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 Daniel Holbert [:dholbert] 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 Robert Longson 2011-10-14 12:53:17 PDT
it does now. As per comment 0 in fact.
Comment 15 Daniel Holbert [:dholbert] 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 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 Daniel Holbert [:dholbert] 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 Ed Morley [:emorley] 2011-10-15 05:22:41 PDT

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