433616 - [FIX]Need facility for managing documents referenced via local URI references

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

See http://groups.google.com/group/mozilla.dev.tech.layout/msg/7495048ab7a2ffec for some context.

Several applications require loading of DOM subdocuments to serve as resources for a normal document:
-- SVG fonts
-- SVG images
-- SVG external element references (e.g. fill:url(foo.svg#bar) )
-- Proposed extensions that would apply externally-referenced SVG effect elements to HTML
-- HTML documents containing canvas elements, if we want to make effects like Webkit's "CSS Canvas Drawing" available in a generic way ( http://webkit.org/blog/176/css-canvas-drawing/ ) 

It would be great if we had a shared facility for this, it would be great to have for 1.9.1. I guess it would be a dictionary of frameloaders in nsDocument, or something like that. I'm not sure who should implement it.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It would be nice to have some requirements on this documented. For example, do we want to be able to do sharing across documents for these resources? I know we talked about having a copy-on-write functionality so that multiple documents could share the same resource DOM. Though if you need to let scripts reach into these DOMs, and possibly mutate them, I think it really needs to be copy-on-script-reference.

Comment 2

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

The SVG 1.2 Tiny document that Cameron linked to ( http://www.w3.org/TR/SVGMobile12/linking.html#externalReferences ) suggests that for a given master document, resource references with the same relative URI (ignoring the hash field, I guess) should resolve to a single DOM, so a script mutation to one would affect all references. That sounds reasonable to me. We would want references from different master documents to always *behave* as if they resolve to different DOMs. Implementing sharing and copy-on-write would be cool but not necessary.

It would be nice to have a DOM API that lets you get the resource document DOM node for a given master document and relative URI, so that outside scripts could do such mutations, but that feature isn't as important as having the basic facility.

One tricky thing here is preventing cycles --- it's certainly going to be possible and useful for a resource document to have its own resource children.

Comment 4

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

Boris, you seem swamped ... would you like me to get started on this?

My initial approach would be to give each document a hashmap of nsIURIs to
nsFrameLoaders, and proceed from there.

Comment 5

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

BTW there's some discussion here
http://groups.google.com/group/mozilla.dev.tech.svg/browse_thread/thread/5ce022d944bd1b03

Comment 6

[Boris Zbarsky [:bzbarsky]]

So as I see it, we want the following things to happen, at least:

1)  A hashmap from URIs to frame loaders or documents
2)  Disallow script in those frame loaders or documents
3)  Audit various places that assume document or docshells with no parent are
    privileged somehow and see whether those privileges make sense here.
4)  Decide how we want to handle external resources in those frame loaders.
5)  Decide whether cross-origin linking is OK or not (I'd say not, for a start,
    because it could be an information leak).
6)  Investigate whether the security manager code that looks up the docshell
    tree is relevant here and if so what we want to do about it.
7)  Decide whether we actually want frame loaders (complete with docshell, etc)
    or a docshell-less (and content viewer-less) document for which we manually
    construct a presentation (a la printing).  Not sure which is easier to make
    secure, or more precisely to lock down to avoid surprises.

Comment 7

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

(In reply to comment #6)
> 2)  Disallow script in those frame loaders or documents

It seems to me SVG wants scripts to run in external resource documents.
http://www.w3.org/TR/SVGMobile12/linking.html#externalReferences

> 4)  Decide how we want to handle external resources in those frame loaders.

Not sure what you mean. SVG wants a "flat" external resource space, so external references from an external resource are resolved via the same dictionary as the external resource itself. I guess this means external resource documents need a link back to their primary document.

> 5)  Decide whether cross-origin linking is OK or not (I'd say not, for a
> start, because it could be an information leak).

I'd say we should leave it up to the consumer of the resource document. For cases like <use>, patterns and gradients we probably don't want to allow cross-origin. But for plain old SVG images we probably do.

> 7)  Decide whether we actually want frame loaders (complete with docshell,
> etc) or a docshell-less (and content viewer-less) document for which we
> manually construct a presentation (a la printing).  Not sure which is easier
> to make secure, or more precisely to lock down to avoid surprises.

