611241 - xul:image with a SVG filter won't display on "about:" pages

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I have a xul:image element, and in CSS I'm attaching a SVG filter (its a greyscale filter) from a separate file. With the filter the image won't display - it displays fine without the filter.

The same code worked a week or two ago (I can't remember when I previously pulled from mozilla-central). I pulled and rebuilt today, and its no longer working.

Noticed in the patch (v5) in bug 601022.

Comment 1

[Robert Longson [:longsonr]]

Cameron, is this down to bug 477171 landing?

Blair can you create a simple testcase please?

Comment 2

[Daniel Holbert [:dholbert]]

Miiiight also be bug 601999 or bug 587336.  (/me slaps self for pushing those + bug 477171 together & making regression-window-tracking for this bug a little tougher)

Comment 3

[Cameron McCormack (:heycam)]

I just tried rev 09d926c185f6 (a couple of days ago) plus with bug 601022 applied, so that's with neither my bug 477171 or the others dholbert checked in together applied, and I could still reproduce the problem.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Simple Testcase (as extension)

I've discovered that loading the extension manager (with bug 601022 applied) from a chrome URL makes the images appear as they should. Its only when loaded as an about: URL that the images will the SVG filter disappears.

So here's a simplified test case. With this extension installed:
* Load chrome://bug611241/content/testcase.xul - both images will appear
* Load about:bug611241 - only the image without the SVG filter will appear (but the other will still take up space)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Isn't that just a duplicate of the general bug about SVG element references not working with URI schemes (like about:) that don't support references?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #5)
> Isn't that just a duplicate of the general bug about SVG element references not
> working with URI schemes (like about:) that don't support references?

I have little experience with SVG, but doesn't that only apply if the SVG element is accessed via such a URI? In this case, the SVG element is always accessed via a chrome:// URI. It's only the page that has an about: URI.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  Hmm.  That's odd.  I assume that this about: URI has the system principal, so you don't see any checkLoadURI errors in your error console?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #7)
> I assume that this about: URI has the system principal, 

Yes.

> so you don't see any checkLoadURI errors in your error console?

I've never seen any such errors, no.

Comment 10

[Daniel Holbert [:dholbert]]

I can reproduce this using Blair's attached extension for as far back as the "about:bug611241" page will load -- that's back to Sept. 11 2010.

Pushlog range for "about:bug611241" no longer loading:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=79d0beec27b5&tochange=73ab2c3c5ad9
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100911 Firefox/4.0b6pre

Removing 'regression' keyword, as it looks like this has been broken for as far back as the testcase is loadable.  (But feel free to restore it if I'm misunderstanding something.)

(I'm guessing that Blair's "The same code worked a week or two ago" assertion in Comment 0 was actually due to a chrome:// vs. about: URI difference, per comment 4.)

Comment 11

[Daniel Holbert [:dholbert]]

Actually, disregard the range I gave in comment 10 -- when I started (again) with a fresh profile and installed Blair's extension, I could go back further.  I'm not sure what caused the 9/10->9/11 boundary in my previous new-profile -- odd.

In any case, now the extension successfully registers its about: page (and reproduces the bug) as far back as 2010-07-02.  The previous day's nightly loads the chrome:// URL fine, but rejects the about: one ("The URL is not valid and cannot be loaded.").

Pushlog range for that is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82edf5bd1abe&tochange=c173731c9d90
I'm guessing that started working due to bsmedberg's mega-push that day (e.g. "ec1c7f00476d Allow a manifest to register contracts and cids in any order[...]")

In any case, my conclusion from comment 10 still stands (not a regression).

Comment 12

[Daniel Holbert [:dholbert]]

I put a breakpoint in "nsExternalResourceMap::RequestResource" -- we hit it for the chrome:// version, but not for the about: URL.

Are about: pages restricted from loading external resources in some way?

Comment 13

[Daniel Holbert [:dholbert]]

Aha: so for about: version, we bail out inside of "nsReferencedElement::Reset" before we get to the "doc->RequestExternalResource" part, because our own document doesn't have a URL.  Relevant code:
> 78 nsReferencedElement::Reset(nsIContent* aFromContent, nsIURI* aURI,
[...]
> 142   nsCOMPtr<nsIURL> documentURL = do_QueryInterface(doc->GetDocumentURI());
> 143   if (!documentURL)
> 144     return;
> 145 
> 146   if (!EqualExceptRef(url, documentURL)) {
> 147     nsRefPtr<nsIDocument::ExternalResourceLoad> load;
> 148     doc = doc->RequestExternalResource(url, aFromContent, getter_AddRefs(load));

In the about: version, we return early at line 144, because documentURL is null.  It looks like we only care about documentURL for the purpose of checking whether the external-resource URL matches it, though -- so maybe we can just drop that early-return...?

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch (dropping the early-return) fixes it.

We might still need to do some equivalent of the "EqualsExceptRef" check, though, to keep us from loading an external document for same-document URI references in an about: page...  (e.g. "filter: url(#myLocalRef)")

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v1b

(In reply to comment #14)
> Created attachment 498433 [details] [diff] [review]
> We might still need to do some equivalent of the "EqualsExceptRef" check,
> though, to keep us from loading an external document for same-document URI
> references in an about: page...  (e.g. "filter: url(#myLocalRef)")

Nope, we don't -- we convert local fragments to URLs by that point -- and even if we happened to not do that, we'd bail out early in this method due to the referenced-element URI ('#foo' or 'about:mypage#foo') failing to QI to a URL. (since those aren't valid URLs)

Comment 16

[Daniel Holbert [:dholbert]]

(new patch just adds some clarifying comments)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 498454 [details] [diff] [review]
fix v1b

>+  // NOTE: aURI shouldn't be just a fragment identifier ('#foo') at this point.
>+  // But if it happens to be, then we'll take the early-return here, because
>+  // it's not a valid URL.

That comment doesn't really make sense.  aURI is an nsIURI.  It can't possibly be "just a fragment identifier" no matter what.  Just take this out, please.

>+  // We've checked that our referenced |url| is a valid nsIURL, so it must be
>+  // more than just a fragment ID like '#foo'. (That wouldn't be a valid URL.)
>+  // So, if |documentURL| is null/invalid (e.g. for "about:" pages), then we
>+  // know that |url| refers to something in a *different* (external) document.

I would phrase this like so:

  // We've already checked that |url| is an nsIURL.  So if the document URI is
  // not an nsIURL then |url| is certainly not going to be pointing to the same
  // document as the document URI.

I would prefer to not bake currently-true-but-we-want-to-change-them details about how about: works into the comments here.

r=me with those comment changes.

Comment 18

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/95bbe1076dcb

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre

Would an automated test be possible?

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #19)
> Would an automated test be possible?

I think so -- we'd just need to set up a testcase (with remote SVG filters) that's registered as an "about:" URI, and visit it at that URI during the test.  (Not having worked on much frontend code, I'm not sure how best to do that in an automated test...)