308590 - SVG patterns, gradients and filters don't work when SVG is loaded from a data: URL

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050914
Firefox/1.6a1

Steps to reproduce:
1. Load URL in URL field.
  Result: Empty rectangle.
  Expected: Rectangle full of little green and red squares.
2. Save to desktop.
3. Load.
  Result: Now it works.

Maybe fill="url(#pat1)" is getting confused because the document's URL is a
data: URL?

Comment 1

[Martijn Wargers (dead)]

Maybe this is invalid? See bug 243917, especially bug 243917, comment 6.

Comment 2

[Jesse Ruderman]

If the document's URL is http://www.mozilla.org/foo.svg, the URL
fill="url(#pat1)" seems to refer to "the pattern described by the node with id
pat1 in this document", not "the pattern described by the node with id pat1 that
you can find by loading http://www.mozilla.org/foo.svg" (see bugs about
liveness, etc).  Why should it be different for data: URLs?

Comment 3

[Boris Zbarsky [:bzbarsky]]

You can do references to other files too, though.  So in general, we have to use URIs as the pattern identifier, and then Martijn is right -- data: URIs don't support fragments.

We have existings bugs on this, for what it's worth; the problem is that the relevant Necko APIs are frozen, so there's not much we can change about the situation.  :(

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, what we could do is say that all implementations of nsIURI will also implement nsIURL, although they may return failure for all methods except for SetRef and GetRef.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That would help some with this case, but not solve the related issues we have with xml:base, etc.  That is, given the following markup at http://foo/:

<something xml:base="http://bar/">
  <svg:whatever xlink:href="#baz" />
</something>

should the href point to http://bar#baz?  Or to the element with the id "baz" in the current document?  I believe the SVG spec specifies the latter, but if we go through URIs it's rather difficult to do that....  Especially since xlink:href="x#baz" should definitely point to http://bar/x#baz in this case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

But yes, perhaps we should just go ahead and start with everything implementing nsIURL.  We'll need to audit all the nsIURL consumers in the tree that assume no errors are thrown on the nsIURL getters, but other than that it should work.  I'm very sure we already have a bug where that was suggested.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #4)
> Well, what we could do is say that all implementations of nsIURI will also
> implement nsIURL

no, we can't say that. nsIProtocolHandler is frozen and doesn't have such a requirement for newURI.

Comment 9

[Jonathan Leech]

I have encountered this bug and I am a little dismayed that it's been around for almost 3 years. And the same functionality works fine in Safari/Webkit and Opera. 

Oh well. I have confirmed that Firefox will do patterns just fine in inline SVG, so here's a workaround for anyone using data:URIs as the src of an iframe or object.

- If its an object, swap it out for an iframe, either in the code that generates it, or in javascript.
- onload, find the iframe, .contentWindow.document.getElementsByTagName("svg")[0] is the svg document, calling .cloneNode(true) on it creates a new svg with the patterns not broken. 
- Replace the original iframe with the new svg.

I am still hashing out all the details, but I've done it in Firebug and it works. I am going to investigate if I can replace the svg in the iframe with the new svg instead of replacing the whole iframe. I would prefer to keep the iframe since I've got some other javascript code around that expects it to be there. 

Also, I would like to point out if this works, then Firefox could surely do the same thing internally, right after it extracts the data out of the data:URI.

Finally, I would like to lobby the powers that be to reopen the bugs like 294517, and modify the corresponding document.implementation.hasFeature("http://www.w3.org/TR/SVG11/feature#") stuff, because unless features like patterns, filters, external references, etc. etc. work in all contexts, they don't work, and are not finished.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jonathan, I suggest actually reading the bug before commenting.  See also <https://bugzilla.mozilla.org/etiquette.html>.

Comment 11

[Jonathan Leech]

Attachment: Demonstration of the workaround

This workaround applies to inline SVG embedded as a DATA:URI in the src of an iframe. The iframe should have the same width and height as the svg, percentages don't work.
It works by finding the svg in the document of the iframe, cloning it, and replacing the iframe with a div containing the cloned svg.
It isn't possible to use the existing iframe, or any other iframe to host the cloned svg, patterns still appear broken in any iframe containing the cloned svg.
The div is necessary, as if the svg is inserted directly into a table cell, the table cell can get the wrong width.
It is important to do all the heavy-lifting after the onload() of the iframe, as before its loaded you can't access its document.
I think multiple svgs with colliding pattern ids would be invalid using this workaround, as the ids must be unique within the entire XML document.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We've unfrozen things so we can actually fix this bug now.

Comment 16

[Alan Tam]

I don't agree with bug 591932 comment 4 saying that WebKit has confusion with how "#" inside a data URI is interpreted. In WebKit, if "#" exists in any data URI, then it is always part of the content to be deserialized, in both examples in bug 591932 comment 0 and bug 591932 comment 4. That's why the "#" as in "#3gradFill" in bug 591932 comment 0 doesn't need to be url encoded as "#23".

The only problem is that there is no real way to serialize the anchor references if the protocol is "data". Given a URI "data:some/xml+type,data-here-with-or-without-the-pound-character", I can't find any way in webkit to construct another _absolute_ URI that refers to #id inside this document. However, relative URI "url(#id")" works for referring to it from within the same document.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> However, relative URI "url(#id")" works for referring to it from within the
> same document

The point is, that's a violation of the URI and CSS specs.  url(#id) must act the same as if the string "#id" were appended to the current base URI (assuming the current base URI doesn't alreaydy have a reference).  So yes, Webkit is in fact inconsistent here, either internally or with the URI and CSS specs.