The question is, if a script sets window.location in an external resource document, what happens? I guess if it's to behave normally and load a new document, we need a docshell. That does lead to a weird situation where the resource document for URI A actually has URI B.

I'm not sure how SVG images (<img> or CSS) fit into this; I don't know if any spec says they should be treated as primary or external resources. I'll try to find out.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> It seems to me SVG wants scripts to run in external resource documents.

I'm not sure that's necessarily acceptable from a security point of view.  What script context would they run in?  One per resource document?  This more or less forces us to have frame loaders, windows, etc, with the associated security worries (setting window.location?).

Note that when I brought this up on m.d.t.svg the only responses agreed there should be no script execution here.

We certainly need to support a script-less mode of operation for <svg:img>, and I would argue that it's simpler to use it for everything to start, then relax the restriction as we convince ourselves it's safe than it is to allow it to start.  But if someone sees a safe enough way to allow script, sure.

> Not sure what you mean. SVG wants a "flat" external resource space

Ah, indeed.  My question is whether external resources are allowed _at all_.  Note that one can have non-SVG external resources (stylesheets, images, scripts).  It feels like we do need to allow them, for this to be useful, but it'll take some care to do safely.

Agreed on cross-origin for <img> being allowed.

> The question is, if a script sets window.location in an external resource
> document, what happens?

Right.  ;)

Honestly, I think allowing script in this stuff might just not be feasible in the 1.9.1 timeframe....  I'd love to be proven wrong, of course.

Comment 9

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

Attachment: testcase support SVG file

Comment 10

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

Attachment: HTML testcase with SVG images via <img> and CSS

Comment 11

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

Attachment: SVG testcase referencing the base file as an external resource document

Comment 12

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

This testcase seems to show that Opera doesn't run scripts in <img src="test.svg"> *nor* in fill="url(test.svg#pattern)". So maybe we can get the spec changed to not run scripts in external resource documents, if we need to.

I'm not sure why my SVG background-image in attachment #330705 [details] isn't working in Opera.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*If* we were to run scripts from <img src="svg.svg"> then we *must* make sure that that script can have no effect what so ever on the containing page. Including setting its window.location.

The reason is that lots of forum pages allow commenters to link to images from any origin, assuming that this will not introduce XSS holes. Even setting window.location could be sensitive if images could be included in an iframe that contains a login form since then an attacker could intercept the login and direct it to his site.

I think similar considerations would apply to other ways of linking to resource documents. I think we would keep things a lot safer both for us and for sites using SVG.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Basically, I see two ways to do script in resource documents:

1)  Start with a Window (+ docshell) and lock things down
2)  Start with a different global object (no location, no print, no DOM storage,
    nothing) and add things in one at a time as we deem them safe.

But again, we need a no-script mode for now anyway for <img>.  I would argue that all navigation in the resource document should be disallowed, by the way.  Not that there are lots of ways to navigate without script and without event delivery, but there might be some (HTML meta, etc).

Comment 15

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

Attachment: fixed SVG testcase

Fix the testcase to reference attachment 330704 [details]

Comment 16

[Boris Zbarsky [:bzbarsky]]

Here's an interesting question.  What's the right setting for the canvas size for these external resource documents?

Comment 17

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

That is a very good question. I'd say (0,0). For most (all?) uses of external references it shouldn't matter, but it's up to the code that uses external references to make sure of that. Certainly for paint servers the geometry of the container should not affect the behaviour of the paint server. Don't know about fonts. For SVG images we'll have to essentially resize the content every time we paint it, but we already do things like that in SVG. (Yeah, that won't work for foreignObject in SVG images, which reflows asynchronously, but that's too bad.)

Comment 18

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

If we find cases where the usage of external references really can depend on the canvas size, then we should get that specced, but I can't think of one off the top of my head. (I bet they exist though just due to oversights somewhere in the spec.)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix part 1: Make various code saner about handling null containers and docshells

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 339978 [details] [diff] [review]
Fix part 1: Make various code saner about handling null containers and docshells

The widget init data part here needs particularly careful review, in my opinion.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix part 2: the loader implementation

I could have used nsIRunnable for the loaded notification, if I wanted to store the URI and try fetching the document based on URI again, but using nsIObserver lets me just pass in the newly-loaded document, which I think is cleaner.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix part 3: nsReferencedElement changes and SVG changes needed to use the new stuff

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests

This doesn't include a test for the redirect stuff yet; I'll try to work on that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I did some testing on Mac/Linux/Windows and things at least work, as far as I can tell... ;)

Comment 25

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

+  // If we ever start allowing non-same-origin loads here, we might need to do
+  // something interesting with aRequestingPrincipal even for the hashtable
+  // gets.

You might want to clarify this comment since you do, in fact, allow non-same-origin loads (modulo Access Controls).

Comment 26

[Boris Zbarsky [:bzbarsky]]

I actually don't yet (since we in fact do not have access controls in the tree that I can tell).

If it lands, we'll need to revisit what happens if site A links to external resource at site B (allowed by B) and that links to external resource at site C, which would allow A but doesn't allow B...

Comment 27

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

Ah OK.

Hmm. That's a tricky one. We should probably kick it up to a spec-level discussion.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yeah, that would be good.  Thing is, the SVG spec doesn't place any same-origin restrictions here at all, so I'm not sure whether the SVG WG is the right place for this discussion.

Comment 29

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

We definitely should have it in the SVG WG since UAs should be consistent about how they apply same-origin checks (or not).

Comment 30

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

Comment on attachment 339978 [details] [diff] [review]
Fix part 1: Make various code saner about handling null containers and docshells

+    initData.mContentType =
+      nsContentUtils::IsInChromeDocshell(mDocument) ?
+        eContentTypeUI : eContentTypeContent;

How could we be in a chrome docshell with no mParentWidget?

So an external document doesn't get the device settings of the master document?

In DocumentViewerImpl::SizeToContent, shouldn't we just error out if we're in a docshell-less
document, instead of continuing?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> How could we be in a chrome docshell with no mParentWidget?

See comments on IsInChromeDocshell (which may need a better name).  The second patch in this bug changes its implementation to follow the display document chain, so that we can basically decide whether this is a resource for a chrome or content document, and base this widget type accordingly.

> So an external document doesn't get the device settings of the master document?

At the moment, no.  If you think I should just use the master document device context, I could do that, I guess.

> In DocumentViewerImpl::SizeToContent, shouldn't we just error out

Hmm...  I can see reasons why we might want to do this with external resources, maybe.  But I'm happy going either way here, I think.

Comment 32

[Cameron McCormack (:heycam)]

(In reply to comment #29)
> We definitely should have it in the SVG WG since UAs should be consistent about
> how they apply same-origin checks (or not).

How certain resources are treated with respect to origin sounds like a broader
issue than just in SVG.  Should there be some other document that specifies how
origin checks work on the web, and then SVG can then hook into that by saying
something like "SVG resource documents are foo document references for the
purpose of origin checks (as defined in That Other Spec)"?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Notes from today's security review, for posterity:

* If we ever switch away from pure same-origin to access-control we absolutely
  need to sort out the behavior of the hashtable.
* cross-document <use> is somewhat scary, since as currently written it allows
  the resource document to inject script into the document the <use> is in, via
  on* attributes on the nodes being cloned.  While we're doing same-origin this
  is sort of OK, since the resource document has an XML type and hence could
  just have script or whatever in it already and be loaded by someone malicious
  and run in the domain of the site it's on (in other words, letting people
  upload XML to your site is already unsafe).  But once we start doing
  access-control, any site that allows a user to inject an <svg:use> node
  suddenly becomes subject to an XSS attack.  Before doing access-control we
  will need a method to neuter script inside <use>-cloned fragments somehow.