Comment 18

[Alan Tam]

OK. I kind of agree after reading RFC2396. We should not follow Webkit to accept the document as stated in bug 591932 comment 0; it is a malformed SVG document instead. But it should work if "#" has been encoded as "%23", and the URL as attached to this bug should work too.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> But it should work if "#" has been encoded as "%23"

How?  The only way to make it work consistently is to not treat anything after '#' as part of the data of the data: URI, no?

Comment 20

[Alan Tam]

I did agree with what you said, in other words, "#" should not be part of the data of the data: URI. Then what is the problem that prevents the modified version (replacing "#" by "%23") of bug 591932 comment 0 from working?

As there is no more unescaped "#" in the data URI, everything from "<svg" till "</svg>" belongs to the same uric during parsing. Then "url(#3gradFill)" refers to "data:image/svg+xml,<svg...</svg>#3gradFill", which is a reference to the current svg document.

While I think that &lt;, &gt; and &quot; should be replaced by %3C, %3E and %22 respectively for due correctness, but Gecko should be permissive enough not to complain, right?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Then what is the problem that prevents the modified version (replacing "#" by
> "%23") of bug 591932 comment 0 from working?

Right now Gecko treats '#' as part of the data of a data: URI.

Comment 22

[Alan Tam]

If RFC2397 says nothing special about "#" in data: URI, then why should Gecko treat a data URI different from any other URI? Indeed, does RFC2397 even have any authority to say whether unescaped "#" is a fragment separator or not? I don't see RFC2396 or RFC3986 giving any flexibility for individual protocols to opt out.

If Gecko has been wrong, can we fix it?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> If RFC2397 says nothing special about "#" in data: URI,

The BNF for data: URIs makes no special provisions for stuff that's part of the URI but not part of the data, right?

> does RFC2397 even have any authority to say whether unescaped "#" is a
> fragment separator or not?

That's the big open issue, yes.

> If Gecko has been wrong, can we fix it?

That's why this bug is open, no?

Comment 24

[Alan Tam]