* Setting the resource documents' loadgroups to the loadgroup of the display
  document is bad, because various callers make assumptions about channels
  based on things they getInterface from the notification callbacks.  In fact,
  I'm pretty sure it can be used to perform (sandboxed) script execution via
  javascript: URIs in some cases.  I'll double-check and attach tests for
  this.  In any case, tomorrow I will focus on sorting out what we actually
  want the loadgroup behavior to be here; I suspect we'll want to give all the
  resource documents individual loadgroups, put them all inside the display
  document's loadgroup, and set up a notification callback that forwards some,
  but not all, interfaces to the docshell (certainly the various event sinks;
  need to look through the others)..

Comment 34

[Boris Zbarsky [:bzbarsky]]

Cameron, there are such definitions already, but since SVG is adding the additional requirement here that the document retrieval action is only performed once, even across sources with different origins, it more or less needs to define how that should work with same-origin checks...  In other words, there is normative SVG spec text that is not compatible with any sort of same-origin checking here (in fact, technically the same-origin restrictions I'm adding are not allowed per SVG spec as I read it), and that normative text will need adjusting somehow.

Comment 35

[Cameron McCormack (:heycam)]

OK.  (I assume this is at least part of the reason Opera doesn't do external <use> yet.)

In 1.2T there is a line in the reference restriction table that says that <use> elements can reference external elements only if they do not contain any scripting:

  http://dev.w3.org/SVG/profiles/1.2T/publish/linking.html#ReferenceRestrictions

That's not quite the same as restricting the references to same-origin documents only, but maybe there was an intent there to do something like that?

Anyway, whatever results for sane origin checking that you find are necessary we'd likely be happy to include in some form.

Comment 36

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

(In reply to comment #31)
> At the moment, no.  If you think I should just use the master document device
> context, I could do that, I guess.

I actually think you should. But I guess that would complicate zooming since we'd have to RebuildAllStyleData on the external document presshells. Then again, we probably need this to handle zooming correctly anyway.

> > In DocumentViewerImpl::SizeToContent, shouldn't we just error out
> 
> Hmm...  I can see reasons why we might want to do this with external resources,
> maybe.  But I'm happy going either way here, I think.

I see what you mean, but I think you should error out for now and then if we need that functionality for external resources, we can reenable *and test it* then.

Comment 37

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

(In reply to comment #35)
> OK.  (I assume this is at least part of the reason Opera doesn't do external
> <use> yet.)

We're also applying same-origin checking to all other external document references. I haven't checked what Opera does.

Note that if you don't do same-origin checks on <use>, then not only are you open to XSS attacks from the destination of the <use>, but you can also get cross-origin information leakage where visiting evil.com triggers a <use> of content from some XML (or at least SVG) document in intranet.org. Even though the <use> clones are anonymous they can still be interrogated in various ways, especially the SVGElementInstance interface.

There are probably other issues with other kinds of cross-origin external document references. For example, external clip-path references can be interrogated with hit-test APIs like elementFromPoint. External masks, filters and paint-servers can also be interrogated with combinations of pointer-events and elementFromPoint. I don't have enough imagination to produce a killer exploit based on those tricks, but it's enough to make one nervous.

> In 1.2T there is a line in the reference restriction table that says that
> <use> elements can reference external elements only if they do not contain any
> scripting:
>  
> http://dev.w3.org/SVG/profiles/1.2T/publish/linking.html#ReferenceRestrictions

That sounds hard to enforce. What if the external document is loading incrementally? The UA has to wait for it to completely load to do those checks before binding the <use>?

> That's not quite the same as restricting the references to same-origin
> documents only, but maybe there was an intent there to do something like that?

Same-origin checking sounds better than ad-hoc restrictions based on the content of the external document.

Comment 38

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

BTW, I've always assumed that in SVG 1.1 URI references were supposed to support non-local references in general. In
http://www.w3.org/TR/SVG/struct.html#HeadOverview
some rules say "can reference any local or non-local resource" and other rules say, e.g., "must reference an XYZ element", but I've always assumed that the latter implied "local or non-local" as well. Is that true, Cameron?

In particular I see that SVG 1.2 Tiny forbids references to external paint servers, where I'd assumed SVG 1.1 intended to allow such references. There's no contradiction there, since SVG 1.1 and 1.2T are different beasts, but I want to make sure we're not running against the intention of SVG 1.1 when we enable these features.

Comment 39

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

(In reply to comment #33)
> * cross-document <use> is somewhat scary, since as currently written it allows
>   the resource document to inject script into the document the <use> is in, via
>   on* attributes on the nodes being cloned.  While we're doing same-origin this
>   is sort of OK, since the resource document has an XML type and hence could
>   just have script or whatever in it already and be loaded by someone malicious
>   and run in the domain of the site it's on (in other words, letting people
>   upload XML to your site is already unsafe).  But once we start doing
>   access-control, any site that allows a user to inject an <svg:use> node
>   suddenly becomes subject to an XSS attack.  Before doing access-control we
>   will need a method to neuter script inside <use>-cloned fragments somehow.

That sounds really fragile. I'd argue strongly that <use> should always be same-origin-only even if we allow Access Controls for other kinds of external references.

Comment 40

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

(In reply to comment #35)
> Anyway, whatever results for sane origin checking that you find are necessary
> we'd likely be happy to include in some form.

IMHO the best way to proceed in the spec and in the code is to start out being conservative and require same-origin checks on everything, then later relax constraints in particular situations that are found to be useful and safe.

Doing it the other way around --- permitting everything and then introducing same-origin checks or other checks later when problems are found --- is bad for security and bad for compatibility.

Comment 41

[Cameron McCormack (:heycam)]

(In reply to comment #37)
> > http://dev.w3.org/SVG/profiles/1.2T/publish/linking.html#ReferenceRestrictions
> 
> That sounds hard to enforce. What if the external document is loading
> incrementally? The UA has to wait for it to completely load to do those checks
> before binding the <use>?

Yes I don't think it makes a lot of sense to define it that way.  Better would be to allow the reference but to disable the scripts, etc.

> Same-origin checking sounds better than ad-hoc restrictions based on the
> content of the external document.

Agreed.

(In reply to comment #38)
> BTW, I've always assumed that in SVG 1.1 URI references were supposed to
> support non-local references in general. In
> http://www.w3.org/TR/SVG/struct.html#HeadOverview
> some rules say "can reference any local or non-local resource" and other rules
> say, e.g., "must reference an XYZ element", but I've always assumed that the
> latter implied "local or non-local" as well. Is that true, Cameron?

Yes I believe the intention is to allow external references in 1.1, it's a handy feature.  The problem is just that the spec says nothing of origin restrictions, and it seems to be assumed that the implementor will make sensible choices.

> In particular I see that SVG 1.2 Tiny forbids references to external paint
> servers, where I'd assumed SVG 1.1 intended to allow such references. There's
> no contradiction there, since SVG 1.1 and 1.2T are different beasts, but I want
> to make sure we're not running against the intention of SVG 1.1 when we enable
> these features.

Some of the things in 1.2T are clarifications/improvements of 1.1, while others are restrictions that were included specifically because of the mobile (constrained memory or CPU or whatever) use case.  (Though as Doug points out to me, the longer 1.2T takes to get done, the less relevant those restrictions are.)  The external paint server restriction I think is one of those mobile restrictions, rather than a clarification of 1.1 behaviour.

Comment 42

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

(In reply to comment #37)
> There are probably other issues with other kinds of cross-origin external
> document references. For example, external clip-path references can be
> interrogated with hit-test APIs like elementFromPoint. External masks, filters
> and paint-servers can also be interrogated with combinations of pointer-events
> and elementFromPoint. I don't have enough imagination to produce a killer
> exploit based on those tricks, but it's enough to make one nervous.

Actually, using <filter> to munge color channels and pointer-events and elementFromPoint for hit-testing of alpha values, untrusted content can recover the exact pixel values for the rendering of arbitrary elements! Well, depending on your exact interpretation of some subtleties of the pointer-events spec. I'll post to www-svg about that. Fortunately we don't implement pointer-events yet.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> http://dev.w3.org/SVG/profiles/1.2T/publish/linking.html#ReferenceRestrictions

Some of the restrictions here (e.g nested externally referenced <use> being prohibited) seem pretty arbitrary and a pain to enforce...

Always doing same-origin on <use> sounds fine to me.

I'll look into the device context stuff.

Comment 44

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

Comment on attachment 339979 [details] [diff] [review]
Fix part 2: the loader implementation

You should document somewhere what a "display document" means.

+  if (doc &&
+      ((aContentType != nsIContentPolicy::TYPE_DTD && doc->IsLoadedAsData()) ||
+       ((aContentType == nsIContentPolicy::TYPE_OBJECT ||
+         aContentType == nsIContentPolicy::TYPE_DOCUMENT ||
+         aContentType == nsIContentPolicy::TYPE_SUBDOCUMENT ||
+         aContentType == nsIContentPolicy::TYPE_SCRIPT )&&
+        doc->GetDisplayDocument()))) {

This is so unreadable I might as well be programming in LISP :-). Please simplify
the expression somehow.

+   * Add an ExternalResource for aURI.  aViewer might be null when this is
+   * called.  This function makes sure to remove the pending load for aURI, if

Document what might cause aViewer to be null?

+  PRBool mHaveShutDown;

PRPackedBool (doesn't really matter due to alignment, but PRBool in structs always seems to
get forgotten later)

Comment 45

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

Comment on attachment 339979 [details] [diff] [review]
Fix part 2: the loader implementation

In SetupViewer, all the stuff around nsIDocumentLoaderFactory looks evil, but I suppose that's
just how it is.

Comment 46

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

Comment on attachment 339980 [details] [diff] [review]
Fix part 3: nsReferencedElement changes and SVG changes needed to use the new stuff

+      if (mPendingNotification) {
+        load->AddObserver(observer);

Make it "if (observer)" for clarity.

Comment 47

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

Comment on attachment 339981 [details] [diff] [review]
Tests

These tests are OK.

My patch in bug 80713 extends reftests to support cross-origin checking --- maybe you could steal that and create a cross-origin test or two.

Might be worth testing a cycle that crosses external documents ... e.g. main document references a mask a.svg#m1 which references b.svg#m2 which references a.svg#m1. It could be a crashtest, but it will at least check that the external document namespace is flattened correctly, and that cycle detection still works.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> In SetupViewer, all the stuff around nsIDocumentLoaderFactory looks evil

Yeah, it's sad.  I've addressed the issues you raise in comment 44 and comment 46, and the SizeToContent issue.

For tests, I do have a test for cyclical <use>, I think.  I agree that having a mask test would be good too; I'll see what I can put together.  And yes, I was going to write a mochitest for the cross-origin stuff, but if I can do it using reftest so much the better.

Would you prefer a separate diff for the loadgroup changes, or just roll those into the "loader implementation" diff?

Comment 49

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

Just roll them up. jst's going to have to take care of that anyway.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated part 1

This just addresses the review comments, except the device context thing (which depends on GetDisplayDocument(), so lives in the next diff).

Comment 51

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to comments

Adds the loadgroup magic (a bit of test to come in a later attachment), as well as the various device context and zoom level munging.  I filed bug 457153 on the cookie ickiness.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to comments and now passing in node instead of principal

This already has r+sr=roc; I didn't make any other changes to it.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional fixes for various issues found

This has several things going on:

1) Fix a long-standing bug where the XML parser would abort on any failure to
   load a stylesheet linked from the document (say because the security manager
   or content policy blocked it).  We've had issues with this before, and it was
   biting one of the testcases for this bug.
2) Fix a base URI bug in <use> that was biting <http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-struct-use-05-b.html>.
3) Change assert to warning in nsJSChannel, since we can hit that codepath now.
4) Adds test for javascript: loads from resource documents; this was in fact
   failing without the loadgroup changes (was running the JS).  Now it passes,
   which is what necessitated changes 1 and 3.
5) Add the SVG testsuite test as a reftest, more or less.

roc, I tried to test recursive masks, but any time a shape in a mask has a mask style on it we seem to completely disappear that shape, so I can't even get a chain of length 2 in masks.  Am I just missing something?

Comment 54

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

Comment on attachment 340512 [details] [diff] [review]
Additional fixes for various issues found

+    if (!globalOwner) {
+        NS_WARNING("Unable to get an nsIScriptGlobalObjectOwner from the "
+                   "channel!");
+    }
     if (!globalOwner) {
         return nsnull;
     }

Fuse these two ifs

+<?xml-stylesheet
+   type="text/css"
+   href="javascript:'circle { fill: black }'" ?>

How does this test work? Some kind of error propagates out?

Comment 55

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

(In reply to comment #53)
> roc, I tried to test recursive masks, but any time a shape in a mask has a mask
> style on it we seem to completely disappear that shape, so I can't even get a
> chain of length 2 in masks.  Am I just missing something?

No, I think it must be a bug. I'll file it. Nevertheless you should be able to get a "WARNING: mask loop detected" on the console :-).

Comment 56

[Boris Zbarsky [:bzbarsky]]

> How does this test work?

If the javascript: runs, then it's equivalent to having

  <style> circle { fill: black } </script>

in the SVG, which means that it changes that part of the mask from white to black and causes the reftest to fail.

I do get the console warning with a testcase as described in comment 47.

Comment 57

[Johnny Stenback (:jst)]

Comment on attachment 340509 [details] [diff] [review]
Part 2 updated to comments

Looks good to me.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Pushed, in order:
changeset 2ea77fa990af
changeset 7d2d8869e73c
changeset 93441e4f521e
changeset 43d2ee100f3b
changeset a2d84821ea71

I still need to write a test for the cross-origin stuff, but this is fixed.

Comment 59

[Boris Zbarsky [:bzbarsky]]

This caused a Windows Txul and Tp regression, so I backed it out....

Comment 60

[Boris Zbarsky [:bzbarsky]]

Oh, and the regression was very distinctly Windows-only, so it must be something about gfx/widget here...  And I'm guessing something from the first diff, since Tp doesn't exercise any of the other stuff.

I've relanded the first diff to test this theory, but the only thing I can see in there offhand that might cause this is the fact that we now create widget-less device contexts on Init() if no widget is passed in.  Turns out, that's somewhat common; see the XXX comment about Init/Open/Show that I added in the second diff.  I timed that Init() call in my Windows VM, and it's fast (under 1ms, for sure).  So it shouldn't have caused the Tp regression, I would think.  But as an experiment, we could also try bailing out of here if aWidget is null and !mDocument->GetDisplayDocument(), I guess.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Well, it looks like relanding just part 1 has cycled and there is no performance regression.

By the way, I just realized that when I backed this out, I failed to back out the actual test files for the crashtest, so the tests patch needs to have the very end lopped off to apply cleanly for the relanding.

Comment 62

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

I relanded part 2. We'll see what happens to that.

Comment 63

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

That seemed to cause the regression. dbaron backed it out.

Comment 64

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

Part 2 is now relanded a second time:
http://hg.mozilla.org/mozilla-central/rev/5df4c3d437ee
http://hg.mozilla.org/mozilla-central/rev/dc71e84d6cb6

Comment 65

[Boris Zbarsky [:bzbarsky]]

Relanded the rest of the changes as well.

Comment 66

[georgi - hopefully not receiving bugspam]

is <img src="svg.svg"> checked in?

can't make it work neither from xhtml nor from svg

Comment 67

[Boris Zbarsky [:bzbarsky]]

> is <img src="svg.svg"> checked in?

No.

Comment 68

[Eric Shepherd [:sheppy]]

After looking over the comments here and through the patches, I no longer believe this impacts developer documentation; looks like this is just a matter of changing how URI references are maintained internally. If that's not correct, let me know and re-apply the doc needed keyword.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Yeah, that's not correct.  This bug added the ability to do cross-document paint servers and such in SVG, basically.  Before that you could only use same-document URI references.

Comment 70

[Eric Shepherd [:sheppy]]

No idea what a "paint server" is. I'll have to investigate this further eventually.

Comment 71

[Robert Longson [:longsonr]]

http://www.w3.org/TR/SVGTiny12/painting.html#PaintServers