(In reply to comment #23)
> > If RFC2397 says nothing special about "#" in data: URI,
> 
> The BNF for data: URIs makes no special provisions for stuff that's part of the
> URI but not part of the data, right?

This is the part I don't understand. RFC2397 says '[...] "urlchar" is imported from [RFC2396]' but there is no BNF grammar for "urlchar" in RFC2396. If it means "uric" in RFC2396, then isn't it obvious that it doesn't include "#"?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah, interesting.  You're right.  So if we assume urlchar is supposed to be uric, then '#' and following stuff isn't part of the data URI itself.  Good, good.  Then we just need to do something like comment 6, or move refs to nsIURI.

Comment 27

[Mardeg]

(In reply to comment #6)
>  I'm very sure we already have a bug where that was suggested.
Nobody found this yet? Is it at all related to bug 126432?

Comment 28

[Arjan]

This will get more publicity now Firefox is singled out in http://ie.microsoft.com/testdrive/Graphics/SVGGradientBackgroundMaker/Default.html

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> Then we just need to do something like comment 6, or move refs to nsIURI.

After discussing this with bz on IRC, I'm going to focus on the latter option there.  This will involve the following changes:
 (a) moving Get/SetRefs to nsIURI
 (b) add impls of Get/SetRefs to all non-nsIURL nsIURI classes
 (c) audit for all places where we use "is this nsIURI a nsIURL" as a proxy for "does this nsIURI support refs", and fix those places.

Comment 30

[Boris Zbarsky [:bzbarsky]]

One other thing we should consider, possibly as a followup:  store the ref in a separate member in nsSimpleURI.  That way when using data:foo as a base and a relative URI of '#bar' we can share the string storage for foo instead of copying, if we do it right.

Comment 31

[Daniel Holbert [:dholbert]]

> One other thing we should consider, possibly as a followup
> That way...we can share the string storage

Ah, that's a good idea!

I've got patches ready to post now, and I don't currently have that optimization in the nsSimpleURI-impl patch...  I think I'd prefer to do that optimization in a followup bug, but I'm open to doing it here if you'd prefer.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: patch 1: Improve test_URIs.js

This patch improves test_URIs.js, making it much more thorough.

Since this improvement is largely independent of the actual code changes here,  I'm including this patch first, with the .ref-specific parts disabled for "pure" URIs. (Those parts are flagged with XXXdholbert and will be enabled in a later patch.)

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: patch 2: Move .ref to nsIURI & add stub impls

This shifts .ref from nsIURL to nsIURI, and adds stubs implementations to all classes that implement nsIURI but not nsIURL.

In two out of three cases, I actually think that the stub implementations are "correct" for now:
(1) nsNullPrincipalURI -- all of the setters there are stubbed as NS_ERROR_NOT_IMPLEMENTED, and many of the getters (e.g. GetUsername, GetHostPort) are also stubs.  I'm not sure if we have reason to need "real" Get/SetRef impls on this class, but I assume we don't...?

(2) nsMozIconURI -- pretty much all of the getters and setters here are also stubs -- e.g. GetPath returns the empty string, and SetPath returns NS_ERROR_FAILURE.  So, I've just duplicated this 'path' behavior for 'ref'.

In the third case -- nsSimpleURI -- we do actually need a meaningful impl, and I'm leaving that for the next patch.

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: patch 3: Add impls for new nsSimpleURI GetRef/SetRef methods

This patch adds impls for GetRef/SetRef.

This is the part that would differ based on comment 30 / comment 31.  I'm reconsidering whether it'd be better to do that optimization up-front, and dispense with this patch's "interim" parsing code that'd be destined for removal anyway.  So, I'm not requesting review on this patch for the moment.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> I think I'd prefer to do that optimization in a followup bug,

Yep, sounds good.  I said so explicitly.  ;)

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: patch 4: tweak test_URIs.js to now check .ref on URIs

This removes patch 1's special-cases for "pure" URIs in test_URIs.js that I mentioned in comment 32.  (because at this point in the patch-stack, those checks will now pass)

It also removes QI's to nsIURL from that test (which were previously necessary to get access to .ref).

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: patch 5: Remove QI's to nsIURL

I searched MXR for calls to GetRef/SetRef, to find all the places where we might be QIing to nsIURL as a proxy for "can we call GetRef/SetRef".  This patch is the fix for all those places.

One part that varied on a case-by-case basis is whether to preserve a null check on the URI (to replace the null-check on the URL).  In some cases, it was clearly unnecessary (e.g. when the URI is the result of a call to "clone()" whose rv we've already checked for success, or when contextual code already null-checks the URI).  Where it was unclear, I looked at the surrounding code to see if we already expect non-null vs. if we check for null, and I matched that local behavior.

There was also one place where I wasn't sure whether the nsIURL check might still be desired -- in nsXBLPrototypeBinding::Init, which has a comment suggesting that we might _want_ special behavior for non-URLs:
> 308   // The binding URI might not be a nsIURL (e.g. for data: URIs). In that case,
> 309   // we always use the first binding, so we don't need to keep track of the ID.

To avoid breaking anything, I left that one as-is and added a XXXdholbert comment.  I still need to dig there and figure out whether we want to preserve that special-casing or not.  (note to self: looks like that chunk was added in bug 366770)

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: patch 5: (diff -w version)

...and since a number of the changes in patch 5 affect indentation, here's a diff -w version for convenience.

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: patch 6: Fix nsDataHandler::NewURI to accept "#myRef" as a spec

This patch fixes nsDataHandler::NewURI, which is invoked under the hood with aSpec == "#pat1" when we load this bug's testcase[1].

Currently, nsDataHandler::NewURI assumes that every URI that it gets passed is a fully specified data URI, but now it needs to handle relative URIs off of the given aBaseURI.  For data:, the only valid relative URIs are references, so this patch just adds a special-case to check for those.

After the patch stack is applied up to this point, we display this bug's testcase[1] correctly! Woot! :)

[1] by "this bug's testcase" I mean "the data URI in this bug's URL field"

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: patch 6: (diff -w version)

patch 6 also tweaks indentation, so here's a diff -w version for convenience.

Comment 41

[Daniel Holbert [:dholbert]]

(In reply to comment #39)
> After the patch stack is applied up to this point, we display this bug's
> testcase[1] correctly! Woot! :)

(I also checked the Microsoft demo from comment 28, and that works too)

Comment 42

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v2: Improve test_URIs.js

(realized that test_URIs.js still needed some data: URIs -- added 2 of them here. That's the only change vs. previous version of patch 1)

Comment 43

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v2: Add impls for new nsSimpleURI GetRef/SetRef methods

...and this tweaked version of patch 3 just adds a hack to nsSimpleURI::SetRef to gracefully handle trivial calls to SetRef(EmptyCString()) on immutable URIs.  Without this hack, we fail a mochitest* due to SetRef calls harmlessly failing (and then the caller misinterpreting that failure as a reason to bail out via NS_ENSURE_SUCCESS).

* see try push http://tbpl.mozilla.org/?tree=Try&rev=ddf53fb60deb -- the mochitest in question was test_bug519928.html.  (The immutable URI that was receiving the troublesome SetRef("") calls was "about:blank".)

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532898 [details] [diff] [review]
patch 1 v2: Improve test_URIs.js

r=me  Yay tests!

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532836 [details] [diff] [review]
patch 2: Move .ref to nsIURI & add stub impls

Two issues:

1)  The two idl files have a little ASCII art diagram of the parts of a nsIURI and nsIURL respectively.  Can you please copy the "#ref" bit from nsIURL to nsIURI? and add to the ascii art in nsIURI?

2)  With just the first patch and this applied, the test will fail.  Can you please remove the typeof == undefined checks the previous patch added in this patch?  Or just not add them in the first place or something?

r=me with those nits.

Comment 46

[Daniel Holbert [:dholbert]]

(In reply to comment #45)
> 1)  [...] add to the ascii art in nsIURI?

Yeah -- I initially had that change, but then I saw the caption for that art specifically mentions that it's based on an RFC (and the RFC makes no mention of "#ref"), so I took that change out.  I'll put it back, though, and mention that the ref is an addition to what's defined in the RFC.

> 2)  With just the first patch and this applied, the test will fail.  Can you
> please remove the typeof == undefined checks the previous patch added in
> this patch?  Or just not add them in the first place or something?

Yeah -- that's in patch 4 -- I'll just merge patch 4 into patches 3 and 2 so that the test passes at every stage.

(Note: I still need to write some reftests for this, along the lines of this bug's data-URI testcase, too.)

Also: I have a new version of nsSimpleURI that implements .ref as a separate string, per comment 30.  But I'm not posting it yet, since there are now more changes coming. (below)

Per discussion with bz in IRC, I'll now be solving the issue from comment 43 a bit differently.  Quoting (selectively) from IRC:
{
<bz>  dholbert: so I think we should throw on all SetRef calls for immutable URIs
<dholbert> bz, that's my leaning too, yeah
<bz> dholbert: but we have several places where this "set ref to null to compare uris except for refs" pattern is used
<dholbert> bz: nsIURI::EqualsExceptRef(nsIURL* someOtherURI)?
<bz> dholbert: it would be cleanest, in some ways
<bz> dholbert: except for possible code duplication with Equals()
<bz> dholbert: but internally we could implement Equals() in terms of EqualsExceptRef and a ref check
<bz> so maybe that's what we should do
<bz> sound ok?
<dholbert> bz, sounds good to me.
}

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 532844 [details] [diff] [review]
patch 5: Remove QI's to nsIURL

(removing review request on 'patch 5: Remove QI's to nsIURL', since a lot of it will change per the end of comment 46)

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532850 [details] [diff] [review]
patch 6: Fix nsDataHandler::NewURI to accept "#myRef" as a spec

> spec.Length() > 0

  !spec.IsEmpty()

> *result = uri.forget().get();

  uri.forget(result);

r=me with that.

Comment 49

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v3: Improve test_URIs.js [r=bz]

Here's the tests patch again, with r+ noted and with the "not add [temporary-undefined-checks] in the first place" part from Comment 45.

Comment 50

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: Move .ref to nsIURI & add stub impls [sr=biesi]

(In reply to comment #46)
> (In reply to comment #45)
> > 1)  [...] add to the ascii art in nsIURI?
> 
> Yeah -- I initially had that change, but then I saw the caption for that art
> specifically mentions that it's based on an RFC (and the RFC makes no
> mention of "#ref"), so I took that change out.  I'll put it back, though,
> and mention that the ref is an addition to what's defined in the RFC.

Sorry, I misremembered -- the issue was actually that the RFC says of #ref: "it is not part of a URI, but is often used in conjunction with a URI" -- whereas we bundle it into the various parts of the URI.

So, I've  updated the ASCII art and added a comment to point this discrepancy out.  Re-requesting review to make sure this is what you had in mind. (The nsIURI header comment is the only change vs. previous version.)

Comment 51

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v3: add nsIURI::equalsExceptRef

This patch adds the method nsIURI::EqualsExceptRef as described in comment 46.

It updates the uuids for nsIURI and all sub-interfaces.  I'm not sure if I also need to also update the CIDs in nsSimpleURI, nsStandardURL, etc, though...  At first glance, it looks like those CIDs are only used internally (for QI'ing, in order to answer the question e.g. "is this URI a nsStandardURL?") But maybe binary components might depend on them too...?

The patch also updates test_URIs.js to test equals() (and now equalsExceptRef()) more thoroughly.

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 533849 [details] [diff] [review]
patch 3 v3: add nsIURI::equalsExceptRef

I'm requesting sr?biesi for the nsIURI.idl change in the equalsExceptRef patch.

Comment 53

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v2: Add impl for .ref on nsSimpleURI

This patch fills in the .ref impl on nsSimpleURI.cpp. It's stored in a separate string, per comment 30.

(I originally used nsACString::IsVoid() to distinguish between 'empty ref' and 'no ref', but dbaron and bsmedberg discouraged that because apparently IsVoid() is semi-DOM-internal and is eventually wanting to be moved to a different level of abstraction. So, I now use a PRPackedBool member var instead.)

Comment 54

[Boris Zbarsky [:bzbarsky]]

Daniel, your comments on IRC about the other SetRef callers you ran into worry me.  Can you please catch me on irc?  Maybe we need to change the way about: URI immutability works instead...

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 533849 [details] [diff] [review]
patch 3 v3: add nsIURI::equalsExceptRef

(canceling some review requests, due to updated patches coming soon)

Comment 56

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v3: add nsIURI::equalsExceptRef

For consistency (both internal & w.r.t. my next patch), and for a reduction in QI's, this new version of the "equalsExceptRef" patch takes this strategy:

 - Shifts existing Equals() code into a protected virtual EqualsInternal() method, which takes an enum as an arg that tells us whether to care about the .ref or not.

 - Makes Equals() and EqualsExceptRef() one-liners that pass different enum values to EqualsInternal().

(In my previous patch, I took a different route of having Equals call EqualsExceptRef and then check mRef.  But that was a bit messier, in part because EqualsExceptRef returns a generic nsIURI* which we have to QI to a concrete type before we can examine its member data... and also mRef can be a bit cumbersome to get at, as is the case in nsJARURI)

As before, I'm requesting sr? on the interface change to nsIURI.idl.

Comment 57

[Daniel Holbert [:dholbert]]

Attachment: patch 3.5 v1: add nsIURI::cloneIgnoringRef

(I'm numbering this patch "3.5" to shoehorn it into my numbering scheme :))

This patch adds a method "nsIURI::cloneIgnoringRef", as an easy way to strip the ref off of a URI (This is for use cases like e.g. nsExternalResourceMap::RequestResource, where we use ref-stripped URIs as keys to a hash table, to bundle together resources from the same external document.)
(This is the issue I brought up on IRC that bz mentioned in comment 54.)

The guts here are basically the same as in the previous patch -- generally, Clone() and CloneIgnoringRef() are one-liner calls to CloneInternal() (which can have its behavior specialized by subclasses).

Requesting SR on this patch's interface change, too. (the addition of nsIURI::cloneIgnoringRef).  Note that I didn't rev the IIDs in this patch, because I already did in the previous patch.

Comment 58

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v3: Add impl for .ref on nsSimpleURI

...and here's the patch with the impl of GetRef/SetRef/etc. on nsSimpleURI.  (not too different from the last version of this one that I posted in comment 53 -- just rebased on top of the other new patches)

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533835 [details] [diff] [review]
patch 2 v2: Move .ref to nsIURI & add stub impls [sr=biesi]

As far as the comment goes, the new setup is a bit of a pain because, for example, it's not clear that the 'r' in "portnumber" is part of the port, and similar issues for the '/' in the path.  How about this:

  *      ftp://username:password@hostname:portnumber/pathname#ref
  *      \ /   \               / \      / \        /\         \ /
  *       -     ---------------   ------   --------  |         -
  *       |            |             |        |      |         |
  *       |            |             |        |      |        Ref
  *       |            |             |       Port    \        /
  *       |            |            Host      /       --------
  *       |         UserPass                 /	         |   
  *     Scheme                              /	        Path 
  *       \                                /
  *        --------------------------------
  *                       |
  *                    PrePath

Also, I'd add _ref.Truncate() to nsNullPrincipalURI::GetRef.

r=me with those nits.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533977 [details] [diff] [review]
patch 3 v3: add nsIURI::equalsExceptRef

r=me

Comment 61

[Boris Zbarsky [:bzbarsky]]

> because apparently IsVoid() is semi-DOM-internal

:(  OK, we'll probably need to add some API way to distinguish the "empty ref" (just "#") case from the "no ref" case, then.  File a followup on that?

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533980 [details] [diff] [review]
patch 3.5 v1: add nsIURI::cloneIgnoringRef

r=me.  Very nice!

Comment 63

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533983 [details] [diff] [review]
patch 4 v3: Add impl for .ref on nsSimpleURI

OK, in _this_ patch you should change the CID of nsSimpleURI, because what Read/Write are doing changed.

The new SetScheme-using setup will always make an extra copy of the scheme string (one from PromiseFlatCString, and one from the assignment)....  Did the old Left() code do that too, in the end?  If not, might be worth reverting that bit.

>@@ -323,22 +366,55 @@ nsSimpleURI::SetPath(const nsACString &p

>+    mPath = Substring(path, 0, PRUint32(hashPos));

  mPath = StringHead(path, hashPos)

>+    PRUint32 refLen = path.Length() - hashPos;
>+    return SetRef(Substring(path, PRUint32(hashPos), refLen));

  return SetRef(Substring(path, hashPos));

should work, right?

r=me with those nits.

Comment 64

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2: Remove QI's to nsIURL

Here's the updated version of patch 5, to remove now-unnecessary QIs to nsIURL that guard GetRef / SetRef calls.

Now available with equalsExceptRef and cloneIgnoringRef tastiness!

Comment 65

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2 (diff -w version)

(diff -w version, for convenience)

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534012 [details] [diff] [review]
patch 5 v2: Remove QI's to nsIURL

>+++ b/content/base/src/nsReferencedElement.cpp
>@@ -104,19 +76,21 @@ nsReferencedElement::Reset(nsIContent* a
>+      nsCOMPtr<nsIURI> bindingDocumentURI =
>+        binding->PrototypeBinding()->DocURI();

That doesn't need to be an nsCOMPtr, right?  Doesn't even need to be a temporary if you want to just pass binding->PrototypeBinding()->DocURI() directly to EqualsExceptRef.

> nsXULContentUtils::MakeElementURI(nsIDocument* aDocument,

I think the changes to this file are unfortunately wrong.  In particular, the !mutableURL codepath is meant to deal with about: URLs, so will fail due to the immutability issues with the new code.  See bug 571598.  We should have tests for this, even, which I assume fail with this patch.

Just revert this change and file a followup bug on doing this better?  I have some ideas for how one could do it...

>+++ b/dom/base/nsLocation.cpp
>+  // XXXdholbert NOTE: If |uri| is immutable, then its clone probably will be,
>+  // too. In that case, we won't actually be returning something "writable",
>+  // despite the name of this method...

That's actually false; in particular the URI of a document is generally immutable no matter what, but the clone is mutable except for about: and the like.  Just remove that comment, please?

> nsLocation::SetHash(const nsAString& aHash)
>-  nsCOMPtr<nsIURI> uri;
>+  nsRefPtr<nsIURI> uri;

Why that change?  Please undo it.

r=me with the above things fixed.

Comment 67

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v3: Move .ref to nsIURI & add stub impls [r=bz][sr=biesi]

(In reply to comment #59)
> How about this [ascii art]

Looks great. Used.

> Also, I'd add _ref.Truncate() to nsNullPrincipalURI::GetRef.

Fixed.

> r=me with those nits.

Thanks! Here's the final version of that patch, then.

Comment 68

[Boris Zbarsky [:bzbarsky]]

> I think the changes to this file are unfortunately wrong.

Ignore that.  I missed you having the whole "SetRef failed" codepath somehow!

Comment 69

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v4: Add impl for .ref on nsSimpleURI [r=bz]

(In reply to comment #63)
> OK, in _this_ patch you should change the CID of nsSimpleURI

Done.

> The new SetScheme-using setup will always make an extra copy of the scheme
> string

You're right, I think this added some overhead, due to the duplicate flattening in SetSpec & SetScheme.  I've now reverted to the old code for parsing the scheme.

> [nicer substring syntax]

Fixed 'em all.

> r=me with those nits.

Thanks!

Comment 70

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2: Remove QI's to nsIURL [r=bz]

(In reply to comment #66)

> just pass binding->PrototypeBinding()->DocURI() directly to EqualsExceptRef.

Done.

> > nsXULContentUtils::MakeElementURI(nsIDocument* aDocument,
> 
> I think the changes to this file are unfortunately wrong.

(ignoring this per comment 68)

> >+++ b/dom/base/nsLocation.cpp
>  Just remove that comment, please?

Done.

> > nsLocation::SetHash(const nsAString& aHash)
> >-  nsCOMPtr<nsIURI> uri;
> >+  nsRefPtr<nsIURI> uri;
> Why that change?  Please undo it.

Done. (Reverted the other identical change in GetHash, too.)

Comment 71

[Daniel Holbert [:dholbert]]

Attachment: patch 6: Fix nsDataHandler::NewURI to accept "#myRef" as a spec [r=bz]

(In reply to comment #48)
>   !spec.IsEmpty()
[...]
>   uri.forget(result);

Fixed both.

> r=me with that.

Thanks!

Comment 72

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 533977 [details] [diff] [review]
patch 3 v3: add nsIURI::equalsExceptRef

Review of attachment 533977 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 533980 [details] [diff] [review]
patch 3.5 v1: add nsIURI::cloneIgnoringRef

Review of attachment 533980 [details] [diff] [review]:
-----------------------------------------------------------------

I'd prefer that you didn't check this in. seems like clone+setRef would work just as well?

longer-term, as bz suggested, we should make URLs immutable and make getters return modified clones, so if you insist that you want this patch we could land this until then, I guess...

Comment 74

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 533980 [details] [diff] [review]
patch 3.5 v1: add nsIURI::cloneIgnoringRef

I'm told about: URIs are always immutable, so they require this

Comment 75

[Daniel Holbert [:dholbert]]

Attachment: patch 7: reftests

This patch has 3 reftests, each with a XHTML document with <embed src="[data URI]>.

There's one test each, for gradient, filter, & pattern.  The former two compare the data URI against their normal rendering, and the latter (pattern) just renders to fully-lime, so I compare it against pass.svg.

I've verified that they all fail in today's nightly & pass in my patched build.

Comment 76

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534127 [details] [diff] [review]
patch 7: reftests

r=me

Comment 77

[Daniel Holbert [:dholbert]]

Landed!

patch 1:    http://hg.mozilla.org/mozilla-central/rev/ee42941252f6
patch 2:    http://hg.mozilla.org/mozilla-central/rev/e9c7616c4f72
patch 3:    http://hg.mozilla.org/mozilla-central/rev/add8d8d67ba8
patch 3.5:  http://hg.mozilla.org/mozilla-central/rev/835e9789e934
patch 4:    http://hg.mozilla.org/mozilla-central/rev/a006860c018d
patch 5:    http://hg.mozilla.org/mozilla-central/rev/655514007ebd
patch 6:    http://hg.mozilla.org/mozilla-central/rev/dbce2f14576e
patch 7:    http://hg.mozilla.org/mozilla-central/rev/76f08a32c046

Comment 78

[Daniel Holbert [:dholbert]]

BTW, I needed to fix some minor bitrot in patch 5 due to colliding with Bug 654399 on changing nsLocation::GetHash.

It turned out to be easy to address, though.  The other bug just added a clause to the end of that method, and I just un-indented that clause along with the rest of that function (now that I've removed the "if (url) {...}" surrounding it).

Comment 79

[[e-mail]]

Does this affect simple nested URIs? (I'm not sure which patch to look at.)

Comment 80

[Boris Zbarsky [:bzbarsky]]

Neil, with this patch simple nested URIs support refs in general if you can get it into the URI object to start with.  NewURI with a relative URI of the form "#foo" probably doesn't work yet; the protocol handlers need to be changed to handle it.  Only the data: protocol handler was changed in this bug.

Comment 81

[David :Bienvenu]

bug 658877 filed for mailnews bustage

Comment 82

[Julian Reschke]

Late comment: I think it would have made more sense to call the new stuff "fragment", because that's what it's called in the specs...

Comment 83

[Julian Reschke]

(In reply to comment #24)
> (In reply to comment #23)
> > > If RFC2397 says nothing special about "#" in data: URI,
> > 
> > The BNF for data: URIs makes no special provisions for stuff that's part of the
> > URI but not part of the data, right?
> 
> This is the part I don't understand. RFC2397 says '[...] "urlchar" is
> imported from [RFC2396]' but there is no BNF grammar for "urlchar" in
> RFC2396. If it means "uric" in RFC2396, then isn't it obvious that it
> doesn't include "#"?

That's a known erratum: see <http://www.rfc-editor.org/errata_search.php?rfc=2397&eid=2045>

Comment 84

[Daniel Holbert [:dholbert]]

(In reply to comment #82)
> Late comment: I think it would have made more sense to call the new stuff
> "fragment", because that's what it's called in the specs...

(Note that "ref" (GetRef/SetRef) has existed on nsIURL since the creation of nsIURL.idl, back in 1999.  This bug just moves that to nsIURI. The newly-added methods use the term "ref" for consistency.)

Comment 85

[Julian Reschke]

(In reply to comment #84)
> (Note that "ref" (GetRef/SetRef) has existed on nsIURL since the creation of
> nsIURL.idl, back in 1999.  This bug just moves that to nsIURI. The
> newly-added methods use the term "ref" for consistency.)

Thanks for the explanation; that makes sense.

Comment 86

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

adding dveditz to cc for security implementation review

Comment 89

[Daniel Holbert [:dholbert]]

Re-adding dev-doc-needed -- we need to update https://developer.mozilla.org/en/data_URIs#Common_problems  (which says that data:[...]#foo isn't supported)

Comment 90

[Julian Reschke]

<https://developer.mozilla.org/en/data_URIs> says:

"No support for anchors, query strings, etc.

    The data portion of a data URI is opaque, so an attempt to use an anchor (a reference to a specific portion of a resource, usually a location in an HTML document, with the syntax <url>#identifier) or a query string (page-specific parameters, with the syntax <url>?parameter-data) with a data URI will just include the anchor or query string in the data the URI represents. For example: ..."

Does this need to be updated?

Comment 91

[Julian Reschke]

(In reply to Julian Reschke from comment #90)
> <https://developer.mozilla.org/en/data_URIs> says:
> 
> "No support for anchors, query strings, etc.
> 
>     The data portion of a data URI is opaque, so an attempt to use an anchor
> (a reference to a specific portion of a resource, usually a location in an
> HTML document, with the syntax <url>#identifier) or a query string
> (page-specific parameters, with the syntax <url>?parameter-data) with a data
> URI will just include the anchor or query string in the data the URI
> represents. For example: ..."
> 
> Does this need to be updated?

It does; "#bottom" does not appear as part of the content anymore.

Suggest:

"No support for query strings

    The data portion of a data URI is opaque, so an attempt to a query string (page-specific parameters, with the syntax <url>?parameter-data) with a data URI will just include the query string in the data the URI represents. For example:

    data:text/html,lots of text...<p><a name%3D"bottom">bottom</a>?arg=val

    This represents an HTML resource whose contents are:

    lots of text...<p><a name="bottom">bottom</a>?arg=val"

The text then could add:

"Note: as of Firefox 6, fragment (anchors) are processed consistent with other URI schemes, thus a bare "#" in the content needs to be escaped as '%23'".

Comment 92

[Boris Zbarsky [:bzbarsky]]

It's a wiki.  ;)  Feel free to make the edits.

I'll put doing that on my todo list too, in case you don't get to it for a bit.

Comment 93

[Julian Reschke]

(In reply to Boris Zbarsky (:bz) from comment #92)
> It's a wiki.  ;)  Feel free to make the edits.
> 
> I'll put doing that on my todo list too, in case you don't get to it for a
> bit.

Done; I just wanted to double check that we agree on the contents :-)

Comment 94

[Daniel Holbert [:dholbert]]

Thanks! I think that addresses the "dev-doc-needed" keyword that I'd re-added in comment 89.

Comment 95

[Michael Donning]

This bug is not fixed.
"SVG patterns, gradients and filters don't work when SVG is loaded from a data: URL" is still true (as of Firefox 7)

Data-Uri Fragments are supported now but the cause for the bug is different:

Loading i.e. filters from a SVG inside data URIs results in a security warning (in the error console). But SVG data URIs for CSS background-images don't fail with a security warning anyhow.

Comment 96

[Masatoshi Kimura [:emk]]

(In reply to Michael Donning from comment #95)
Please give a test case.

Comment 97

[Michael Donning]

Attachment: Testcase demonstrating the use of svg via data URIs in css

The second paragraph/span in this example is invisible. This is due to security restrictions which disallow SVG filters in css via data URIs. SVG Background-images via data URIs are allowed however.

Comment 98

[Michael Donning]

(In reply to Masatoshi Kimura [:emk] from comment #96)
> Please give a test case.

Hi Masatoshi Kimura,
thank you for the fast response.

I uploaded the test case: (#565504)
https://bugzilla.mozilla.org/attachment.cgi?action=edit&id=565504

It shows the use of a css svg-filter (gaussian blur) and an invisible text which is caused by the same filter when used via data URI.
A third line demonstrates the successful use of svg data URIs for a css background-image.

The error console should list a security error concerning the (filter) data URI only.

Comment 99

[Boris Zbarsky [:bzbarsky]]

Michael, this bug was about an SVG document loaded from a data: URI referencing a filter inside itself.  It's fixed.

Your testcase is about a document referencing a filter in an SVG resource document loaded from a data: URI.  Would you mind filing a separate bug on that and CCing me?  SVG resource documents are subject to same-origin checks that could well be blocking this pattern, as you note.

Comment 100

[Michael Donning]

Ok, thank you for the explanation. I now understand that this bug addresses the use of filters in SVG documents whereas my description/testcase adressed the use of SVG filters in CSS/HTML Documents.

I opened BUG 692806